Opened 9 years ago

Closed 8 years ago

#12835 closed enhancement (fixed)

upgrade fpLLL to version 4.0.4

Reported by: malb Owned by: tbd
Priority: major Milestone: sage-5.11
Component: packages: standard Keywords: lll, fplll, spkg
Cc: jpflori Merged in: sage-5.11.beta0
Authors: Paulo César Pereira de Andrade, Jean-Pierre Flori Reviewers: Martin Albrecht, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

Sage still ships fpLLL 3.0.12 (we upgraded to 3.0.12 in May 2009!) but fpLLL 4.0.4 is out, which includes an implementation of BKZ.

Use spkg at:

Apply to Sage library:

Apply to local/bin/:

Attachments (6)

libfplll-fplllv31.patch (1.3 KB) - added by pcpa 9 years ago.
fplll patch
sage-fplll.patch (8.9 KB) - added by pcpa 9 years ago.
sage patch
trac_12835-fplll4.patch (8.6 KB) - added by jpflori 8 years ago.
trac_12835-doctests.patch (3.0 KB) - added by jpflori 8 years ago.
trac_12835-hgignore.patch (406 bytes) - added by jpflori 8 years ago.
libfplll-4.0.4.diff (2.2 KB) - added by jpflori 8 years ago.
Spkg diff, for review only.

Download all attachments as: .zip

Change History (50)

comment:1 Changed 9 years ago by Snark

I just checked it won't be painless : sage/libs/fplll/fplll.cpp:243:24: fatal error: fplll/fast.h: No such file or directory

From what I see in upstream's sources:

  • wrapper.h still exists, but isn't installed ;
  • fast.h, fast_earlyred.h, heuristic.h, heuristic_early_red.h and proved.h don't exist anymore, but there's a fplllv31.h which provides a compatibility layer to version 3.1 (which is more recent than what sage has... so perhaps even that isn't enough!)

comment:2 Changed 9 years ago by malb

AFAIK fpLLL 4.0 is essentially a new library from an interface point of view, i.e., they switched to a more C++-style interface. I suggest to essentially start over with the interface instead of trying to patch the old interface.

comment:3 Changed 9 years ago by fbissey

I'd agree with that. A long time ago when fplll-3.1 was released we looked at supporting it in sage-on-gentoo https://github.com/cschwan/sage-on-gentoo/issues/71. Funny thing compiling against 3.0.12 and then upgrading to 3.1.1 without rebuilding sage worked fine.

comment:4 Changed 9 years ago by Snark

There are two points to be made here :

  • I had a better look, and saw wrapper.h's previous features seem to be in fplllv31.h ; but I can't find the "proved" class anywhere. In fact, choosing which method will be used is now done through an enum instead of different classes as far as I see. So indeed a pure rewrite would be the saner route.
  • Apparently, changing the sage-5.1.beta5.spkg and using "sage -f" on it isn't enough : generally sage extracts the spkg in spkg/build/ but really builds sources it copied some previous time in devel/sage/ -- I haven't gotten the logic of it down.

All in all, that makes working on this pretty painful : I'll pass!

Last edited 9 years ago by Snark (previous) (diff)

comment:5 Changed 9 years ago by pcpa

I made sagemath 5.4.beta1 experimental rpm packages for fedora rawhide, patching both fplll and sagemath.

The only issue I noticed so far was:

$ rpm -q sagemath libfplll
sagemath-5.4.beta1-1.fc19.x86_64
libfplll-4.0.1-2.fc19.x86_64
$ sage -t /usr/share/sagemath/devel/sage/sage/libs/fplll/fplll.pyx
sage -t  "devel/sage/sage/libs/fplll/fplll.pyx"             
**********************************************************************
File "/usr/share/sagemath/devel/sage/sage/libs/fplll/fplll.pyx", line 646:
    sage: L = A.LLL(); L
Expected:
    [ 192  264 -152  272   -8  272  -48 -264  104   -8]
    [-128 -176 -240  160 -336  160   32  176  272 -336]
    [ -24 -161  147  350  385  -34  262  161  115  257]
    [ 520   75 -113  -74 -491   54  126  -75  239 -107]
    [-376 -133  255   22  229  150  350  133   95 -411]
    [-168 -103    5  402 -377 -238 -214  103 -219 -249]
    [-352   28  108 -328 -156  184   88  -28  -20  356]
    [ 120 -219  289  298  123  170 -286  219  449 -261]
    [ 160 -292   44   56  164  568  -40  292  -84 -348]
    [ 192  264 -152  272   -8  272  -48  760  104   -8]
Got:
    [ 192  264 -152  272   -8  272  -48 -264  104   -8]
    [-128 -176 -240  160 -336  160   32  176  272 -336]
    [ -24 -161  147  350  385  -34  262  161  115  257]
    [ 520   75 -113  -74 -491   54  126  -75  239 -107]
    [-376 -133  255   22  229  150  350  133   95 -411]
    [-168 -103    5  402 -377 -238 -214  103 -219 -249]
    [-352   28  108 -328 -156  184   88  -28  -20  356]
    [ 120 -219  289  298  123  170 -286  219  449 -261]
    [ 160 -292   44   56  164  568  -40  292  -84 -348]
    [-192  760  152 -272    8 -272   48  264 -104    8]
**********************************************************************
1 items had failures:
   1 of   8 in __main__.example_16
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/pcpa/.sage/tmp/fplll_17597.py
         [17.5 s]
 
----------------------------------------------------------------------
The following tests failed:


        sage -t  "devel/sage/sage/libs/fplll/fplll.pyx"
Total time for all tests: 17.6 seconds

Changed 9 years ago by pcpa

fplll patch

Changed 9 years ago by pcpa

sage patch

comment:6 Changed 9 years ago by fbissey

I must say I hate when I see that kind of sign flipping. Still we can have something based on something less ancient even if it is in compatibility mode. Now who could check that est before we also up the spkg.

Version 0, edited 9 years ago by fbissey (next)

comment:7 Changed 8 years ago by jpflori

  • Cc jpflori added

comment:8 Changed 8 years ago by Snark

Is it possible to avoid patching fplll? It it's not possible, did you discuss with upstream for inclusion?

comment:9 Changed 8 years ago by jpflori

I think upstream is more or less actively preparing a new release including fixes for Cygwin under my impulsion, so they should be quite reactive if you ask for the inclusion of such a patch as well.

comment:10 follow-up: Changed 8 years ago by Snark

  • Status changed from new to needs_info

I would like to see this go through : should I contact upstream myself?

comment:11 Changed 8 years ago by malb

I second that, I haven an interest in seeing this go in because I want to write libs.fplll4 which uses the new interface.

comment:12 in reply to: ↑ 10 Changed 8 years ago by jpflori

Replying to Snark:

I would like to see this go through : should I contact upstream myself?

Yup you should. I was in contact with Damien Stelhé around Christmas for #13354 and he included the fix I wanted in a to be released version. But when testing it I discovered other problems and got no reply so far. I must admit I did not ping him since the 30th of January...

So if you could also ask him for the last changes which were needed for Cygwin :)

