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

upgrade fpLLL to version 4.0.4 #12835

Closed
malb opened this issue Apr 12, 2012 · 52 comments
Closed

upgrade fpLLL to version 4.0.4 #12835

malb opened this issue Apr 12, 2012 · 52 comments

Comments

@malb
Copy link
Member

malb commented Apr 12, 2012

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/:

CC: @jpflori

Component: packages: standard

Keywords: lll, fplll, spkg

Author: Paulo César Pereira de Andrade, Jean-Pierre Flori

Reviewer: Martin Albrecht, Jeroen Demeyer

Merged: sage-5.11.beta0

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

@malb malb added this to the sage-5.10 milestone Apr 12, 2012
@SnarkBoojum
Copy link
Mannequin

SnarkBoojum mannequin commented Jun 20, 2012

comment:1

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!)

@malb
Copy link
Member Author

malb commented Jun 20, 2012

comment:2

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.

@kiwifb
Copy link
Member

kiwifb commented Jun 20, 2012

comment:3

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

@SnarkBoojum
Copy link
Mannequin

SnarkBoojum mannequin commented Jun 20, 2012

comment:4

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!

@pcpa
Copy link
Mannequin

pcpa mannequin commented Oct 21, 2012

comment:5

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

@pcpa
Copy link
Mannequin

pcpa mannequin commented Oct 21, 2012

Attachment: libfplll-fplllv31.patch.gz

fplll patch

@pcpa
Copy link
Mannequin

pcpa mannequin commented Oct 21, 2012

Attachment: sage-fplll.patch.gz

sage patch

@kiwifb
Copy link
Member

kiwifb commented Oct 26, 2012

comment:6

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 test before we also up the spkg.

@SnarkBoojum
Copy link
Mannequin

SnarkBoojum mannequin commented Feb 20, 2013

comment:8

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

@jpflori
Copy link
Contributor

jpflori commented Feb 20, 2013

comment:9

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.

@SnarkBoojum
Copy link
Mannequin

SnarkBoojum mannequin commented Mar 22, 2013

comment:10

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

@SnarkBoojum SnarkBoojum mannequin added the s: needs info label Mar 22, 2013
@malb
Copy link
Member Author

malb commented Mar 22, 2013

comment:11

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

@jpflori
Copy link
Contributor

jpflori commented Mar 26, 2013

comment:12

Replying to @SnarkBoojum:

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 :)

@SnarkBoojum
Copy link
Mannequin

SnarkBoojum mannequin commented May 13, 2013

comment:13

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

@jpflori
Copy link
Contributor

jpflori commented May 14, 2013

comment:14

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.

@SnarkBoojum
Copy link
Mannequin

SnarkBoojum mannequin commented May 14, 2013

comment:15

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

@jpflori
Copy link
Contributor

jpflori commented May 28, 2013

Attachment: trac_12835-fplll4.patch.gz

@jpflori

This comment has been minimized.

@jpflori
Copy link
Contributor

jpflori commented May 28, 2013

Author: Paulo César Pereira de Andrade, Jean-Pierre Flori

@jpflori
Copy link
Contributor

jpflori commented May 28, 2013

comment:16

Attachment: trac_12835-doctests.patch.gz

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

@jpflori jpflori changed the title upgrade fpLLL to newest upstream release upgrade fpLLL to version 4.0.4 May 28, 2013
@malb
Copy link
Member Author

malb commented May 29, 2013

Reviewer: Martin Albrecht

@malb
Copy link
Member Author

malb commented May 29, 2013

comment:17
  • patches look good
    • doctests pass

@jdemeyer
Copy link
Contributor

comment:30

Replying to @SnarkBoojum:

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

@jpflori
Copy link
Contributor

jpflori commented May 30, 2013

comment:31

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.

@jpflori
Copy link
Contributor

jpflori commented May 30, 2013

comment:32

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

@jdemeyer
Copy link
Contributor

Work Issues: hgignore, make final spkg

@jdemeyer
Copy link
Contributor

Changed work issues from hgignore, make final spkg to hgignore, license, make final spkg

@jpflori
Copy link
Contributor

jpflori commented May 30, 2013

Attachment: trac_12835-hgignore.patch.gz

@jpflori
Copy link
Contributor

jpflori commented May 30, 2013

Changed keywords from lll, fplll to lll, fplll, spkg

@jpflori
Copy link
Contributor

jpflori commented May 30, 2013

comment:35

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

@jpflori

This comment has been minimized.

@jpflori
Copy link
Contributor

jpflori commented May 30, 2013

Changed work issues from hgignore, license, make final spkg to none

@jdemeyer
Copy link
Contributor

comment:36

The spkg diff isn't right....

@jpflori
Copy link
Contributor

jpflori commented May 30, 2013

Spkg diff, for review only.

@jpflori
Copy link
Contributor

jpflori commented May 30, 2013

comment:37

Attachment: libfplll-4.0.4.diff.gz

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.

@jpflori
Copy link
Contributor

jpflori commented May 30, 2013

comment:38

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.

@jdemeyer
Copy link
Contributor

comment:39

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.

@jdemeyer
Copy link
Contributor

Changed reviewer from Martin Albrecht to Martin Albrecht, Jeroen Demeyer

@jdemeyer
Copy link
Contributor

comment:40

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

@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title upgrade fpLLL to version 4.0.4 when it's released upgrade fpLLL to version 4.0.4 May 30, 2013
@jdemeyer jdemeyer added this to the sage-5.11 milestone Jun 3, 2013
@jdemeyer jdemeyer removed the pending label Jun 3, 2013
@jdemeyer
Copy link
Contributor

Merged: sage-5.11.beta0

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

4 participants