Opened 19 months ago

Closed 19 months ago

Last modified 18 months ago

#29002 closed enhancement (fixed)

spkg-configure.m4 for sqlite

Reported by: mjo Owned by:
Priority: major Milestone: sage-9.1
Component: build: configure Keywords:
Cc: embray, dimpase, ​isuruf Merged in:
Authors: Michael Orlitzky, Matthias Koeppe Reviewers: Dima Pasechnik, Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: b261b31 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

SQLite is another package that many people will have installed (firefox uses it, for example) and that has few dependencies in Sage (only readline).

libsqlite3 is used by Python during its build. Some Python packages need sufficiently new versions. We set the lower bound slightly lower than the current version available on XCode.

We detect it using AC_RUN_IFELSE because on macOS without homebrew, there is no pkgconfig file.

Change History (91)

comment:1 Changed 19 months ago by mjo

  • Branch set to u/mjo/ticket/29002
  • Commit set to e5e79c9396db02a5fa5de3f3990afb25f6f1420d
  • Status changed from new to needs_review

I don't think we need such a new version, but 3.29.0 (from 2019-07-10) is the oldest one in Gentoo that I can test against. Should we accept versions back to 3.27 or 3.16 (or further) for Debian?


New commits:

e5e79c9Trac #29002: new spkg-configure.m4 for sqlite.

comment:2 Changed 19 months ago by dimpase

I'm testing this with sqlite 3.20 on Fedora 26 now. (My Debian system has 3.27).

comment:3 Changed 19 months ago by dimpase

  • Status changed from needs_review to positive_review

could you make the minimal version 3.20? Otherwise, all good. (I modified the version being 3.20 and tested with such sqlite)

On Fedora the packages are named sqlite and sqlite-devel - these ought to be listed in the corresponding docs - perhaps on another ticket.

comment:4 Changed 19 months ago by dimpase

  • Reviewers set to Dima Pasechnik

comment:5 Changed 19 months ago by git

  • Commit changed from e5e79c9396db02a5fa5de3f3990afb25f6f1420d to 61d78220089040d4c91ee5b52df63d29107e80aa
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

61d7822Trac #29002: lower the version bound on the system sqlite check.

comment:6 Changed 19 months ago by mjo

Done, but the commit hook set this back to needs_review.

comment:7 Changed 19 months ago by dimpase

  • Status changed from needs_review to positive_review

comment:8 Changed 19 months ago by chapoton

missing author full name please

comment:9 Changed 19 months ago by mjo

  • Authors set to Michael Orlitzky

Sorry, I'm new at this.

comment:10 Changed 19 months ago by mkoeppe

  • Status changed from positive_review to needs_work

This does not accept the system sqlite3 on macOS because we are installing readline -- but that is irrelevant because the system sqlite3 is linked against libedit, not readline.

comment:11 Changed 19 months ago by mkoeppe

egret:~/s/sage/sage-rebasing/worktree-venv (t/27824/public/27824-python3-spkg-configure *$)$ otool -L /usr/lib/libsqlite3.dylib 
/usr/lib/libsqlite3.dylib:
	/usr/lib/libsqlite3.dylib (compatibility version 9.0.0, current version 308.4.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.0.0)
egret:~/s/sage/sage-rebasing/worktree-venv (t/27824/public/27824-python3-spkg-configure *$)$ otool -L /usr/bin/sqlite3 
/usr/bin/sqlite3:
	/usr/lib/libncurses.5.4.dylib (compatibility version 5.4.0, current version 5.4.0)
	/usr/lib/libedit.3.dylib (compatibility version 2.0.0, current version 3.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.0.0)

comment:12 follow-up: Changed 19 months ago by mkoeppe

Is the executable sqlite3 used at all in Sage? That is the only thing that depends on readline on Linux. On Debian, libsqlite3.so and /usr/bin/sqlite3 are provided by different packages.

comment:13 follow-up: Changed 19 months ago by dimpase

how does pkg-config even know about MacOS's sqlite3?

comment:14 in reply to: ↑ 12 Changed 19 months ago by mkoeppe

Replying to mkoeppe:

Is the executable sqlite3 used at all in Sage?

Apart from providing the shortcut sage -sqlite, apparently not.

Should the spkg-configure check for the availability of the binary?

comment:15 in reply to: ↑ 13 Changed 19 months ago by mkoeppe

