Opened 5 years ago

Closed 5 years ago

#16348 closed defect (fixed)

Cdd linker path

Reported by: vbraun Owned by:
Priority: blocker Milestone: sage-6.3
Component: build Keywords:
Cc: Merged in:
Authors: Volker Braun Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: 55d2466 (Commits) Commit: 55d2466229dbb6e198e4979a46ce337f99991f93
Dependencies: Stopgaps:

Description (last modified by vbraun)

Another cdd buildsystem horror...

Change History (23)

comment:1 Changed 5 years ago by vbraun

  • Branch set to u/vbraun/cdd_linker_path

comment:2 Changed 5 years ago by vbraun

  • Authors set to Volker Braun
  • Commit set to 55d2466229dbb6e198e4979a46ce337f99991f93
  • Component changed from PLEASE CHANGE to build
  • Description modified (diff)
  • Status changed from new to needs_review
  • Type changed from PLEASE CHANGE to defect

New commits:

55d2466remove /usr/local from include paths

comment:3 Changed 5 years ago by leif

Wouldn't it make sense to add a separate patch for this?

(The other changes are unrelated, and probably subject to change/vanish with #15871.)

comment:4 follow-up: Changed 5 years ago by vbraun

Feel free to change the way changes are split into patches.

comment:5 Changed 5 years ago by leif

The main culprit is

export gmpdir := /usr/local

in cddlib-094g.p0/src/Makefile, so calling $MAKE gmpdir="$SAGE_LOCAL might also work without patching anything further, or just changing that line.

comment:6 in reply to: ↑ 4 Changed 5 years ago by leif

Replying to vbraun:

Feel free to change the way changes are split into patches.

Not my job.

comment:7 follow-up: Changed 5 years ago by vbraun

You can override gmpdir, but then SAGE_LOCAL will be added multiple times to the include/linker path.

comment:8 Changed 5 years ago by vbraun

  • Priority changed from major to blocker

comment:9 in reply to: ↑ 7 Changed 5 years ago by leif

Replying to vbraun:

You can override gmpdir, but then SAGE_LOCAL will be added multiple times to the include/linker path.

... which doesn't hurt (unless we're e.g. exceeding the maximal number of command line arguments, but then other packages and especially the Sage library won't build either).

comment:10 Changed 5 years ago by leif

... or just remove adding "$SAGE_LOCAL/..." to CPPFLAGS and LDFLAGS in turn.

comment:11 Changed 5 years ago by vbraun

These are all equivalent solutions. None has any significant advantage over the others.

comment:12 follow-up: Changed 5 years ago by leif

Well, even in Sage ;-) we have the convention

  • to only patch upstream if necessary (and submit such patches upstream if appropriate)
  • one issue per patch
  • ...

It's tedious and error-prone to later untangle cumulative patches, not to mention reviewing and documentation of patches.

comment:13 in reply to: ↑ 12 Changed 5 years ago by vbraun

Either off topic or doesn't work here (because its likely to trigger autoconf runs)

comment:14 Changed 5 years ago by leif

http://trac.sagemath.org/ticket/13026#comment:15

To go triple-safe, touch the autotools-generated files in spkg-install after patching.

But as mentioned, patching the autotools source files is not even necessary in this case (although it would make sense to submit such a patch upstream, and hopefully remove it (and only this specific one!) upon one of the next upstream upgrades).

I can't follow what otherwise would be off-topic, but that's probably intended.

comment:15 follow-up: Changed 5 years ago by vbraun

The .am patches are not applied in spkg-install, fyi.

Last edited 5 years ago by vbraun (previous) (diff)

comment:16 in reply to: ↑ 15 Changed 5 years ago by leif

Replying to vbraun:

The .am patches are not applied in spkg-install, fyi.

I know, which is IMHO a bad hack anyway.

But how is this related?

comment:17 Changed 5 years ago by vbraun

IMHO all of cddlib is a bad hack. We basically only have it because gfan depends on it.

In any case, this ticket is about making cddlib compile if there is something in /usr/local. Suggestions about how to reorganize the patches and still avoid re-running autoconf are off-topic, as you said.

