sagemath/sage

FPLLL 5.4.0 and FPyLLL 0.5.4

Closed this issue · 49 comments

malb commented

Tarball URLs - see checksums.ini

CC: @kiwifb

Component: packages: standard

Keywords: sd111

Author: Martin Albrecht

Branch: 29fcb34

Reviewer: Matthias Koeppe

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

malb commented

Commit: 8faa924

malb commented

Author: Martin Albrecht

malb commented

New commits:

8faa924FPLLL 5.4.0 and FPyLLL 0.5.3.dev0
malb commented

Description changed:

--- 
+++ 
@@ -1 +1,3 @@
+https://github.com/fplll/fplll/releases/download/5.4.0/fplll-5.4.0.tar.gz
 
+https://github.com/fplll/fpylll/releases/download/0.5.3.dev0/fpylll-0.5.3.dev0.tar.gz
comment:4

build/pkgs/fpylll/checksums.ini needs upstream_url - see other python packages for the preferred URL format

Changed commit from 8faa924 to 0ab1d0e

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

0ab1d0eadd upstream_url
malb commented
comment:6

Ta. Fixed.

Changed keywords from none to sd111

Description changed:

--- 
+++ 
@@ -1,3 +1 @@
-https://github.com/fplll/fplll/releases/download/5.4.0/fplll-5.4.0.tar.gz
-
-https://github.com/fplll/fpylll/releases/download/0.5.3.dev0/fpylll-0.5.3.dev0.tar.gz
+Tarball URLs - see `checksums.ini`
comment:11