Replying to dimpase:

how does pkg-config even know about MacOS's sqlite3?

homebrew's pkg-config knows it.

comment:16 Changed 19 months ago by dimpase

then what does prevent one from having a homebrew-installed readline?

comment:17 Changed 19 months ago by mkoeppe

Anything regarding readline is irrelevant for macOS's sqlite.

comment:18 Changed 19 months ago by mkoeppe

By the way, I don't quite get the point of SAGE_SPKG_DEPCHECK here at all. If we install our own readline, how is that relevant for the functioning of the system's sqlite3 package?

comment:19 follow-up: Changed 19 months ago by mjo

For now, the sage sqlite package depends on readline, so the dep check is the right thing to do.

But, if sage doesn't care whether or not sqlite was built with readline support, then why don't we build it without readline support and remove the dependency from the spkg? Afterwards we would drop the dep check here, too, and everything would work out.

Personally, I would say that's a good idea, but I know there's a dissenting opinion that sage should install an independently-usable sqlite package, and that we should insist that the version on the system is comparable to the spkg.

comment:20 in reply to: ↑ 19 Changed 19 months ago by mkoeppe

Replying to mjo:

For now, the sage sqlite package depends on readline, so the dep check is the right thing to do.

To my understanding, PKG_CHECK_MODULES is used to guard specifically against link-time confusion of libraries and their dependencies.

But, if sage doesn't care whether or not sqlite was built with readline support, then why don't we build it without readline support and remove the dependency from the spkg? Afterwards we would drop the dep check here, too, and everything would work out.

Actually, the sqlite3 on macOS is fully functional - it just provides command line editing through libedit, not libreadline.

comment:21 Changed 19 months ago by mkoeppe

By the way, on macOS, I get

pkg-config --modversion sqlite3
3.8.10.2

Is that too old?

comment:22 Changed 19 months ago by mkoeppe

I would propose the following change (updated):

  • build/pkgs/sqlite/spkg-configure.m4

    diff --git a/build/pkgs/sqlite/spkg-configure.m4 b/build/pkgs/sqlite/spkg-configure.m4
    index 00bf6cb14a..44c37e57cb 100644
    a b  
    11SAGE_SPKG_CONFIGURE([sqlite], [
    2   SAGE_SPKG_DEPCHECK([readline], [
    32    PKG_CHECK_MODULES([SQLITE],
    4                       [sqlite3 >= 3.20.0],
    5                       [],
     3                      [sqlite3 >= 3.8.10],
     4                      [AC_CHECK_PROG([SQLITE3], sqlite3, [],
     5                        [AC_MSG_WARN([libsqlite3 is found, so we do not install the sqlite spkg, but sqlite3 is not found, so "sage -sqlite3" will not work])])],
    66                      [sage_spkg_install_sqlite=yes])
    7   ])
    87])
Last edited 19 months ago by mkoeppe (previous) (diff)

comment:23 Changed 19 months ago by mkoeppe

I note that the previous update tickets for the sqlite spkg were only motivated by build system issues, or because "it's time to upgrade". So I don't think the version has to be very new to work for any of our code. For example:

  • #27303 - Upgrade sqlite3 to 3.27.1
  • #22687- update sqlite to version 3.17
  • #16098 - Update sqlite to 3.8.4.3
Last edited 19 months ago by mkoeppe (previous) (diff)

comment:24 Changed 19 months ago by dimpase

AC_CHECK_PROG is not used correctly in comment:22 - 3rd and 4rd parameters of it are values to be assigned to the variable from the 1st parameter, not actions to be performed...

comment:25 follow-up: Changed 19 months ago by mjo

Replying to mkoeppe:

By the way, on macOS, I get

pkg-config --modversion sqlite3
3.8.10.2

Is that too old?

Yeah, the lower bound is at 3.20 right now.

I would propose the following change (updated)

Ok, I agree. The dependency on readline *replaces* a dependency on editline (you have to pick one), so simply dropping the dependency from the sqlite package is not the option I thought it was. Moreover, readline/editline accomplish the same thing, so the dep check is counterproductive here.

I don't think we even need to check for the sqlite3 executable. Zero people will run the system sqlite using sage -sqlite3, and even fewer than zero people read the ./configure output.

comment:26 follow-ups: Changed 19 months ago by dimpase