comment:13 Changed 8 years ago by Snark

4.0.3 is out with cygwin changes ; it's already been seven weeks and I haven't written the mail I mentioned...

comment:14 Changed 8 years ago by jpflori

I just mailed Damien to thank him for the Cygwin changes and ask about our patch (from what I saw in the patch it seems not only legitimate, but needed to actually use the compatibility interface (except for the additional function we define of course)), so if you did not, please don't do it now.

comment:15 Changed 8 years ago by Snark

I had a mail discussion yesterday evening ; he is at a conference this week but will look into the matter later. :-P

Changed 8 years ago by jpflori

Changed 8 years ago by jpflori

comment:16 Changed 8 years ago by jpflori

  • Authors set to Paulo César Pereira de Andrade, Jean-Pierre Flori
  • Description modified (diff)
  • Status changed from needs_info to needs_review
  • Summary changed from upgrade fpLLL to newest upstream release to upgrade fpLLL to version 4.0.4

Some doctests changed but they are still LLL-reduced, so nothing is wrong.

comment:17 Changed 8 years ago by malb

  • Reviewers set to Martin Albrecht
  • Status changed from needs_review to positive_review
  • patches look good
  • doctests pass

comment:18 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.10 to sage-5.11

comment:19 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Various files have permission

-rwxr--r--

which is strange; most likely this should be

-rwxr-xr-x

comment:20 follow-ups: Changed 8 years ago by jpflori

It's so in the upstream tarball (not released yet, but its also the same in the 4.0.3 released one and the 3.1.3 one, though is correct in the really old 3.0.12). If you are really worried about this, I (or you) can report it upstream.

comment:21 in reply to: ↑ 20 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-5.11 to sage-pending
  • Summary changed from upgrade fpLLL to version 4.0.4 to upgrade fpLLL to version 4.0.4 when it's released

comment:22 in reply to: ↑ 20 Changed 8 years ago by jdemeyer

