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

Remove broken SAGE_BUILD_TOOLCHAIN support #27215

Closed
jdemeyer opened this issue Feb 4, 2019 · 28 comments
Closed

Remove broken SAGE_BUILD_TOOLCHAIN support #27215

jdemeyer opened this issue Feb 4, 2019 · 28 comments

Comments

@jdemeyer
Copy link

jdemeyer commented Feb 4, 2019

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.

CC: @embray

Component: build

Author: Jeroen Demeyer

Branch/Commit: 7e2b100

Reviewer: Erik Bray

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

@jdemeyer jdemeyer added this to the sage-8.7 milestone Feb 4, 2019
@jdemeyer
Copy link
Author

jdemeyer commented Feb 4, 2019

@jdemeyer
Copy link
Author

jdemeyer commented Feb 4, 2019

Commit: 8e727df

@jdemeyer
Copy link
Author

jdemeyer commented Feb 4, 2019

New commits:

8e727dfRemove broken SAGE_BUILD_TOOLCHAIN support

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 4, 2019

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

7184b6cRemove broken SAGE_BUILD_TOOLCHAIN support

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 4, 2019

Changed commit from 8e727df to 7184b6c

@dimpase
Copy link
Member

dimpase commented Mar 11, 2019

comment:5

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.

@dimpase
Copy link
Member

dimpase commented Mar 11, 2019

comment:6

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

@dimpase
Copy link
Member

dimpase commented Mar 11, 2019

comment:7

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

@jdemeyer
Copy link
Author

comment:8

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.

@dimpase
Copy link
Member

dimpase commented Mar 11, 2019

comment:9

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

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@embray
Copy link
Contributor

embray commented Mar 11, 2019

comment:12

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?

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

comment:14

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.

@embray
Copy link
Contributor

embray commented Mar 11, 2019

comment:15

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.

@embray
Copy link
Contributor

embray commented Mar 11, 2019

Reviewer: Erik Bray

@embray
Copy link
Contributor

embray commented Mar 11, 2019

comment:16

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.

@embray
Copy link
Contributor

embray commented Mar 11, 2019

comment:17

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.

@jdemeyer
Copy link
Author

comment:18

Replying to @embray:

For successfully built systems

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

@embray
Copy link
Contributor

embray commented Mar 11, 2019

comment:19

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.

@jdemeyer
Copy link
Author

comment:20

OK, I see your point.

@dimpase
Copy link
Member

dimpase commented Mar 11, 2019

comment:21

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

@embray
Copy link
Contributor

embray commented Mar 12, 2019

comment:22

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 12, 2019

Changed commit from 7184b6c to 7e2b100

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 12, 2019

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

7e2b100Remove broken SAGE_BUILD_TOOLCHAIN support

@jdemeyer
Copy link
Author

comment:24

Removed the package version bumping.

@vbraun
Copy link
Member

vbraun commented Mar 14, 2019

Changed branch from u/jdemeyer/remove_broken_sage_build_toolchain_support to 7e2b100

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

4 participants