cryptominisat package plays with SQL, in fact it can generate some SQL data, and sqlite3 is listed as its dependency.

comment:27 in reply to: ↑ 26 ; follow-up: Changed 19 months ago by mjo

Replying to dimpase:

cryptominisat package plays with SQL, in fact it can generate some SQL data, and sqlite3 is listed as its dependency.

Right, and we still have the pkg-config check to ensure that sqlite3 is installed. Are there any distros that install it *without* the CLI? I doubt it... and that's the only reason we would need an extra check for it.

comment:28 in reply to: ↑ 25 ; follow-up: Changed 19 months ago by mkoeppe

Replying to mjo:

Replying to mkoeppe:

pkg-config --modversion sqlite3
3.8.10.2

Is that too old?

Yeah, the lower bound is at 3.20 right now.

I can see that - but my question is why.

comment:29 in reply to: ↑ 28 ; follow-up: Changed 19 months ago by mjo

Replying to mkoeppe:

I can see that - but my question is why.

Mainly just because the further back in time we go, the less valid my testing with v3.29 becomes. New commits incoming.

comment:30 in reply to: ↑ 27 ; follow-up: Changed 19 months ago by mkoeppe

Replying to mjo:

Are there any distros that install it *without* the CLI? I doubt it... and that's the only reason we would need an extra check for it.

Yes, if one installs libsqlite3-dev on Debian, the CLI is not installed.

comment:31 in reply to: ↑ 29 Changed 19 months ago by mkoeppe

Replying to mjo:

Replying to mkoeppe:

I can see that - but my question is why.

Mainly just because the further back in time we go, the less valid my testing with v3.29 becomes. New commits incoming.

Tests of what? I can test on macOS if necessary. I think we should support the default libsqlite that is shipped with the current XCode on macOS.

comment:32 Changed 19 months ago by git

  • Commit changed from 61d78220089040d4c91ee5b52df63d29107e80aa to d32194842f5f7f83c2d50d020267838176a666f3

Branch pushed to git repo; I updated commit sha1. New commits:

961f850Trac #29002: lower the version bound on the system sqlite check (again).
d321948Trac #29002: drop the readline dependency check for system sqlite.

comment:33 in reply to: ↑ 26 Changed 19 months ago by mkoeppe

Replying to dimpase:

cryptominisat package plays with SQL, in fact it can generate some SQL data, and sqlite3 is listed as its dependency.

But no specific version of sqlite is tested for in cryptominisat's build system, as far as I can see.

comment:34 in reply to: ↑ 30 ; follow-up: Changed 19 months ago by mjo

Replying to mkoeppe:

Yes, if one installs libsqlite3-dev on Debian, the CLI is not installed.

JFC. Should we fall back to the spkg if no CLI is found, then? I think Dima's point is that people will want to run the sqlite CLI in one way or another, to e.g. look at the cryptominisat data.

comment:35 in reply to: ↑ 34 Changed 19 months ago by mkoeppe

Replying to mjo:

Should we fall back to the spkg if no CLI is found, then? I think Dima's point is that people will want to run the sqlite CLI in one way or another, to e.g. look at the cryptominisat data.

Fine with me.

comment:36 Changed 19 months ago by mjo

Hmmm, can you try a full build on macOS against the system sqlite now? Since the spkg python also links against readline, I'm not 100% sure there won't be collisions between the readline symbols from python and the editline symbols from sqlite. Hopefully the linkage is isolated to the CLI and it won't matter.

comment:37 Changed 19 months ago by mkoeppe

As you can see in comment 11, the library is not linked against libedit, only the executable is.

comment:38 Changed 19 months ago by dimpase

I think I'm fine with not checking for existence of sqlite3 executable at all.

comment:39 Changed 19 months ago by dimpase

  • Status changed from needs_work to positive_review

comment:40 Changed 19 months ago by mkoeppe

  • Reviewers changed from Dima Pasechnik to Dima Pasechnik, Matthias Koeppe

comment:41 Changed 19 months ago by mjo

Ok, I think it's safe too. If anyone shows up to complain later, the leg work is already done. We just have to add an additional AC_CHECK_PROG for sqlite3 and install the spkg if not found.

comment:42 follow-up: Changed 19 months ago by embray

  • Status changed from positive_review to needs_work