comment:18 Changed 5 years ago by leif

I'm talking about making the spkg even less maintainable (and distro-unfriendly), which is what you currently IMHO do, for no reason.

The issue here btw. popped up again because a previous patch got removed upon the last upgrade.

comment:19 Changed 5 years ago by vbraun

No the issue popped up again because replacing the upstream insanity with a sane autotools project was removed.

comment:20 follow-up: Changed 5 years ago by fbissey

This is so wrong. Volker's patch is just the minimal work to make it more distro friendly. I would remove the "export"s statements from the the top Makefile.am too. That particular mistake is usually not too bad for distro preparing binary packages in a sensible way. But it is ugly as hell.

To finish with the comments on distro unfriendliness we fortunately start with upstream adding only the piece we deem necessary. We don't start from the sage spkg unless there is no other sources around :)

I am a bit uncomfortable with the way autotools patches are applied and enforced but I can accept that. I am more to apply the full autotool solution that could be pushed upstream if they were responsive rather than the quick nice fix proposed by Leif. So if I have a deciding vote I am going for Volker.

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

Replying to fbissey:

To finish with the comments on distro unfriendliness we fortunately start with upstream adding only the piece we deem necessary. We don't start from the sage spkg unless there is no other sources around :)

To clarify, I'm not against patching the autotools files, I'm against mixing completely unrelated patches. Especially if we want to submit parts of it upstream. (And it's easy to make sure they get applied in the correct order if at all relevant. Same for timestamps if patched files depend on other patched files.)

And I'm sorry to say, but unfortunately Gentoo still isn't the only distro. ;-)

Others prefer to have patches/fix_upstream_bug_{foo,bar}.patch, patches/add_sage_functionality_baz.patch rather than patches/modifying_{this,that}_file_for_a_couple_of_reason_you_can_probably_guess.patch.

Even for just Sage, the former is by far more easy to maintain.


I am a bit uncomfortable with the way autotools patches are applied and enforced but I can accept that. I am more to apply the full autotool solution that could be pushed upstream if they were responsive rather than the quick nice fix proposed by Leif. So if I have a deciding vote I am going for Volker.

You have it, just give it positive review if you really feel it's the right thingTM to do. Also in the light of #15871.

Although the Debianists should probably speak up, too.

comment:22 in reply to: ↑ 21 Changed 5 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

Replying to leif:

Replying to fbissey:

To finish with the comments on distro unfriendliness we fortunately start with upstream adding only the piece we deem necessary. We don't start from the sage spkg unless there is no other sources around :)

To clarify, I'm not against patching the autotools files, I'm against mixing completely unrelated patches. Especially if we want to submit parts of it upstream. (And it's easy to make sure they get applied in the correct order if at all relevant. Same for timestamps if patched files depend on other patched files.)

And I'm sorry to say, but unfortunately Gentoo still isn't the only distro. ;-)

How very unfortunate ;) that's why I have to read rpm .spec files from time to time and even troll debian sources.But that was a long and boring dragging comment - the kind you make in the morning because you are gathering your thoughts.

Others prefer to have patches/fix_upstream_bug_{foo,bar}.patch, patches/add_sage_functionality_baz.patch rather than patches/modifying_{this,that}_file_for_a_couple_of_reason_you_can_probably_guess.patch.

Even for just Sage, the former is by far more easy to maintain.


I am a bit uncomfortable with the way autotools patches are applied and enforced but I can accept that. I am more to apply the full autotool solution that could be pushed upstream if they were responsive rather than the quick nice fix proposed by Leif. So if I have a deciding vote I am going for Volker.

You have it, just give it positive review if you really feel it's the right thingTM to do. Also in the light of #15871.

Although the Debianists should probably speak up, too.

I guess we would need to invite them next time ;)

In the absence of a nice way to run autoreconf and al. on pristine source (as Gentoo would) it is good enough for me. What we should send upstream and in which form is another discussion.

comment:23 Changed 5 years ago by vbraun

  • Branch changed from u/vbraun/cdd_linker_path to 55d2466229dbb6e198e4979a46ce337f99991f93
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.