On local-conda-forge-ubuntu-standard (https://github.com/mkoeppe/sage/runs/1533944840)

  build/src/fpylll/fplll/enumeration.cpp: In function 'PyObject* __pyx_pf_6fpylll_5fplll_11enumeration_11Enumeration_6get_nodes(__pyx_obj_6fpylll_5fplll_11enumeration_Enumeration*, PyObject*)':
  build/src/fpylll/fplll/enumeration.cpp:7430:99: error: no matching function for call to 'fplll::Enumeration<fplll::Z_NR<__mpz_struct [1]>, fplll::FP_NR<double> >::get_nodes(int&)'
       __pyx_t_3 = __Pyx_PyInt_From_unsigned_long(__pyx_v_self->_core.mpz_d->get_nodes(__pyx_v__level)); if (unlikely(!__pyx_t_3)) __PYX_ERR(0, 576, __pyx_L1_error)
                                                                                                     
comment:12

Works fine in Gentoo. I must I never know where to find the appropriate logs in your runs. Sometimes it is easy and sometimes I just get lost. I found the good stuff in the raw logs.

2020-12-11T03:40:06.7154190Z   building 'fpylll.fplll.enumeration' extension
2020-12-11T03:40:06.7164990Z   /home/runner/work/sage/sage/.tox/local-conda-forge-ubuntu-standard/conda/bin/x86_64-conda-linux-gnu-cc -Wno-unused-result -Wsign-compare -DNDEBUG -fwrapv -O2 -Wall -Wstrict-prototypes -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -pipe -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -pipe -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem /home/runner/work/sage/sage/.tox/local-conda-forge-ubuntu-standard/conda/include -DNDEBUG -D_FORTIFY_SOURCE=2 -O2 -isystem /home/runner/work/sage/sage/.tox/local-conda-forge-ubuntu-standard/conda/include -fPIC -Isrc/fpylll/fplll -I/home/runner/work/sage/sage/.tox/local-conda-forge-ubuntu-standard/local/lib/python3.8/site-packages/cysignals -I/home/runner/work/sage/sage/.tox/local-conda-forge-ubuntu-standard/local/include -I/home/runner/work/sage/sage/.tox/local-conda-forge-ubuntu-standard/local/lib/python3.8/site-packages/numpy/core/include -I/home/runner/work/sage/sage/.tox/local-conda-forge-ubuntu-standard/local/include -I/home/runner/work/sage/sage/.tox/local-conda-forge-ubuntu-standard/conda/include/python3.8 -c build/src/fpylll/fplll/enumeration.cpp -o build/temp.linux-x86_64-3.8/build/src/fpylll/fplll/enumeration.o -std=c++11 -fvisibility-inlines-hidden -std=c++17 -fmessage-length=0 -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem /home/runner/work/sage/sage/.tox/local-conda-forge-ubuntu-standard/conda/include
2020-12-11T03:40:06.7174509Z   cc1plus: warning: command line option '-Wstrict-prototypes' is valid for C/ObjC but not for C++
2020-12-11T03:40:06.7176260Z   build/src/fpylll/fplll/enumeration.cpp: In function 'PyObject* __pyx_pf_6fpylll_5fplll_11enumeration_11Enumeration_6get_nodes(__pyx_obj_6fpylll_5fplll_11enumeration_Enumeration*, PyObject*)':
2020-12-11T03:40:06.7178575Z   build/src/fpylll/fplll/enumeration.cpp:7430:99: error: no matching function for call to 'fplll::Enumeration<fplll::Z_NR<__mpz_struct [1]>, fplll::FP_NR<double> >::get_nodes(int&)'
2020-12-11T03:40:06.7180107Z        __pyx_t_3 = __Pyx_PyInt_From_unsigned_long(__pyx_v_self->_core.mpz_d->get_nodes(__pyx_v__level)); if (unlikely(!__pyx_t_3)) __PYX_ERR(0, 576, __pyx_L1_error)

I have a feeling -std=c++17 is responsible for this, I compile with c++11 in Gentoo (probably a default somewhere).

malb commented
comment:13

Is there perhaps a race condition that FPyLLL is built before the new FPLLL?

comment:14

Replying to @malb:

Is there perhaps a race condition that FPyLLL is built before the new FPLLL?

Looking at the log again, I'd say it looks like it!

comment:15

This build is accepting a system package

Checking whether SageMath should install SPKG fplll...
checking whether any of mpfr is installed as or will be installed as SPKG... no
checking for fplll >= 5.3... yes
configure: will use system package and not install SPKG fplll

If the new fpylll has other version requirements, build/pkgs/fplll/spkg-configure.m4 needs to be changed

comment:16

Same failure also on fedora-32-standard https://github.com/mkoeppe/sage/runs/1533944016 and other builds

comment:17

If practical at all, I would suggest to upstream to allow the Python bindings to accept a range of fplll versions...

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

9eb14e4update required FPLLL version

Changed commit from 0ab1d0e to 9eb14e4

malb commented
comment:19

Updated accordingly, I don't think we can commit to supporting a range of FPLLL versions in FPyLLL

comment:20

Perhaps then an upper bound for the version needs to be set as well?

comment:21

Likewise, should we set a range of acceptable fpylll versions (in the install-requires of sagelib)?

malb commented
comment:22

Replying to @mkoeppe:

Perhaps then an upper bound for the version needs to be set as well?

I think an upper bound is unclear, we could enforce a specific version, e.g. 5.4.0?

malb commented
comment:23

Replying to @mkoeppe:

Likewise, should we set a range of acceptable fpylll versions (in the install-requires of sagelib)?

For now, Sage only makes very high-level use of FPyLLL, i.e. it should work with many versions.

comment:24

Replying to @malb:

Replying to @mkoeppe:

Perhaps then an upper bound for the version needs to be set as well?

I think an upper bound is unclear, we could enforce a specific version, e.g. 5.4.0?

So there is no expectation that FPyLLL 0.5.3.dev0 can work with any FPLLL 5.4.x?

comment:25

Replying to @malb:

Replying to @mkoeppe:

Likewise, should we set a range of acceptable fpylll versions (in the install-requires of sagelib)?

For now, Sage only makes very high-level use of FPyLLL, i.e. it should work with many versions.

To clarify my question, the Sage distribution ships a specific version of FPLLL (5.4.0 with this ticket); but if fpylll comes from PyPI, what future versions should we expect to support this version of FPLLL?

If FPLLL/FPyLLL do not have a policy regarding version compatibility, it would probably pay off to set one...

malb commented
comment:26

Replying to @mkoeppe:

So there is no expectation that FPyLLL 0.5.3.dev0 can work with any FPLLL 5.4.x?

No expectation at all.

malb commented
comment:27

Replying to @mkoeppe:

To clarify my question, the Sage distribution ships a specific version of FPLLL (5.4.0 with this ticket); but if fpylll comes from PyPI, what future versions should we expect to support this version of FPLLL?

If FPLLL/FPyLLL do not have a policy regarding version compatibility, it would probably pay off to set one...

Realistically the only viable policy is: whatever versions were released at the same time should work together. So I think it makes sense to enforce a tuple: FPLLL=X,FPyLLL=Y

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

37414ebupdate to FPyLLL 0.5.4
26032b1enforce exact FPLLL

Changed commit from 9eb14e4 to 26032b1

comment:30

Based on your explanations, I think fpylll's install-requires.txt should also fix the version.

comment:33

No, that's not related to this ticket

comment:34

Replying to @mkoeppe:

Based on your explanations, I think fpylll's install-requires.txt should also fix the version.

As in:

diff --git a/build/pkgs/fpylll/install-requires.txt b/build/pkgs/fpylll/install-requires.txt
index 6e7d17221c..df75cd9fa7 100644
--- a/build/pkgs/fpylll/install-requires.txt
+++ b/build/pkgs/fpylll/install-requires.txt
@@ -1 +1 @@
-fpylll >=0.5.4
+fpylll ==0.5.4
comment:35

I would also suggest to add some comments:

diff --git a/build/pkgs/fplll/spkg-configure.m4 b/build/pkgs/fplll/spkg-configure.m4
index 6da0045fe0..b98a71938e 100644
--- a/build/pkgs/fplll/spkg-configure.m4
+++ b/build/pkgs/fplll/spkg-configure.m4
@@ -4,6 +4,9 @@ SAGE_SPKG_CONFIGURE([fplll], [
     dnl if there's a usable system copy of fplll. Unless there's
     dnl a system that ships fplll without fplll.pc file, falling
     dnl back to a manual header/library search is pointless.
+    dnl
+    dnl Trac #31025: FPLLL/FPyLLL make no guarantee regarding compatibility
+    dnl other than "whatever versions were released at the same time should work together"
     PKG_CHECK_MODULES([FPLLL],
                       [fplll == 5.4],
                       [sage_spkg_install_fplll=no],
diff --git a/build/pkgs/fpylll/install-requires.txt b/build/pkgs/fpylll/install-requires.txt
index 6e7d17221c..4b425c186e 100644
--- a/build/pkgs/fpylll/install-requires.txt
+++ b/build/pkgs/fpylll/install-requires.txt
@@ -1 +1,3 @@
-fpylll >=0.5.4
+# Trac #31025: FPLLL/FPyLLL make no guarantee regarding compatibility
+# other than "whatever versions were released at the same time should work together"
+fpylll ==0.5.4

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

29fcb34implement suggested changes by Matthias

Changed commit from 26032b1 to 29fcb34

Changed reviewer from https://github.com/mkoeppe/sage/actions/runs/426748847 to Matthias Koeppe

comment:39

Marking it as critical because some rolling distributions are already picking up 5.4.0

comment:41

For some reason, even after make distclean, configure is unable to find homebrew install of fplll 5.4.0 (on macOS). Then, the compilation fails on fpylll (may be a mix of fplll installations).

After modifying spkg-configure.m4 to change fplll == 5.4.0 to fplll >= 5.4.0 and adding file distros/homebrew.txt with fplll, configure finds the local install and everything goes well.

Should we open a ticket for that ?

Changed commit from 29fcb34 to none

comment:42

Replying to @dcoudert:

Should we open a ticket for that ?

Yes please. What version of fplll does pkg-config report?

comment:43

not sure how to get that, but inside config.log, I see:

## ------------------------------------------------------ ##
## Checking whether SageMath should install SPKG fplll... ##
## ------------------------------------------------------ ##
configure:19538: checking whether any of mpfr is installed as or will be installed as SPKG
configure:19547: result: no
configure:19552: checking for fplll >= 5.4
configure:19559: $PKG_CONFIG --exists --print-errors "fplll >= 5.4"
configure:19562: $? = 0
configure:19576: $PKG_CONFIG --exists --print-errors "fplll >= 5.4"
configure:19579: $? = 0
configure:19617: result: yes
configure:19629: will use system package and not install SPKG fplll

and I have fplll 5.4.0 installed by homebrew.

Follow-up on #31127

comment:44

Another follow up in #31146: cygwin-standard: fpylll build fails