Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refine python3's SAGE_SPKG_DEPCHECK: Remove sqlite #30559

Closed
mkoeppe opened this issue Sep 11, 2020 · 43 comments
Closed

Refine python3's SAGE_SPKG_DEPCHECK: Remove sqlite #30559

mkoeppe opened this issue Sep 11, 2020 · 43 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Sep 11, 2020

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:

See also:

CC: @orlitzky @dimpase @slel @mwageringel @kiwifb @antonio-rojas @embray

Component: build: configure

Author: Matthias Koeppe

Branch: 4731840

Reviewer: John Palmieri

Issue created by migration from https://trac.sagemath.org/ticket/30559

@mkoeppe mkoeppe added this to the sage-9.2 milestone Sep 11, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Oct 8, 2020
@jhpalmieri
Copy link
Member

comment:9

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?

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 10, 2020

comment:10

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...

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 10, 2020

comment:11

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 10, 2020

comment:12

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

@jhpalmieri
Copy link
Member

comment:13

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.

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 10, 2020

Author: Matthias Koeppe

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 10, 2020

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 10, 2020

comment:16

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 10, 2020

Commit: efb9dcc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 10, 2020

Changed commit from efb9dcc to a39a9f1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 10, 2020

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 10, 2020

Changed commit from a39a9f1 to 362f6c9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 10, 2020

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

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Refine python3's SAGE_SPKG_DEPCHECK Refine python3's SAGE_SPKG_DEPCHECK: Remove sqlite Oct 11, 2020
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Oct 17, 2020

comment:23

Thanks!

@vbraun
Copy link
Member

vbraun commented Oct 25, 2020

comment:24
[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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 2, 2020

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 2, 2020

Changed commit from 362f6c9 to 08e3369

@dimpase
Copy link
Member

dimpase commented Nov 11, 2020

comment:27

this seems to need testing on Cygwin

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 11, 2020

Changed commit from 08e3369 to 4731840

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 11, 2020

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 11, 2020

comment:29

Cygwin test now running at https://github.com/mkoeppe/sage/actions/runs/358167921

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 11, 2020

Changed reviewer from John Palmieri to John Palmieri, https://github.com/mkoeppe/sage/actions/runs/358167921

@jhpalmieri
Copy link
Member

comment:30

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

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 22, 2020

comment:31

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.

@jhpalmieri
Copy link
Member

comment:32

Okay, let's try again, then.

@jhpalmieri
Copy link
Member

Changed reviewer from John Palmieri, https://github.com/mkoeppe/sage/actions/runs/358167921 to John Palmieri

@mkoeppe
Copy link
Member Author

mkoeppe commented Nov 22, 2020

comment:33

Thanks!

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@vbraun
Copy link
Member

vbraun commented Nov 29, 2020

Changed branch from u/mkoeppe/refine_python3_s_sage_spkg_depcheck to 4731840

@fchapoton
Copy link
Contributor

Changed commit from 4731840 to none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants