Opened 9 years ago

Closed 8 years ago

#10328 closed enhancement (fixed)

Flint-1.5.2 with ARM 32-bits

Reported by: Snark Owned by: GeorgSWeber
Priority: major Milestone: sage-5.0
Component: build Keywords:
Cc: leif Merged in: sage-5.0.beta14
Authors: Julien Puydt Reviewers: Dmitrii Pasechnik
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by dimpase)

Here is a patch for flint-1.5.2, which only modifies what gets built on ARM 32-bits.

I can confirm that with that change, flint-1.5.2 builds -- and all tests end up ok.

The issue has been discussed upstream ; they're ok with a sage-only patch, since they're focusing on flint2.

I built my own spkg by :

  • taking the current flint spkg ;
  • putting the 1.5.2 version of flint in src/ instead of the current 1.5.0 ;
  • copying src/longlong.h to patches/
  • copying the attached patch to patches/
  • applying it to the patches/longlong.h

In spkg-install, I also added two lines :

# Patched longlong.h doesn't produce invalid assembly on 32-bit ARM

cp patches/longlong.h src/

Then I packed it into flint-1.5.2.??.spkg, and put it in ~/sage/spkg/standard/ ; removing the current flint spkg.

The following is updated as requested:

spkg: http://boxen.math.washington.edu/home/dima/packages/flint-1.5.2.p0.spkg

Attachments (1)

longlong.diff (2.2 KB) - added by Snark 9 years ago.
Patch to remove buggy sub_ddmmss implementation

Download all attachments as: .zip

Change History (30)

Changed 9 years ago by Snark

Patch to remove buggy sub_ddmmss implementation

comment:1 follow-up: Changed 9 years ago by leif

  • Authors changed from Snark to Snark - PLEASE CHANGE
  • Cc leif added

I'm intending to upgrade Sage's FLINT to 1.5.2 at #9858, because of #8664, to be hopefully merged into (an early alpha of) Sage 4.6.2.

Note that the spkg needs clean-up I'll do there as well. (I already did some stuff, until black-out... :/ )

Btw, FLINT 1.6 is on the way, too.


The Author(s) field should contain realnames; please also add yourself to http://trac.sagemath.org/sage_trac/wiki#AccountNamesmappedtoRealNames .

In general you should provide a proper Mercurial patch to the spkg (along with a link to the updated spkg), but I can include your changes in my spkg.

comment:2 in reply to: ↑ 1 Changed 9 years ago by leif

Replying to leif:

I'm intending to upgrade Sage's FLINT to 1.5.2 at #9858, because of #8664, to be hopefully merged into (an early alpha of) Sage 4.6.2.

Note that the spkg needs clean-up I'll do there as well. (I already did some stuff, until black-out... :/ )

Btw, FLINT 1.6 is on the way, too.

FLINT 1.6 was released on/for Chrismas (Dec. 24th).

Unless somebody else already did/does I'll open a ticket to upgrade Sage's version to that one.

Since Sage currently does weird things with the Makefile, providing a new, "clean" spkg will take some time (besides testing collaboration with the new FLINT of course).

(Though FLINT is a plain C library, with a separate module to interface with NTL, which is C++ and hence the interface as well, Sage builds one monolithic "FLINT" library with that interface module included such that it always depends on the NTL library and the C++ standard library, which is totally odd and frequently causes trouble. Changing that requires changes to the Sage library, including module_list.py and perhaps setup.py, too.)

comment:3 Changed 8 years ago by Snark

  • Status changed from new to needs_info

sage 4.7.1 is still using flint 1.5.0.p5 which doesn't build on ARM, so what I described above is still current. How are things going with respect to an upgraded flint?

comment:4 Changed 8 years ago by kcrisman

Copying a comment I made on #9858 here:

Here is another issue - if one of the patches fails in the current Flint spkg, then the Cygwin-only move of $SAGE_LOCAL/lib/libntl.a to a temporary file is NOT rescinded. So this should already happen if the patches do not apply. Just a point for any upgrade to FLINT - this should be fixed.

