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

disabling the MAPLE interface to linbox #21482

Closed
dimpase opened this issue Sep 13, 2016 · 32 comments
Closed

disabling the MAPLE interface to linbox #21482

dimpase opened this issue Sep 13, 2016 · 32 comments

Comments

@dimpase
Copy link
Member

dimpase commented Sep 13, 2016

this is to address an issue mentioned on #17635.

Upstream: Fixed upstream, in a later stable release.

CC: @dcoudert

Component: packages: standard

Author: Clément Pernet, Dima Pasechnik

Branch/Commit: 5dbd252

Reviewer: David Coudert

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

@dimpase dimpase added this to the sage-7.4 milestone Sep 13, 2016
@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 13, 2016

comment:2

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

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 13, 2016

comment:3

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.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 13, 2016

comment:4

Replying to @nexttime:

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

@dimpase
Copy link
Member Author

dimpase commented Sep 13, 2016

Branch: u/dimpase/disable_maple_linbox

@dimpase
Copy link
Member Author

dimpase commented Sep 13, 2016

comment:5

here is the patch.
please test.


New commits:

02bcc52patch from comment 231 of #17635

@dimpase
Copy link
Member Author

dimpase commented Sep 13, 2016

Commit: 02bcc52

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 13, 2016

comment:6

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.

@dimpase
Copy link
Member Author

dimpase commented Sep 13, 2016

comment:8

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

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 13, 2016

comment:9

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

@dimpase
Copy link
Member Author

dimpase commented Sep 13, 2016

comment:10

Replying to @nexttime:

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

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 13, 2016

comment:11

Replying to @dimpase:

Replying to @nexttime:

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.

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 13, 2016

comment:12

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.

@fchapoton
Copy link
Contributor

comment:13

seems to work for me

@ClementPernet
Copy link
Contributor

comment:14

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

@nexttime
Copy link
Mannequin

nexttime mannequin commented Sep 13, 2016

comment:15

Replying to @ClementPernet:

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.

@ClementPernet
Copy link
Contributor

comment:16

Replying to @nexttime:

IMHO there are things to get fixed in other ways upstream

Of course. This is now linbox-team/linbox#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.

@dcoudert
Copy link
Contributor

comment:17

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?

@ClementPernet
Copy link
Contributor

Attachment: linbox-sage-21482.patch.gz

Alternative fix

@dimpase
Copy link
Member Author

dimpase commented Sep 14, 2016

Upstream: Fixed upstream, in a later stable release.

@dimpase
Copy link
Member Author

dimpase commented Sep 14, 2016

comment:18

OK, Clement, why won't you take over and produce a git branch and a link to the updated linbox tarball?

@ClementPernet
Copy link
Contributor

comment:19

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.

@ClementPernet
Copy link
Contributor

New commits:

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

@ClementPernet
Copy link
Contributor

Changed commit from 02bcc52 to 5dbd252

@ClementPernet
Copy link
Contributor

Changed branch from u/dimpase/disable_maple_linbox to u/cpernet/linbox_maple

@ClementPernet
Copy link
Contributor

comment:21

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

@dcoudert
Copy link
Contributor

comment:22

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

@dcoudert
Copy link
Contributor

comment:23

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

@ClementPernet
Copy link
Contributor

comment:24

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

@dcoudert
Copy link
Contributor

Reviewer: David Coudert

@dcoudert
Copy link
Contributor

comment:25

This patch solves my compilation error with 7.4-beta4.
Thank you.
David.

@ClementPernet
Copy link
Contributor

Author: Clément Pernet, Dima Pasechnik

@vbraun
Copy link
Member

vbraun commented Sep 17, 2016

Changed branch from u/cpernet/linbox_maple to 5dbd252

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

5 participants