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. Kind regards, Daniel
