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 )
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)
Change History (30)
Changed 9 years ago by
comment:1 follow-up: ↓ 2 Changed 9 years ago by
- 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
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
- 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
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
See #11727 for that problem.
comment:6 Changed 8 years ago by
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: ↓ 8 Changed 8 years ago by
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
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: ↓ 10 Changed 8 years ago by
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
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
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: ↓ 13 Changed 8 years ago by
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
- Status changed from needs_info to needs_review
comment:14 Changed 8 years ago by
When will that spkg find its way into sage?
comment:15 Changed 8 years ago by
- Status changed from needs_review to positive_review
comment:16 Changed 8 years ago by
- 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
- Status changed from needs_work to needs_review
comment:18 Changed 8 years ago by
- Description modified (diff)
comment:19 Changed 8 years ago by
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
- 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: ↓ 23 Changed 8 years ago by
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
- Description modified (diff)
comment:23 in reply to: ↑ 21 ; follow-up: ↓ 24 Changed 8 years ago by
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
comment:25 Changed 8 years ago by
- Milestone set to sage-5.0
comment:26 follow-up: ↓ 27 Changed 8 years ago by
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
comment:28 Changed 8 years ago by
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
- Merged in set to sage-5.0.beta14
- Resolution set to fixed
- Status changed from positive_review to closed
Patch to remove buggy sub_ddmmss implementation