Opened 3 years ago

Closed 3 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) Commit: 5dbd2523f7e388a3e037d383e47eacd8c5accce3
Dependencies: Stopgaps:

Description

this is to address an issue ​mentioned on #17635.

Attachments (1)

linbox-sage-21482.patch​ (1010 bytes) - added by cpernet 3 years ago.
Alternative fix

Download all attachments as: .zip

Change History (28)

comment:1 Changed 3 years ago by dcoudert

  • Cc dcoudert added

comment:2 follow-up: Changed 3 years ago by leif

Ok, I'll create a branch, but I've also commented on #17635 on how my patch there could be tested until then...

comment:3 Changed 3 years ago by leif

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 3 years ago by leif

Replying to leif:

Ok, I'll create a branch, but I've also commented on #17635 on how my patch there could be tested until then...

P.S.: TBH, I didn't create one yesterday also because I didn't recall into which beta #17635 finally got merged... ;-) (Again, sudo fill merged_field.)

comment:5 follow-up: Changed 3 years ago by dimpase

  • Branch set to u/dimpase/disable_maple_linbox
  • Cc dcoudert removed
  • Commit set to 02bcc52d7eacb53fa209261e6dc9092b39345240
  • Status changed from new to needs_review

here is the patch. please test.

New commits:

​02bcc52patch from comment 231 of #17635

comment:6 Changed 3 years ago by leif

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 3 years ago by dimpase

  • Cc dcoudert added

comment:8 Changed 3 years ago by dimpase

somehow I managed to clear CC field while changing the ticket, sorry. Restored. Ready for review.

comment:9 in reply to: ↑ 5 ; follow-up: Changed 3 years ago by leif

Replying to dimpase:

here is the patch. please test.

New commits:

​02bcc52patch 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...).

Last edited 3 years ago by leif (previous) (diff)

comment:10 in reply to: ↑ 9 ; follow-up: Changed 3 years ago by dimpase

Replying to leif:

Replying to dimpase:

here is the patch. please test.

New commits:

​02bcc52patch 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 3 years ago by leif

Replying to dimpase:

Replying to leif:

Replying to dimpase:

here is the patch. please test.

New commits:

​02bcc52patch 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 3 years ago by leif

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 3 years ago by chapoton

seems to work for me

comment:14 follow-up: Changed 3 years ago by 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

comment:15 in reply to: ↑ 14 ; follow-up: Changed 3 years ago by leif

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 3 years ago by cpernet

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 3 years ago by dcoudert

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?

Changed 3 years ago by cpernet

Alternative fix

comment:18 follow-up: Changed 3 years ago by dimpase

  • 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 3 years ago by cpernet

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 3 years ago by cpernet

  • Branch changed from u/dimpase/disable_maple_linbox to u/cpernet/linbox_maple
  • Commit changed from 02bcc52d7eacb53fa209261e6dc9092b39345240 to 5dbd2523f7e388a3e037d383e47eacd8c5accce3

New commits:

​5dbd252repace --with-all by --with-default for #21482

comment:21 Changed 3 years ago by cpernet

@dcoudert can you confirm this new branch fixes your compilation error?

comment:22 Changed 3 years ago by dcoudert

I made a distclean before trying this patch and it's working ! Cool :)

comment:23 Changed 3 years ago by dcoudert

Can I set this ticket to positive review? or is it better to wait for feedback from others?

comment:24 Changed 3 years ago by cpernet

You can, as you could initially reproduce the bug and now say that it's fixed with this branch.

comment:25 Changed 3 years ago by dcoudert

  • 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 3 years ago by cpernet

  • Authors set to Clément Pernet, Dima Pasechnik

comment:27 Changed 3 years ago by vbraun

  • Branch changed from u/cpernet/linbox_maple to 5dbd2523f7e388a3e037d383e47eacd8c5accce3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.