(I noticed this because one of the patches didn't apply for me. Which makes no sense, because it was exactly right. But whatever.)

comment:5 Changed 8 years ago by kcrisman

See #11727 for that problem.

comment:6 Changed 8 years ago by Snark

  • Authors changed from Snark - PLEASE CHANGE to Snark

I gave sage-4.7.2.alpha4 a try, and it still has flint-1.5.0.p9, which doesn't build.

I replaced sage-4.7.2.alpha4's flint-1.5.0.p9.spkg's src/ directory with flint-1.5.2's sources, put my longlong.diff patch in patches/ as longlong.patch, and it compiled successfully.

comment:7 follow-up: Changed 8 years ago by was

I had to do some rebasing of patches, etc. to get a flint-1.5.2 to actually work. Here's my spkg, since it may be useful:

http://wstein.org/patches/flint-1.5.2.spkg

The repo isn't checked in. This did work for me on Android.

william

comment:8 in reply to: ↑ 7 Changed 8 years ago by dimpase

Replying to was:

I had to do some rebasing of patches, etc. to get a flint-1.5.2 to actually work. Here's my spkg, since it may be useful:

http://wstein.org/patches/flint-1.5.2.spkgthat

The repo isn't checked in. This did work for me on Android.

I can confirm that it also builds on AC100, ubuntu 11.10. I will be willing to give it positive review once I am done building and testing Sage.

william

comment:9 follow-up: Changed 8 years ago by was

I don't think we should give this a positive review as is. More precisely, it deletes a chunk of code that isn't buggy (as far as we know) on other platforms, and which could lead to a big performance regression (for all I know). That code should only get deleted when building Sage on ARM.

comment:10 in reply to: ↑ 9 Changed 8 years ago by dimpase

Replying to was:

I don't think we should give this a positive review as is. More precisely, it deletes a chunk of code that isn't buggy (as far as we know) on other platforms, and which could lead to a big performance regression (for all I know). That code should only get deleted when building Sage on ARM.

OK, I actually did not look into the spkg. Right, thus some rebasing needs to be done...

comment:11 Changed 8 years ago by Snark

The code I remove is under "#if defined (arm) && W_TYPE_SIZE == 32", so "it deletes a chunk of code that isn't buggy (as far as we know) on other platforms" is wrong : that code is platform-specific and is known not to compile on that very platform!

The removal is unconditional because the code is conditional anyway : if it's not there, then we fall back on C code which compiles and runs. Not optimal, but there is no working optimised code for the platform anyway, so there's no regression.

I hope that clears matters.

comment:12 follow-up: Changed 8 years ago by dimpase

I've cleaned up the spkg, updated SPKG.txt, checked in stuff, and put the result here. I'll test on a couple of platforms, just to make sure everything is OK.

comment:13 in reply to: ↑ 12 Changed 8 years ago by dimpase

  • Status changed from needs_info to needs_review

Replying to dimpase:

I've cleaned up the spkg, updated SPKG.txt, checked in stuff, and put the result here. I'll test on a couple of platforms, just to make sure everything is OK.

all is well, it appears (tested on Linux, MacOSX).

comment:14 Changed 8 years ago by Snark

When will that spkg find its way into sage?

comment:15 Changed 8 years ago by dimpase

  • Status changed from needs_review to positive_review

comment:16 Changed 8 years ago by jdemeyer

  • Authors changed from Snark to Julien Puydt
  • Reviewers set to Dmitrii Pasechnik
  • Status changed from positive_review to needs_work

Dmitrii: I guess somebody else should review your changes.

comment:17 Changed 8 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:18 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:19 Changed 8 years ago by dimpase

I can confirm that this still builds and passes ptestlong on Sage 5.0.beta9. As I cannot review my own changes, Julien, could you please review this?

comment:20 Changed 8 years ago by Snark

  • Status changed from needs_review to positive_review

I have been using (compiling&ptestlonging) this package both on ARM and x86_64 since it has been available and last time this very morning. Never had anything to say about it.

So positive review.

comment:21 follow-up: Changed 8 years ago by leif

Well, the spkg should at least have a .p0 extension, as it modifies upstream code. (In the ticket's description, Julien says he made a .p1.)

Provided I do have the correct / most recent spkg (this, md5sum 4ef63f3b19e31e3a8f0161a24cfa661a), I don't see how the "spkg got cleaned up", but I don't mind the latter either, as I've been working on a (cleaned-up!) FLINT 1.5.2 spkg a couple of times and had to rebase my changes at least five times. Hopefully I'll finally get my changes in on some other ticket.

comment:22 Changed 8 years ago by dimpase

  • Description modified (diff)

comment:23 in reply to: ↑ 21 ; follow-up: Changed 8 years ago by dimpase

Replying to leif:

Well, the spkg should at least have a .p0 extension, as it modifies upstream code. (In the ticket's description, Julien says he made a .p1.)

Done.

Provided I do have the correct / most recent spkg (this, md5sum 4ef63f3b19e31e3a8f0161a24cfa661a), I don't see how the "spkg got cleaned up",

well, I just committed the changes into the repo, updated SPKG.txt, etc.

comment:24 in reply to: ↑ 23 Changed 8 years ago by leif

Replying to dimpase:

Replying to leif:

Well, the spkg should at least have a .p0 extension, as it modifies upstream code. (In the ticket's description, Julien says he made a .p1.)

Done.

Ok, thanks. (New spkg is also "clean" as far as I can see.)

comment:25 Changed 8 years ago by jdemeyer

  • Milestone set to sage-5.0

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

I see beta12 still has the old flint.

What more is there to do here?

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

Replying to Snark:

What more is there to do here?

Patience you must have.

comment:28 Changed 8 years ago by Snark

Sigh. I just noticed 1.6 has been released since I opened that ticket. Let's get 1.5.2 in sage first...

comment:29 Changed 8 years ago by jdemeyer

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