Opened 4 years ago

Closed 3 years ago

#27215 closed defect (fixed)

Remove broken SAGE_BUILD_TOOLCHAIN support

Reported by: jdemeyer Owned by:
Priority: blocker Milestone: sage-8.7
Component: build Keywords:
Cc: embray Merged in:
Authors: Jeroen Demeyer Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: 7e2b100 (Commits, GitHub, GitLab) Commit: 7e2b10087c2e81539644d12c0a70cd1a95ea7866
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

The environment variable SAGE_BUILD_TOOLCHAIN has always been a bit of a hack. It is currently broken (probably due to #24919): imagine a system where the gcc Sage package is installed. If mpir is upgraded, then mpir is rebuilt (as dependency of gcc) with SAGE_BUILD_TOOLCHAIN but there is nothing forcing a second build of mpir without SAGE_BUILD_TOOLCHAIN.

Instead of trying to fix SAGE_BUILD_TOOLCHAIN, we just stop supporting it: toolchain packages are now built once just like any other package.

Change History (25)

comment:1 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/remove_broken_sage_build_toolchain_support

comment:2 Changed 4 years ago by jdemeyer

  • Commit set to 8e727df24312c855ae101213f8b2bc8fcd1f6752

New commits:

8e727dfRemove broken SAGE_BUILD_TOOLCHAIN support

comment:3 Changed 4 years ago by git

  • Commit changed from 8e727df24312c855ae101213f8b2bc8fcd1f6752 to 7184b6c25559ae01abb89c84ef54a0a4dfc26227

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7184b6cRemove broken SAGE_BUILD_TOOLCHAIN support

comment:4 Changed 4 years ago by jdemeyer

  • Status changed from new to needs_review

comment:5 Changed 3 years ago by dimpase

I think #27373 shows the other side of the same problem (things aren't nice of mpir/gmp) comes from the system rather than is built.

Version 0, edited 3 years ago by dimpase (next)

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

Does this ticket try to repair anything, or just removes stuff?

comment:7 Changed 3 years ago by dimpase

  • Cc embray added

I guess #27212 ought to depend on this one, or the other way around, otherwise merging gets messy.

comment:8 in reply to: ↑ 6 Changed 3 years ago by jdemeyer

Replying to dimpase:

Does this ticket try to repair anything

Yes, as explained in the ticket description. That's also why it's a blocker ticket: it breaks builds whenever the Sage GCC package is installed.

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

The ticket description does not explain the change in functionality that should be the result, it only explains the defect.

comment:10 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:11 Changed 3 years ago by jdemeyer

  • Description modified (diff)

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

Replying to dimpase:

The ticket description does not explain the change in functionality that should be the result, it only explains the defect.

It doesn't even explain the defect. Just "it breaks". What breaks? How? How does this fix it? Is there anything this fix breaks?

comment:13 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:14 in reply to: ↑ 12 Changed 3 years ago by jdemeyer

Replying to embray:

Just "it breaks". What breaks?

With all due respect, I think that this explanation from the ticket description is more than just "it breaks":

imagine a system where the gcc Sage package is installed. If mpir is upgraded, then mpir is rebuilt (as dependency of gcc) with SAGE_BUILD_TOOLCHAIN but there is nothing forcing a second build of mpir.

If you have more concrete questions, I'm happy to answer them.

comment:15 Changed 3 years ago by embray

  • Reviewers set to Erik Bray
  • Status changed from needs_review to positive_review

I had to re-read it several times (I don't know if that's a problem with the wording, or if it's just subtle), but I think I understand now. And because mpir is not rebuilt, in this case, with SAGE_BUILD_TOOLCHAIN=no it is missing some functionality I guess (specifically it is built without the C++ library).

I think there probably could be a better way to indicate if mpir is being built as a dependency of a (not yet built) gcc. But I agree that there's not a whole lot of advantage to doing so, especially if the feature breaks normal builds in service to a (what I would consider) the rather marginal use case of installing Sage's own gcc.

comment:16 Changed 3 years ago by embray

I guess it would be a problem on a system without a C++ compiler to begin with (per #12782). I suppose we're saying we don't care about that though--install a minimal C++ compiler as a prerequisite.

comment:17 follow-up: Changed 3 years ago by embray

  • Status changed from positive_review to needs_info

Actually one quick question before I set back to positive-review: Unless this issue actually broke some buildbots or something, do we really want to increase the package versions? For successfully built systems this changes nothing about their GMP/MPIR builds, so I would rather not force the overhead of rebuilding those packages and their many dependencies if we don't really need to.

comment:18 in reply to: ↑ 17 Changed 3 years ago by jdemeyer

Replying to embray:

For successfully built systems

Well, this change was meant for non-successfully built systems.

comment:19 Changed 3 years ago by embray

I think, up to a point, we don't have to care about a few broken builds due to issues introduced several versions ago, unless we have some specific known need to (e.g. fixing some incremental builds on buildbots). Otherwise you're just forcing a lot of needless electricity usage.

comment:20 Changed 3 years ago by jdemeyer

OK, I see your point.

comment:21 Changed 3 years ago by dimpase

So, do we agree do not bump up the versions here? I'd like to merge this branch into #27212...

comment:22 Changed 3 years ago by embray

I leave it up to Jeroen--I think it's a sensible policy to always default to bumping package versions when updating their spkg-install, but I also believe there are cases (and this is one of them) where it might be smarter not to unless there's a strong reason for doing so.

comment:23 Changed 3 years ago by git

  • Commit changed from 7184b6c25559ae01abb89c84ef54a0a4dfc26227 to 7e2b10087c2e81539644d12c0a70cd1a95ea7866

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7e2b100Remove broken SAGE_BUILD_TOOLCHAIN support

comment:24 Changed 3 years ago by jdemeyer

  • Status changed from needs_info to positive_review

Removed the package version bumping.

comment:25 Changed 3 years ago by vbraun

  • Branch changed from u/jdemeyer/remove_broken_sage_build_toolchain_support to 7e2b10087c2e81539644d12c0a70cd1a95ea7866
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.