I agree that the sqlite3 CLI should not be required. But this doesn't seem to work for me; my Ubuntu doesn't install a sqlite.pc? Couldn't we also fall back on a AC_CHECK_LIB?

comment:43 Changed 19 months ago by dimpase

Erik, isn't your Ubuntu past EOL?

comment:44 in reply to: ↑ 42 ; follow-up: Changed 19 months ago by mkoeppe

Replying to embray:

I agree that the sqlite3 CLI should not be required. But this doesn't seem to work for me; my Ubuntu doesn't install a sqlite.pc? Couldn't we also fall back on a AC_CHECK_LIB?

Which Ubuntu is that?

comment:45 Changed 19 months ago by mkoeppe

But I think I would also be in favor of falling back to AC_CHECK_LIB, in fact. MacOS (without homebrew) has sqlite3 but no sqlite.pc.

comment:46 Changed 19 months ago by mkoeppe

Unfortunately AX_LIB_SQLITE3 from autoconf-archive is not good, it tries to parse header files to find out the version.

comment:47 follow-up: Changed 19 months ago by embray

What's wrong with that? In any case, I'd be okay for a fallback to not check the version. Having pkgconfig and a version check is definitely preferable as a first pass...

comment:48 in reply to: ↑ 44 ; follow-up: Changed 19 months ago by embray

Replying to mkoeppe:

Replying to embray:

I agree that the sqlite3 CLI should not be required. But this doesn't seem to work for me; my Ubuntu doesn't install a sqlite.pc? Couldn't we also fall back on a AC_CHECK_LIB?

Which Ubuntu is that?

I'm testing this on bionic (18.04).

comment:49 in reply to: ↑ 48 Changed 19 months ago by embray

Replying to embray:

Replying to mkoeppe:

Replying to embray:

I agree that the sqlite3 CLI should not be required. But this doesn't seem to work for me; my Ubuntu doesn't install a sqlite.pc? Couldn't we also fall back on a AC_CHECK_LIB?

Which Ubuntu is that?

I'm testing this on bionic (18.04).

My bad, it turns out I didn't have libsqlite3-dev installed (thought I did). After that I have the .pc file. Still would be nice to have a fallback though if possible.

comment:50 follow-up: Changed 19 months ago by dimpase

One can write an AC_RUN_IFELSE test that calls sqlite3_libversion_number().

comment:51 Changed 19 months ago by dimpase

although it's probably an overkill.

comment:52 in reply to: ↑ 47 ; follow-up: Changed 19 months ago by mkoeppe

Replying to embray:

What's wrong with that?

Everything. It cannot reliably succeed to parse the correct file that would be used by the compiler. And it did fail on macOS for exactly that reason.

comment:53 in reply to: ↑ 50 ; follow-up: Changed 19 months ago by mkoeppe

Replying to dimpase:

One can write an AC_RUN_IFELSE test that calls sqlite3_libversion_number().

That's the correct solution, I can work on this if you're not faster.

comment:54 in reply to: ↑ 52 Changed 19 months ago by embray

Replying to mkoeppe:

Replying to embray:

What's wrong with that?

Everything. It cannot reliably succeed to parse the correct file that would be used by the compiler. And it did fail on macOS for exactly that reason.

Ah, of course. My brain went completely past literally parsing the header file directly to parsing the output of a test program that prints the value of a macro sqlite3.h or something :)

comment:55 follow-up: Changed 19 months ago by mjo

Does the MacOS version (without homebrew) also install the library and development headers? Otherwise we'd be adding a fallback for nobody.

If we wind up digging through headers and libraries anyway, the pkg-config check becomes redundant.

comment:56 in reply to: ↑ 53 Changed 19 months ago by dimpase

Replying to mkoeppe:

Replying to dimpase:

One can write an AC_RUN_IFELSE test that calls sqlite3_libversion_number().

That's the correct solution, I can work on this if you're not faster.

please go ahead, I have to write a paper and presentations now :-)

We certainly already have examples of using AC_RUN_IFELSE, e.g. in build/pkgs/ntl/spkg-configure.m4

comment:57 in reply to: ↑ 55 Changed 19 months ago by mkoeppe

Replying to mjo:

Does the MacOS version (without homebrew) also install the library and development headers?

Yes. Part of XCode.

comment:58 Changed 19 months ago by mkoeppe

  • Branch changed from u/mjo/ticket/29002 to u/mkoeppe/ticket/29002

