Opened 3 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: |
Description (last modified by )
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 3 years ago by
- Branch set to u/jdemeyer/remove_broken_sage_build_toolchain_support
comment:2 Changed 3 years ago by
- Commit set to 8e727df24312c855ae101213f8b2bc8fcd1f6752
comment:3 Changed 3 years ago by
- Commit changed from 8e727df24312c855ae101213f8b2bc8fcd1f6752 to 7184b6c25559ae01abb89c84ef54a0a4dfc26227
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7184b6c | Remove broken SAGE_BUILD_TOOLCHAIN support
|
comment:4 Changed 3 years ago by
- Status changed from new to needs_review
comment:5 Changed 3 years ago by
I think #27373 shows the other side of the same problem--things aren't nice with mpir/gmp if they come from the system rather than built.
comment:6 follow-up: ↓ 8 Changed 3 years ago by
Does this ticket try to repair anything, or just removes stuff?
comment:7 Changed 3 years ago by
- 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
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: ↓ 12 Changed 3 years ago by
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
- Description modified (diff)
comment:11 Changed 3 years ago by
- Description modified (diff)
comment:12 in reply to: ↑ 9 ; follow-up: ↓ 14 Changed 3 years ago by
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
- Description modified (diff)
comment:14 in reply to: ↑ 12 Changed 3 years ago by
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. Ifmpir
is upgraded, thenmpir
is rebuilt (as dependency ofgcc
) withSAGE_BUILD_TOOLCHAIN
but there is nothing forcing a second build ofmpir
.
If you have more concrete questions, I'm happy to answer them.
comment:15 Changed 3 years ago by
- 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
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: ↓ 18 Changed 3 years ago by
- 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
Replying to embray:
For successfully built systems
Well, this change was meant for non-successfully built systems.
comment:19 Changed 3 years ago by
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
OK, I see your point.
comment:21 Changed 3 years ago by
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
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
- Commit changed from 7184b6c25559ae01abb89c84ef54a0a4dfc26227 to 7e2b10087c2e81539644d12c0a70cd1a95ea7866
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
7e2b100 | Remove broken SAGE_BUILD_TOOLCHAIN support
|
comment:24 Changed 3 years ago by
- Status changed from needs_info to positive_review
Removed the package version bumping.
comment:25 Changed 3 years ago by
- Branch changed from u/jdemeyer/remove_broken_sage_build_toolchain_support to 7e2b10087c2e81539644d12c0a70cd1a95ea7866
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Remove broken SAGE_BUILD_TOOLCHAIN support