Opened 2 years ago

Closed 22 months ago

#30559 closed enhancement (fixed)

Refine python3's SAGE_SPKG_DEPCHECK: Remove sqlite

Reported by: Matthias Köppe Owned by:
Priority: critical Milestone: sage-9.3
Component: build: configure Keywords:
Cc: Michael Orlitzky, Dima Pasechnik, slellievre, Markus Wageringel, François Bissey, Antonio Rojas, Erik Bray Merged in:
Authors: Matthias Koeppe Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: 4731840 (Commits, GitHub, GitLab) Commit: 4731840945a8598cbc97367e2b4ad60e6d9c6c7c
Dependencies: Stopgaps:

Status badges

Description (last modified by Matthias Köppe)

Currently we have SAGE_SPKG_DEPCHECK([sqlite libpng bzip2 xz libffi], ...), which causes system python3 to be rejected on many systems.

We should review whether this can be made more fine-grained.

SAGE_SPKG_DEPCHECK is specifically for checking whether we are going to install a shared library that may interfere with a system-provided version of the same shared library that the package is linked against.

Package sqlite....

$ grep sqlite build/pkgs/*/dependencies
build/pkgs/cryptominisat/dependencies:$(PYTHON) m4ri zlib libpng | cmake sqlite boost_cropped
build/pkgs/elliptic_curves/dependencies:| sqlite $(PYTHON)
build/pkgs/python3/dependencies:zlib readline sqlite libpng bzip2 xz libffi

elliptic_curves only seems to use the Python module sqlite - it does not depend on sqlite itself.

cryptominisat does not seem to depend on sqlite at all.

In this ticket, we remove these unnecessary dependencies and the sqlite depcheck for python3; and set sqlite as "not required" if system python3 will be used.


Follow-up tickets may address the following dependencies:

  • #30949 Refine python3's SAGE_SPKG_DEPCHECK: libpng, bzip2, xz, libffi
  • #30948 Package xz represents both the shared library liblzma and the xz binary.

See also:

  • #28019 If system mpir is found, yasm should not be built

Change History (37)

comment:1 Changed 2 years ago by Matthias Köppe

Cc: Markus Wageringel added
Component: PLEASE CHANGEbuild: configure

comment:2 Changed 2 years ago by Matthias Köppe

Description: modified (diff)

comment:3 Changed 2 years ago by Matthias Köppe

Description: modified (diff)

comment:4 Changed 2 years ago by Matthias Köppe

Cc: François Bissey Antonio Rojas added
Description: modified (diff)

comment:5 Changed 2 years ago by Matthias Köppe

Description: modified (diff)

comment:6 Changed 2 years ago by Matthias Köppe

Description: modified (diff)

comment:7 Changed 2 years ago by Matthias Köppe

Description: modified (diff)

comment:8 Changed 2 years ago by Matthias Köppe

Milestone: sage-9.2sage-9.3

comment:9 Changed 2 years ago by John Palmieri

I don't really understand m4 syntax, but for sqlite, it looks like build/pkgs/python3/spkg-configure.m4 checks both that sqlite as a package has been installed and separately whether the system Python includes an sqlite module. Shouldn't just the second of these be good enough?

Last edited 2 years ago by John Palmieri (previous) (diff)

comment:10 Changed 2 years ago by Matthias Köppe

The "depcheck" macro is used generally for avoiding a problem with conflicting shared libraries. Here, if we install sqlite from SPKG for any reason, then we insist that also python3 must be built from SPKG. Otherwise, the system python's sqlite module is linked to some other libsqlite3, and then IF we load it together with our libsqlite3, this could lead to trouble.

Now, as the ticket summary explains, ACTUALLY we never use our libsqlite3 for anything other than building python3.

So because of that, I think that we could, as you say, remove the depcheck for sqlite3 from spkg-configure.m4

However, if we ever add another package (providing a shared library linked into a Python module) that ALSO uses libsqlite3, then we would have to put the depcheck back...

comment:11 Changed 2 years ago by Matthias Köppe

Moreover, again because python3 is the unique package that uses sqlite3 directly, if system python3 is available, we do not have to build sqlite3 from SPKG even if we don't find system sqlite3.

comment:12 Changed 2 years ago by Matthias Köppe

This is all rather complicated, and I am not sure if our current spkg-configure framework is able to express it cleanly. But I think it would be very valuable to sort it out -- the missing sqlite (headers) are probably the top reason why we might reject python3 at least on macOS

comment:13 Changed 2 years ago by John Palmieri

We would have to pay attention if another package were added to use sqlite3, but as long as that doesn't happen, couldn't we just duplicate code from spkg-configure.m4 for python3 into the one for sqlite3? That is, put the checks for a valid system python3 into the spkg-configure.m4 script for sqlite3, and only if those fail, check to see if there is a valid sqlite3 package that Sage can use when building its python3.

comment:14 Changed 2 years ago by Matthias Köppe

Authors: Matthias Koeppe

comment:15 Changed 2 years ago by Matthias Köppe

Branch: u/mkoeppe/refine_python3_s_sage_spkg_depcheck

comment:16 Changed 2 years ago by Matthias Köppe

Commit: efb9dcc8f5b54d52d2c321beccbe57477121e2fe

Here's a version that might work


New commits:

dd3fc1ebuild/pkgs/python3/spkg-configure.m4: Remove DEPCHECK on sqlite
efb9dccbuild/pkgs/sqlite/spkg-configure.m4: Set as not required if system python3 will be used

comment:17 Changed 2 years ago by git

Commit: efb9dcc8f5b54d52d2c321beccbe57477121e2fea39a9f175db87cd646209a61528a2987d937a4fb

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

a39a9f1build/pkgs/cryptominisat/dependencies: Remove unused dep on sqlite

comment:18 Changed 2 years ago by git

Commit: a39a9f175db87cd646209a61528a2987d937a4fb362f6c96c5cec54d25a3ffc230b23212d651bcc8

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

362f6c9build/pkgs/elliptic_curves/dependencies: Remove sqlite; the package only uses it through the sqlite python module

comment:19 Changed 2 years ago by Matthias Köppe

Description: modified (diff)

comment:20 Changed 2 years ago by Matthias Köppe

Description: modified (diff)
Status: newneeds_review
Summary: Refine python3's SAGE_SPKG_DEPCHECKRefine python3's SAGE_SPKG_DEPCHECK: Remove sqlite

comment:21 Changed 2 years ago by Matthias Köppe

Description: modified (diff)

comment:22 Changed 2 years ago by John Palmieri

Reviewers: John Palmieri
Status: needs_reviewpositive_review

Unfortunately (?), sqlite is no longer broken on my mac running Big Sur, at least according to Sage's ./configure, so it's a little harder to test this, but if I run ./configure --with-system-sqlite=no, then without this branch Python gets built, while with it, the system Python 3 is used.

comment:23 Changed 2 years ago by Matthias Köppe

Thanks!

comment:24 Changed 2 years ago by Volker Braun

Status: positive_reviewneeds_work
[sagelib-9.2] [1/1] gcc -Wno-unused-result -Wsign-compare -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DNDEBUG -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -D_GNU_SOURCE -fPIC -fwrapv -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -D_GNU_SOURCE -fPIC -fwrapv -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -D_GNU_SOURCE -fPIC -fwrapv -fPIC -I/home/release/Sage/build/pkgs/sagelib/src -I/usr/include/python3.8 -I/home/release/Sage/local/lib64/python3.8/site-packages/numpy/core/include -Ibuild/cythonized -I/home/release/Sage/local/include -I/usr/include/python3.8 -c build/cythonized/sage/misc/sage_ostools.c -o build/temp.linux-x86_64-3.8/build/cythonized/sage/misc/sage_ostools.o -fno-strict-aliasing -DCYTHON_CLINE_IN_TRACEBACK=1 -std=c99
[sagelib-9.2] gcc -pthread -shared -Wl,-z,relro -Wl,--as-needed -Wl,-z,now -g -Wl,-z,relro -Wl,--as-needed -Wl,-z,now -g -Wl,-rpath-link,/home/release/Sage/local/lib -L/home/release/Sage/local/lib -Wl,-rpath,/home/release/Sage/local/lib build/temp.linux-x86_64-3.8/build/cythonized/sage/misc/sage_ostools.o -L/usr/lib64 -lsqlite3 -o build/lib.linux-x86_64-3.8/sage/misc/sage_ostools.cpython-38-x86_64-linux-gnu.so
[sagelib-9.2] /usr/bin/ld: cannot find -lsqlite3
[sagelib-9.2] collect2: error: ld returned 1 exit status
[sagelib-9.2] error: command 'gcc' failed with exit status 1

Note: there is no libsqlite3.so and I don't have sqlite3-devel installed

$ ll /usr/lib64/libsqlite3*
lrwxrwxrwx. 1 root root      19 Oct 11 10:30 /usr/lib64/libsqlite3.so.0 -> libsqlite3.so.0.8.6
-rwxr-xr-x. 1 root root 1280944 Oct 11 10:30 /usr/lib64/libsqlite3.so.0.8.6

comment:25 Changed 2 years ago by git

Commit: 362f6c96c5cec54d25a3ffc230b23212d651bcc808e336912b15b2e76c5441a40e31f278e24275ec

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

5ca8742Merge tag '9.3.beta0' into t/30559/refine_python3_s_sage_spkg_depcheck
08e3369sage.misc.sage_ostools: Link to sqlite only on Cygwin

comment:26 Changed 2 years ago by Matthias Köppe

Status: needs_workneeds_review

comment:27 Changed 23 months ago by Dima Pasechnik

Cc: Erik Bray added

this seems to need testing on Cygwin

comment:28 Changed 23 months ago by git

Commit: 08e336912b15b2e76c5441a40e31f278e24275ec4731840945a8598cbc97367e2b4ad60e6d9c6c7c

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

4731840Merge tag '9.3.beta1' into t/30559/refine_python3_s_sage_spkg_depcheck

comment:29 Changed 23 months ago by Matthias Köppe

Reviewers: John PalmieriJohn Palmieri, https://github.com/mkoeppe/sage/actions/runs/358167921

comment:30 Changed 23 months ago by John Palmieri

The Cygwin tests look okay to me. Shall we merge it?

comment:31 Changed 23 months ago by Matthias Köppe

I haven't yet seen a Cygwin test to proceed to actually running Sage because of the general fragility of the multi-stage GH actions (I restarted the workflow several times...). But I think we can proceed anyway, it should not hold up this improvement.

comment:32 Changed 23 months ago by John Palmieri

Reviewers: John Palmieri, https://github.com/mkoeppe/sage/actions/runs/358167921John Palmieri
Status: needs_reviewpositive_review

Okay, let's try again, then.

comment:33 Changed 23 months ago by Matthias Köppe

Thanks!

comment:34 Changed 23 months ago by Matthias Köppe

Description: modified (diff)

comment:35 Changed 23 months ago by Matthias Köppe

Description: modified (diff)

comment:36 Changed 23 months ago by Matthias Köppe

Description: modified (diff)

comment:37 Changed 22 months ago by Volker Braun

Branch: u/mkoeppe/refine_python3_s_sage_spkg_depcheck4731840945a8598cbc97367e2b4ad60e6d9c6c7c
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.