comment:59 Changed 19 months ago by mkoeppe

  • Authors changed from Michael Orlitzky to Michael Orlitzky, Matthias Koeppe
  • Commit changed from d32194842f5f7f83c2d50d020267838176a666f3 to 5f0e680bf2e675f67761107d97e0512f9675a1a0
  • Status changed from needs_work to needs_review

This now works on macOS with current XCode even if no homebrew is in PATH.


New commits:

5f0e680build/pkgs/sqlite/spkg-configure.m4: Fall back to checking with AC_RUN_IFELSE

comment:60 follow-up: Changed 19 months ago by mkoeppe

By the way, the check using pkgconfig is a bit questionable because Python, as far as I can see, does not actually use pkgconfig to find the library, and the results obtained by PKG_CHECK_MODULES, the variables SQLITE_CFLAGS and SQLITE_LIBS, are not passed to Python's build system.

comment:61 Changed 19 months ago by mkoeppe

But that is not a problem at least on macOS with homebrew. By default it provides the pc for the system-installed library, /usr/local/Homebrew/Library/Homebrew/os/mac/pkgconfig/10.15/sqlite3.pc, which is 3.8.10.2. The homebrew-provided library is installed "keg-only" and informs at installation that "For pkg-config to find sqlite you may need to set:export PKG_CONFIG_PATH="/usr/local/opt/sqlite/lib/pkgconfig". That provides 3.30.1.

comment:62 Changed 19 months ago by dimpase

why are you using sqlite3_libversion() rather than sqlite3_libversion_number() ? the latter returns int you are assembling by hand from the output of sqlite3_libversion()...

comment:63 Changed 19 months ago by mkoeppe

No, I'm converting the required version from its components.

comment:64 in reply to: ↑ 60 Changed 19 months ago by mjo

Replying to mkoeppe:

By the way, the check using pkgconfig is a bit questionable because Python, as far as I can see, does not actually use pkgconfig to find the library, ...

Everything we do in these spkg-configure files is a bit questionable =)

Basically we're trying to fake the checks that already (or should already) take place in the upstream packages, but as simply and with as little code as possible. A "perfect" implementation of spkg-configure for libfoo would copy/paste every upstream check for libfoo. But that's crazy, so we try to get away with something less than perfect, but still functional. If it works in 95% of cases and is harmless in the others, that's a win.

...which brings me to your custom version check. I believe that it works, but it's way too complicated for what we need to do. Keep in mind that we don't really care about the version of sqlite3. I only bothered to check it with pkg-config because it's trivial to do with pkg-config. As far as I can tell, you can cover the known cases with a combination of AC_CHECK_HEADER([sqlite3.h],...) and AC_SEARCH_LIBS([(sqlite3_libversion],[sqlite3],...). That's still not perfect, because it doesn't check for every function that python uses, but if it works for the people CC'ed on this ticket it will probably work for everyone.

There's no reason to keep the PKG_CHECK_MODULES around at that point. If the first test works sometimes and the second test works all the time, why try both?

comment:65 Changed 19 months ago by git

  • Commit changed from 5f0e680bf2e675f67761107d97e0512f9675a1a0 to 18f32a927b44f886e2ba599ba2a2db5947e05e2e

Branch pushed to git repo; I updated commit sha1. New commits:

18f32a9build/pkgs/sqlite/spkg-configure.m4: Remove the test with PKG_CHECK_MODULES

comment:66 Changed 19 months ago by mkoeppe

Thanks for the discussion. I have removed the PKG_CHECK_MODULES.

Regarding the version check, I am following the recommendations of upstream: https://www.sqlite.org/c3ref/libversion.html

comment:67 follow-up: Changed 19 months ago by mjo

My point about the version check is that your program is currently doing four things:

  1. Ensuring that sqlite3.h is in the include path.
  2. Ensuring that the sqlite3_libversion() function is in the sqlite3 library.
  3. Doing some math to see if the version of sqlite3 is newer than 3.8.7.
  4. Setting LIBS+=" -lsqlite3" if everything worked out, or setting sage_spkg_install_sqlite=yes if it didn't.

That's all good, but the third item is superfluous since we don't really care what version of sqlite3 you have (so long as it works). And if you remove the version check, then items 1, 2, and 4 can all be accomplished with AC_CHECK_HEADER and AC_SEARCH_LIBS in three or four lines.

