On Sat, Aug 17, 2024 at 6:22 PM Daniel Sahlberg <daniel.l.sahlb...@gmail.com> wrote: > > Den mån 12 aug. 2024 kl 23:12 skrev Timofei Zhakov <t...@chemodax.net>: >> >> On Mon, Aug 12, 2024 at 11:39 PM Daniel Sahlberg >> <daniel.l.sahlb...@gmail.com> wrote: >> > >> > Hi, >> > >> > One question on the SQLiteAmalgamation code. >> > >> > There is currently a configuration option to request to use an >> > amalgamation for SQLite (SVN_SQLITE_USE_AMALGAMATION), defaulting to ON. >> > If you don't have an amalgamation downloaded (in the sqlite-amalgamation >> > directory), configuration will fail unless you set the option to OFF. >> > >> > The old ./configure script had the logic to use the amalgamation if >> > available or else check for a library. >> > >> > Is there a reason for defaulting to the amalgamation and requiring an >> > explicit option to NOT use it? >> > >> > With something like below, it seems the build would check for the >> > amalgamation and use it if found and otherwise check for a library. >> > >> > [[[ >> > Index: CMakeLists.txt >> > =================================================================== >> > --- CMakeLists.txt (revision 1919836) >> > +++ CMakeLists.txt (working copy) >> > @@ -228,8 +228,8 @@ >> > >> > ### SQLite3 >> > >> > -if(SVN_SQLITE_USE_AMALGAMATION) >> > - find_package(SQLiteAmalgamation REQUIRED) >> > +find_package(SQLiteAmalgamation) >> > +if(SQLiteAmalgamation_FOUND) >> > add_library(external-sqlite ALIAS SQLite::SQLite3Amalgamation) >> > else() >> > find_package(SQLite3 REQUIRED) >> > ]]] >> > >> > WDYT? >> >> Hi, >> >> I also met this problem, especially on Linux, because I use the >> amalgamation on Windows and it works without any modifications or >> additional options. The current logic has existed since the initial >> version of the CMake and I didn't change anything there, because it >> was simpler to implement at the beginning. >> >> I would agree with the logic suggested, but my idea was to make less >> implicit options for build to be more repeatable, clear, and >> understandable. For example, to enable ra-serf, the user would have to >> explicitly pass the -DSVN_ENABLE_RA_SERF=ON option, even if it is >> installed on the system. However the case with SQLite differs a bit >> from the Serf one. > > > I understand the logic of not enabling things automatically just because the > corresponding library is installed on the system. (Even though I'm personally > annoyed by not having RA_SERF available since I almost always use http/https > access methods, but I can live with setting it to ON). > >> >> There is also a similar situation with lz4 and >> utf8proc libraries, which could be used from internal and as external >> libraries. > > > Those are a bit different, since those are distributed in the tarball so we > can't check it the same way as with SQLite Amalgamation. Probably good to > have them as an option. I see they are on by default. I know at least the > utf8proc lib has some updates but I don't know if there are advantages in not > using the built in version. > >> >> >> The other approach would be to change the default of the >> SVN_SQLITE_USE_AMALGAMATION to OFF to require the installed SQLite >> version. This should be okay, because usually only the advanced users >> or packagers use the amalgamation. However the amalgamation is simpler >> to use on WIndows than the installed one, because it doesn't require >> any compilation, so the other users could use it and it might be >> better to check both versions in this case for sure. > > > I can't say I have a strong feeling about the default value. Either it is ON > by default (and you have to download and install the amalgamation) or it is > OFF by default (and you have to install the libsqlite3-dev package, vcpkg for > the Windows world, or set it to ON and download/install the amalgamation). > Either way there is at least one manual step whenever you build from scratch.
Hi, I committed a resolution of the problem as revision 1920296 with a bit alternative approach of combining the options and the checks to get an availability of reproducible builds and great user experience for the users at the same time. I think this fixes the issue. Additionally, I have an idea to do something similar with ra-serf. However, it is going to be a bit more complicated to implement than the case with SQLite. What do you think? Thanks! -- Timofei Zhakov
