Opened 5 years ago
Closed 5 years ago
#21482 closed defect (fixed)
disabling the MAPLE interface to linbox
Reported by: | dimpase | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.4 |
Component: | packages: standard | Keywords: | |
Cc: | dcoudert | Merged in: | |
Authors: | Clément Pernet, Dima Pasechnik | Reviewers: | David Coudert |
Report Upstream: | Fixed upstream, in a later stable release. | Work issues: | |
Branch: | 5dbd252 (Commits, GitHub, GitLab) | Commit: | 5dbd2523f7e388a3e037d383e47eacd8c5accce3 |
Dependencies: | Stopgaps: |
Description
this is to address an issue mentioned on #17635.
Attachments (1)
Change History (28)
comment:1 Changed 5 years ago by
- Cc dcoudert added
comment:2 follow-up: ↓ 4 Changed 5 years ago by
comment:3 Changed 5 years ago by
Random comment: It seems we should disable the use of "external" packages when building bdists anyway, as these might not be present on target machines even with the same distro.
comment:4 in reply to: ↑ 2 Changed 5 years ago by
comment:5 follow-up: ↓ 9 Changed 5 years ago by
- Branch set to u/dimpase/disable_maple_linbox
- Cc dcoudert removed
- Commit set to 02bcc52d7eacb53fa209261e6dc9092b39345240
- Status changed from new to needs_review
comment:6 Changed 5 years ago by
P.P.S.: There are IMHO a couple of issues related to this we should report and fix upstream, but on another ticket, as this is just for working around the current brokenness in Sage.
After all, I'm not patching configure.ac
, just configure
, which is also much easier to maintain.
comment:7 Changed 5 years ago by
- Cc dcoudert added
comment:8 Changed 5 years ago by
somehow I managed to clear CC field while changing the ticket, sorry. Restored. Ready for review.
comment:9 in reply to: ↑ 5 ; follow-up: ↓ 10 Changed 5 years ago by
comment:10 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 5 years ago by
Replying to leif:
Replying to dimpase:
here is the patch. please test.
New commits:
02bcc52 patch from comment 231 of #17635
Well, that's a slightly different patch (to
configure
), also "disabling"--with-default=yes
(to not say break the latter...).
it should do the same thing - I got lost in nested robot-written ifs there...
comment:11 in reply to: ↑ 10 Changed 5 years ago by
Replying to dimpase:
Replying to leif:
Replying to dimpase:
here is the patch. please test.
New commits:
02bcc52 patch from comment 231 of #17635
Well, that's a slightly different patch (to
configure
), also "disabling"--with-default=yes
(to not say break the latter...).it should do the same thing - I got lost in nested robot-written ifs there...
Well, what was the reason to change the patch to configure
at all?
If someone now changes spkg-install
, it's again broken.
comment:12 Changed 5 years ago by
By the way, --with-maple=no
doesn't work as expected because we also pass --with-all=...
, but I think I already mentioned that on #17635.
comment:13 Changed 5 years ago by
seems to work for me
comment:14 follow-up: ↓ 15 Changed 5 years ago by
The --with-all forces all --with-XXX to yes, therefore --with-maple was not working.
I suggest to simply change the --with-all="$SAGE_LOCAL"
with --with-default="$SAGE_LOCAL"
.
This seems to be a simpler/smaller fix and works on my box. Let me know how it works on yours before I mess up with Dima's patch.
Clément
comment:15 in reply to: ↑ 14 ; follow-up: ↓ 16 Changed 5 years ago by
Replying to cpernet:
The --with-all forces all --with-XXX to yes, therefore --with-maple was not working.
I suggest to simply change the
--with-all="$SAGE_LOCAL"
with--with-default="$SAGE_LOCAL"
.This seems to be a simpler/smaller fix and works on my box. Let me know how it works on yours before I mess up with Dima's patch.
Clément
See my previous comments; IMHO there are things to get fixed in other ways upstream (besides the compiler errors themselves), I'll probably post on a further ticket, since here we just want to fix the Sage build.
comment:16 in reply to: ↑ 15 Changed 5 years ago by
Replying to leif:
IMHO there are things to get fixed in other ways upstream
Of course. This is now https://github.com/linbox-team/linbox/issues/38
(besides the compiler errors themselves),
I'm not sure to have found which one you're referring to.
For me, the option --with-all
is the bug here, as we definitely do not need/want LinBox? to look for all of its possible dependencies.
The --with-default
is the one we want, to specify the common folder where to find the dependencies that are turned on when found (namely fflas-ffpack, NTL, IML).
I see no point in adding a patch to the configure file.
comment:17 Changed 5 years ago by
With this patch, I'm now able to compile 7.4-beta4. Thank you !
Should I set this ticket to positive review or you want to improve it first?
comment:18 follow-up: ↓ 19 Changed 5 years ago by
- Report Upstream changed from N/A to Fixed upstream, in a later stable release.
OK, Clement, why won't you take over and produce a git branch and a link to the updated linbox tarball?
comment:19 in reply to: ↑ 18 Changed 5 years ago by
Replying to dimpase:
OK, Clement, why won't you take over and produce a git branch and a link to the updated linbox tarball?
Sure, I'll do it. I just didn't want to step on your toes, since you already posted a branch.
No need to update the linbox tarball.
comment:20 Changed 5 years ago by
- Branch changed from u/dimpase/disable_maple_linbox to u/cpernet/linbox_maple
- Commit changed from 02bcc52d7eacb53fa209261e6dc9092b39345240 to 5dbd2523f7e388a3e037d383e47eacd8c5accce3
New commits:
5dbd252 | repace --with-all by --with-default for #21482
|
comment:21 Changed 5 years ago by
@dcoudert can you confirm this new branch fixes your compilation error?
comment:22 Changed 5 years ago by
I made a distclean before trying this patch and it's working ! Cool :)
comment:23 Changed 5 years ago by
Can I set this ticket to positive review? or is it better to wait for feedback from others?
comment:24 Changed 5 years ago by
You can, as you could initially reproduce the bug and now say that it's fixed with this branch.
comment:25 Changed 5 years ago by
- Reviewers set to David Coudert
- Status changed from needs_review to positive_review
This patch solves my compilation error with 7.4-beta4. Thank you. David.
comment:26 Changed 5 years ago by
comment:27 Changed 5 years ago by
- Branch changed from u/cpernet/linbox_maple to 5dbd2523f7e388a3e037d383e47eacd8c5accce3
- Resolution set to fixed
- Status changed from positive_review to closed
Ok, I'll create a branch, but I've also commented on #17635 on how my patch there could be tested until then...