comment:68 in reply to: ↑ 67 ; follow-up: Changed 19 months ago by mkoeppe

Replying to mjo:

... we don't really care what version of sqlite3 you have (so long as it works)

The version of sqlite3 for a python build was an issue as recently as 2017 here: https://bugs.python.org/issue29098

I don't have time to determine a more precise bound. But we do need to check *some* lower bound for the version (or test features of sqlite3).

comment:69 in reply to: ↑ 68 ; follow-up: Changed 19 months ago by mjo

Replying to mkoeppe:

Replying to mjo:

... we don't really care what version of sqlite3 you have (so long as it works)

The version of sqlite3 for a python build was an issue as recently as 2017 here: https://bugs.python.org/issue29098

They changed that code so that older versions of sqlite will work.

I don't have time to determine a more precise bound. But we do need to check *some* lower bound for the version (or test features of sqlite3).

Why? Python itself doesn't even bother.

comment:70 in reply to: ↑ 69 ; follow-up: Changed 19 months ago by mkoeppe

Replying to mjo:

Replying to mkoeppe:

Replying to mjo:

... we don't really care what version of sqlite3 you have (so long as it works)

The version of sqlite3 for a python build was an issue as recently as 2017 here: https://bugs.python.org/issue29098

They changed that code so that older versions of sqlite will work.

Which versions do?

comment:71 in reply to: ↑ 70 Changed 19 months ago by mjo

Replying to mkoeppe:

They changed that code so that older versions of sqlite will work.

Which versions do?

Python doesn't do a version check, and doesn't document any version bounds, so presumably all of them.

Put another way: if we find a version that makes it past our version-less check but that breaks python... then that's a python bug.

comment:72 follow-up: Changed 19 months ago by mkoeppe

But it is not the goal of sage-the-distribution to provoke python bugs.

We vendor a version of sqlite so that the vendored python can build.

comment:73 in reply to: ↑ 72 Changed 19 months ago by mjo

Replying to mkoeppe:

But it is not the goal of sage-the-distribution to provoke python bugs.

You make it sound so negative =)

If there's a version of sqlite that doesn't work with python, and if someone tries to build sage with it (both unlikely), then we report the bug upstream. At that point -- depending on the upstream fix -- we either do nothing or amend this spkg-configure with a lower bound.

Otherwise, we're just making things complicated for no ostensible reason. Since nobody is going to test every 3.x version of sqlite, we're still ultimately guessing at the lower version bound. My guess just happens to be 3.0.0.

If it's wrong, we can change it, but 3.0.0 is more likely to be correct than any other untested bound, and simplifies this check as a bonus.

comment:74 follow-up: Changed 19 months ago by dimpase

I think one reason for these checks is to make supporting sage(lib) a bearable job. We don't want to bog down in corner cases on museum-grade OSes, just cause they might carry a museum-grade version of sqlite3.

So I am for keeping a version check.

One other thing - for better of worse, spkg-configure.m4 in other packages undefine local m4 variables they happen to define, whereas here it does not happen. They use m4_pushdef/m4_popdef pair of macros for these purposes. Can we keep to the same style here?

Last edited 19 months ago by dimpase (previous) (diff)

comment:75 follow-up: Changed 19 months ago by mjo

Replying to dimpase:

I think one reason for these checks is to make supporting sage(lib) a bearable job. We don't want to bog down in corner cases on museum-grade OSes, just cause they might carry a museum-grade version of sqlite3.

So I am for keeping a version check.

That's a bit ironic considering that four people have wasted hours replacing

SAGE_SPKG_CONFIGURE([sqlite], [
  PKG_CHECK_MODULES([SQLITE],
                    [sqlite3 >= 3.8.7],
                    [],
                    [sage_spkg_install_sqlite=yes])
])

with