Replying to jpflori:

If you are really worried about this, I (or you) can report it upstream.

Please do report this upstream, maybe they can still ship a corrected version 4.0.4.

comment:23 Changed 8 years ago by jdemeyer

Another small mistake in SPKG.txt: the license is

LGPL v2.1+

(version 2.1 is not the same as version 2)

comment:24 Changed 8 years ago by jpflori

Reported the permissions thing, I'll correct the LGPL stuff when I get feedback from Damien.

comment:25 follow-up: Changed 8 years ago by Snark

Ah, I was about to also positively review : I finally finished compiling and doctesting without problem (except that my boxes are all slow :-( )

Yes, I think upstream didn't publicly release 4.0.4 yet : Jean-Pierre, myself then everyone through this ticket got to see a release-candidate.

comment:26 in reply to: ↑ 25 Changed 8 years ago by jdemeyer

Replying to Snark:

Yes, I think upstream didn't publicly release 4.0.4 yet : Jean-Pierre, myself then everyone through this ticket got to see a release-candidate.

It is usually better to wait for a public release before making the spkg for Sage. Or at least, if it's a release candidate, make it clear that it is (libfplll-4.0.4.rc.spkg)

comment:27 follow-ups: Changed 8 years ago by Snark

@jdemeyer: in fact, upstream is waiting for us(this ticket) to say "let go" to release, so Jean-Pierre wasn't that wrong to provide a spkg already :-)

comment:28 in reply to: ↑ 27 Changed 8 years ago by jdemeyer

Replying to Snark:

@jdemeyer: in fact, upstream is waiting for us(this ticket) to say "let go" to release

Then have upstream release a public and versioned release candidate for version 4.0.4.

comment:29 Changed 8 years ago by jdemeyer

latticegen should be added to local/bin/.hgignore.

comment:30 in reply to: ↑ 27 Changed 8 years ago by jdemeyer

Replying to Snark:

@jdemeyer: in fact, upstream is waiting for us(this ticket) to say "let go" to release, so Jean-Pierre wasn't that wrong to provide a spkg already :-)

It was very right to provide a spkg. I'm just saying that it should have been made clear that this was for a non-released version of fplll and hence it wasn't the final spkg.

comment:31 Changed 8 years ago by jpflori

4.0.4 is officially released without the new permissions, I quickly mailed Damien to see if he could not fix that and replace the upped tarball with a fixed one without anyone noticing.

comment:32 Changed 8 years ago by jpflori

In fact Damien responded and it should have fixed permissions, he will reupload a proper tarball later today.

comment:33 Changed 8 years ago by jdemeyer

  • Work issues set to hgignore, make final spkg

comment:34 Changed 8 years ago by jdemeyer

  • Work issues changed from hgignore, make final spkg to hgignore, license, make final spkg

Changed 8 years ago by jpflori

comment:35 Changed 8 years ago by jpflori

  • Description modified (diff)
  • Keywords spkg added
  • Status changed from needs_work to needs_review
  • Work issues hgignore, license, make final spkg deleted

A proper 4.0.4 is now online on upstream website and packaged here.

comment:36 Changed 8 years ago by jdemeyer

The spkg diff isn't right....

Changed 8 years ago by jpflori

Spkg diff, for review only.

comment:37 Changed 8 years ago by jpflori

Fixed hopefully.

A few remarks:

  • I've update the website address.
  • According to the info there only Damien is in charge now.
  • I've added his professional mail address.
  • updated the license.
  • Fixed the date and author of the previous spkg we distributed which were wrong.

comment:38 follow-up: Changed 8 years ago by jpflori

And there is an accent in Damien family's name which appears correctly when I look at SPKG.txt and the diff file with a capable reader but not in the diff when displayed on trac.

Last edited 8 years ago by jpflori (previous) (diff)

comment:39 in reply to: ↑ 38 Changed 8 years ago by jdemeyer

Replying to jpflori:

And there is an accent in Damien family's name which appears correctly when I look at SPKG.txt and the diff file with a capable reader but not in the diff when displayed on trac.

That's not a problem.

comment:40 Changed 8 years ago by jdemeyer

  • Reviewers changed from Martin Albrecht to Martin Albrecht, Jeroen Demeyer

Looks good on first sight, will set to positive review if build and doctests work.

comment:41 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from upgrade fpLLL to version 4.0.4 when it's released to upgrade fpLLL to version 4.0.4

comment:42 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:43 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-pending to sage-5.11

comment:44 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.11.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.