You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Sahlberg <da...@gmail.com> on 2024/08/12 20:38:17 UTC
CMake - SQLiteAmalgamation or not
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?
Kind regards,
Daniel
Re: CMake - SQLiteAmalgamation or not
Posted by Timofei Zhakov <ti...@chemodax.net>.
On Sat, Aug 17, 2024 at 6:22 PM Daniel Sahlberg
<da...@gmail.com> wrote:
>
> Den mån 12 aug. 2024 kl 23:12 skrev Timofei Zhakov <ti...@chemodax.net>:
>>
>> On Mon, Aug 12, 2024 at 11:39 PM Daniel Sahlberg
>> <da...@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
Re: CMake - SQLiteAmalgamation or not
Posted by Branko Čibej <br...@apache.org>.
On 17. 8. 24 18:22, Daniel Sahlberg wrote:
> 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.
Using the bundled versions should always be the fallback, not the
default[1]. The utf8proc example is a good one, this library contains
unicode data that changes with every new Unicode version, and we
probably don't keep up with these changes in the bundled version.
-- Brane
[1] Except on Windows, where using the bundled version is much simpler.
Re: CMake - SQLiteAmalgamation or not
Posted by Daniel Sahlberg <da...@gmail.com>.
Den mån 12 aug. 2024 kl 23:12 skrev Timofei Zhakov <ti...@chemodax.net>:
> On Mon, Aug 12, 2024 at 11:39 PM Daniel Sahlberg
> <da...@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
Re: CMake - SQLiteAmalgamation or not
Posted by Timofei Zhakov <ti...@chemodax.net>.
On Mon, Aug 12, 2024 at 11:39 PM Daniel Sahlberg
<da...@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. There is also a similar situation with lz4 and
utf8proc libraries, which could be used from internal and as external
libraries.
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.
--
Timofei Zhakov