SAGE_SPKG_CONFIGURE([sqlite], [
  m4_define([sqlite3_min_version_major], [3])
  m4_define([sqlite3_min_version_minor], [8])
  m4_define([sqlite3_min_version_micro], [7])
  m4_define([sqlite3_min_version], [sqlite3_min_version_major.sqlite3_min_version_minor.sqlite3_min_version_micro])
  AC_MSG_CHECKING([libsqlite3 >= sqlite3_min_version])
                     dnl https://www.sqlite.org/c3ref/libversion.html
                     dnl https://www.sqlite.org/c3ref/c_source_id.html
                     SQLITE_SAVED_LIBS="$LIBS"
                     LIBS="$LIBS -lsqlite3"
                     AC_RUN_IFELSE([
                       AC_LANG_PROGRAM([[
                                         #include <sqlite3.h>
                                         #include <assert.h>
                                         #include <stdlib.h>
                                         #include <string.h>
                                       ]],
                                       [[
                                         assert( strcmp(sqlite3_libversion(),SQLITE_VERSION)==0 );
                                         if (SQLITE_VERSION_NUMBER < ]]sqlite3_min_version_major[[*1000000 + ]]sqlite3_min_version_minor[[*1000 + ]]sqlite3_min_version_micro[[)
                                           exit(1);
                                         else
                                           exit(0);
                                       ]])
                       ],
                       [AC_MSG_RESULT([yes])],
                       [AC_MSG_RESULT([no])
                        LIBS="$SQLITE_SAVED_LIBS"
                        sage_spkg_install_sqlite=yes])
])

to support one version of sqlite on one proprietary OS whose maintainers went out of their way to delete an important file from the package.

sagelib doesn't even use sqlite, it's a dependency of a dependency

comment:76 in reply to: ↑ 75 ; follow-up: Changed 19 months ago by mkoeppe

Replying to mjo:

considering that four people have wasted hours

Excuse me, m4 and autotools are my hobby.

comment:77 in reply to: ↑ 74 Changed 19 months ago by mkoeppe

Replying to dimpase:

spkg-configure.m4 in other packages undefine local m4 variables they happen to define, whereas here it does not happen. They use m4_pushdef/m4_popdef pair of macros for these purposes. Can we keep to the same style here?

Good idea, will do.

comment:78 Changed 19 months ago by git

  • Commit changed from 18f32a927b44f886e2ba599ba2a2db5947e05e2e to b261b315c3866a3fc178626431df1089e9059705

Branch pushed to git repo; I updated commit sha1. New commits:

b261b31build/pkgs/sqlite/spkg-configure.m4: Use m4_push/popdef and match style of other spkg-configure.m4

comment:79 Changed 19 months ago by mkoeppe

Ready for review.

comment:80 Changed 19 months ago by mkoeppe

  • Description modified (diff)

comment:81 Changed 19 months ago by mkoeppe

  • Description modified (diff)

comment:82 Changed 19 months ago by dimpase

  • Status changed from needs_review to positive_review

lgtm

comment:83 Changed 19 months ago by mkoeppe

Thank you!

comment:84 in reply to: ↑ 76 ; follow-up: Changed 19 months ago by embray

Replying to mkoeppe:

Replying to mjo:

considering that four people have wasted hours

Excuse me, m4 and autotools are my hobby.

HAHA :)

I'm also +1 for keeping the version check; personally I would lower the minimum version 3.0.0 until/unless a stronger minimum is actually determined. But 3.8.7 is still old-enough that it should be satisfied on most system so I'm okay with it.

comment:85 in reply to: ↑ 84 Changed 19 months ago by mkoeppe

Replying to embray:

I would lower the minimum version 3.0.0 until/unless a stronger minimum is actually determined. But 3.8.7 is still old-enough that it should be satisfied on most system so I'm okay with it.

3.0.0 was released in 2004. https://www.sqlite.org/chronology.html

comment:86 Changed 19 months ago by vbraun

  • Branch changed from u/mkoeppe/ticket/29002 to b261b315c3866a3fc178626431df1089e9059705
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:87 follow-up: Changed 18 months ago by embray

  • Commit b261b315c3866a3fc178626431df1089e9059705 deleted

It turns out there are some tests that fail if the sqlite3 executable is not installed:

sage -t --long --warn-long 78.2 src/sage/tests/cmdline.py
**********************************************************************
File "src/sage/tests/cmdline.py", line 608, in sage.tests.cmdline.test_executable
Failed example:
    out.startswith("3.")
Expected:
    True
Got:
    False
**********************************************************************
File "src/sage/tests/cmdline.py", line 610, in sage.tests.cmdline.test_executable
Failed example:
    err
Expected:
    ''
Got:
    '/home/sage/sage/src/bin/sage: line 546: exec: sqlite3: not found\n'
**********************************************************************
File "src/sage/tests/cmdline.py", line 612, in sage.tests.cmdline.test_executable
Failed example:
    ret
Expected:
    0
Got:
    127
**********************************************************************
1 item had failures:
   3 of 236 in sage.tests.cmdline.test_executable
    [235 tests, 3 failures, 45.53 s]
----------------------------------------------------------------------
sage -t --long --warn-long 78.2 src/sage/tests/cmdline.py  # 3 doctests failed
----------------------------------------------------------------------

So, technically it is a regression of sage -sqlite3 does not work. I don't think this is very important functionality and it would probably be better if it were slowly deprecated (I think the same should be the case for most sage -<program name>--rather, that most of those should be undocumented, and that there should be a generic interface for running executables that might be installed in the Sage distribution).

In the meantime, I'm leaning towards we need to check for it...

comment:88 in reply to: ↑ 87 ; follow-up: Changed 18 months ago by charpent

Replying to embray:

It turns out there are some tests that fail if the sqlite3 executable is not installed:

[ Snip... ]

So, technically it is a regression of sage -sqlite3 does not work.

Indeed. I just got bitten by this (trying to ebuild 9.1.beta2 from scratch rather than upgrading).

I don't think this is very important functionality and it would probably be better if it were slowly deprecated (I think the same should be the case for most sage -<program name>--rather, that most of those should be undocumented, and that there should be a generic interface for running executables that might be installed in the Sage distribution).

I tend to disagree : an unsuspecting user might have a hard time to understand why "our" version replaces the one he/she installed in his/her system if he/she runs out utility that installs system shortcuts for Sage-installed programs ...

In the meantime, I'm leaning towards we need to check for it...

Indeed. But since sqlite is so widely used, we might alsoi depend on the relevant packages (easy for Unices, possibly not so easy for Windows (Madison ?), and possible Apple shenanigans...).

BTW, our move to reduce the size of Sage-the-distribution should reopen the question of what are our minimal dependencies. I would have no problem to depend on sqlite, more on Maxima (Lisp interpreters !) and even more on R (size, dependencies on OpenSSL, etc...)

Should that be disciussed on sage-devel ?

In the meantime, should we open a new ticket ?

comment:89 in reply to: ↑ 88 Changed 18 months ago by dimpase

Replying to charpent:

Replying to embray:

It turns out there are some tests that fail if the sqlite3 executable is not installed:

[ Snip... ]

So, technically it is a regression of sage -sqlite3 does not work.

Indeed. I just got bitten by this (trying to ebuild 9.1.beta2 from scratch rather than upgrading).

I don't think this is very important functionality and it would probably be better if it were slowly deprecated (I think the same should be the case for most sage -<program name>--rather, that most of those should be undocumented, and that there should be a generic interface for running executables that might be installed in the Sage distribution).

I tend to disagree : an unsuspecting user might have a hard time to understand why "our" version replaces the one he/she installed in his/her system if he/she runs out utility that installs system shortcuts for Sage-installed programs ...

In the long run there should be no sqlite or libsqlite bundled into Sage.

In the meantime, I'm leaning towards we need to check for it...

Indeed. But since sqlite is so widely used, we might alsoi depend on the relevant packages (easy for Unices, possibly not so easy for Windows (Madison ?), and possible Apple shenanigans...).

BTW, our move to reduce the size of Sage-the-distribution should reopen the question of what are our minimal dependencies. I would have no problem to depend on sqlite, more on Maxima (Lisp interpreters !) and even more on R (size, dependencies on OpenSSL, etc...)

I won't worry too much about R, as it's already more or less sorted. Maxima/ECL is harder, as it's tigher integrated into Sage, and as ECL is moving very slowly nowadays (and with significant changes to come).

Should that be disciussed on sage-devel ?

In the meantime, should we open a new ticket ?

yes, please, for deprecating/removing sage -FOO functionality for FOO=sqlite and other values like this, right?

comment:90 Changed 18 months ago by mkoeppe

@embray Thanks for spotting this and for opening the follow-up ticket:

  • #29092 sqlite3 CLI executable should be checked for at configure time

I agree that this needs to be fixed.

comment:91 Changed 18 months ago by mkoeppe

I have opened:

  • #29111 Specify a subset of sage command line options that are supported by sagelib - rather than sage-the-distribution
Note: See TracTickets for help on using tickets.