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

Repeated configure run with gcc already installed treats gcc as not installed #25188

Closed
jdemeyer opened this issue Apr 17, 2018 · 51 comments
Closed

Comments

@jdemeyer
Copy link
Contributor

If ./configure detects an existing GCC installed in Sage (SAGE_INSTALL_GCC=exists) then it should keep gcc selected among the packages to be installed as part of the distribution. Otherwise it basically deselects gcc which is the wrong thing to do (with any package).

Depends on #25857

CC: @embray

Component: build

Author: Erik Bray

Branch/Commit: 5d0a981

Reviewer: Volker Braun, Julian Rüth, Dima Pasechnik

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

@jdemeyer jdemeyer added this to the sage-8.2 milestone Apr 17, 2018
@jdemeyer
Copy link
Contributor Author

Attachment: Makefile.gz

Auto-generated build/make/Makefile

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@embray
Copy link
Contributor

embray commented Apr 17, 2018

comment:3

I don't think this is any different from how it used to work. For each package, there is a variable in the Makefile called inst_<pkgname>. If the package should be installed (particularly as a dependency of another package) its inst_<pkgname> should point to the real stamp file for that package under SAGE_SPKG_INSTS. However, if the package is not being installed in Sage (i.e. due to a configure check) then its inst_<pkgname> just points to .dummy. But explicit build rules for that package are still added to the Makefile, so if you run make gfortran you can still install the gfortran package, overriding the configure-time determination.

I'm not 100% convinced that's the best behavior (I think if you want to override a configure-based selection you should have to actually re-run configure). But that's how it always worked until now.

@embray
Copy link
Contributor

embray commented Apr 17, 2018

comment:4

I think the problem has more to do with the oddities of how gcc is being handled, especially since #24703.

I was confused because you wrote that "gcc is also installed", but in your Makefile it has gcc in DUMMY_PACKAGES implying that it was not selected for installation. Yet all the packages have $(SAGE_LOCAL)/bin/gcc as dependencies implying that it was installed at some point.

I think that what #24703 missed is that even if $SAGE_INSTALL_GCC = exists it needs to be setting needs_to_install_gcc = yes (this doesn't actually mean it will reinstall gcc if it isn't necessary to--this just means gcc is selected for possible installation in Sage, pending checking of its prerequisites by make). Since that isn't happening, the build system gets into a funny state where Sage's gcc is installed, but the build system doesn't really think it should be.

@embray
Copy link
Contributor

embray commented Apr 17, 2018

Branch: u/embray/build/ticket-25188

@embray
Copy link
Contributor

embray commented Apr 17, 2018

Author: Erik Bray

@embray
Copy link
Contributor

embray commented Apr 17, 2018

Commit: 2f97ecf

@embray
Copy link
Contributor

embray commented Apr 17, 2018

comment:5

I'm not positive since you weren't clear on the actual symptoms, but I think this should fix the basic problem (I'm guessing that it was a complaint about no target for $(SAGE_LOCAL)/bin/gcc).


New commits:

2f97ecfshould still set need_to_install_gcc=yes in configure if Sage's gcc is already installed

@embray embray changed the title DUMMY_PACKAGES does not work Repeated configure run with gcc already installed treats gcc as not installed Apr 17, 2018
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 17, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

0baf862build/make/Makefile should be rebuild with build/make/Makefile.in is updated

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 17, 2018

Changed commit from 2f97ecf to 0baf862

@jdemeyer
Copy link
Contributor Author

comment:7

I haven't reviewed the patch, but it would really be useful if DEBUG_RULES=1 would show everything that ends up in the Makefile. In particular, the expansions of the inst_FOO variables are currently missing.

This is precisely the sort of thing why I opposed #21524: the Makefile rules are too obscure to understand what is broken.

@jdemeyer
Copy link
Contributor Author

comment:8

Replying to @embray:

I was confused because you wrote that "gcc is also installed", but in your Makefile it has gcc in DUMMY_PACKAGES implying that it was not selected for installation. Yet all the packages have $(SAGE_LOCAL)/bin/gcc as dependencies implying that it was installed at some point.

Yes, I meant "gcc has been installed at some point in the past". configure correctly detects that gfortran should not be installed, but it's still being installed anyway.

@jdemeyer
Copy link
Contributor Author

comment:9

Replying to @embray:

if you run make gfortran you can still install the gfortran package, overriding the configure-time determination.

To be clear: this is not what I'm doing. gfortran is build during the normal build process.

@jdemeyer
Copy link
Contributor Author

comment:10

Never mind, user error...

@jdemeyer jdemeyer removed this from the sage-8.2 milestone Apr 17, 2018
@embray
Copy link
Contributor

embray commented Apr 18, 2018

comment:11

Replying to @jdemeyer:

I haven't reviewed the patch, but it would really be useful if DEBUG_RULES=1 would show everything that ends up in the Makefile. In particular, the expansions of the inst_FOO variables are currently missing.

This is precisely the sort of thing why I opposed #21524: the Makefile rules are too obscure to understand what is broken.

I don't think anything's really being obscured here. The old hard-coded makefile didn't expand those variables either so if you suspected something to do with them you've lost nothing.

@embray
Copy link
Contributor

embray commented Apr 18, 2018

comment:12

Well don't just close it. You've been very vague about what the actual problem is here, though you led me to actually examine the issue closely and this fix actually is the correct thing to do.

If ./configure detects an existing GCC installed in Sage (SAGE_INSTALL_GCC=exists) then it should keep gcc selected among the packages to be installed as part of the distribution. Otherwise it basically deselects gcc which is the wrong thing to do (with any package).

@embray embray reopened this Apr 18, 2018
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 18, 2018

Changed commit from d71c88f to f062068

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 18, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

f062068fix accidentally removed blank line needed to make debugging output look better

@embray
Copy link
Contributor

embray commented Jul 18, 2018

comment:34

I had to make a small change over the previous fix, but I'm pretty confident this works. I also had to merge in the fix from #25857 in order to be able to be able to test this at all without breaking things too much.

You can install sage-gcc, then packages that gcc depends on like mpir can be reinstalled and updated as much as you want. This isn't a problem because, in part, all of gcc's dependencies are listed as prerequisite-only.

@embray
Copy link
Contributor

embray commented Jul 18, 2018

Dependencies: #25857

@embray embray modified the milestones: sage-8.2, sage-8.4 Jul 18, 2018
@dimpase
Copy link
Member

dimpase commented Sep 7, 2018

comment:36

I suppose this also needs configure tarball and bump...

@embray
Copy link
Contributor

embray commented Sep 7, 2018

comment:37

It might not need a configure tarball, since this looks like one of those ones that "happens to work" even without rebuilding configure.

@dimpase
Copy link
Member

dimpase commented Oct 15, 2018

Changed reviewer from Volker Braun, Julian Rüth to Volker Braun, Julian Rüth, Dima Pasechnik

@vbraun
Copy link
Member

vbraun commented Oct 18, 2018

comment:39

Merge conflict

@dimpase
Copy link
Member

dimpase commented Oct 19, 2018

comment:41

Replying to @vbraun:
IIRC that work branch was https://github.com/sagemath/sage - but this does not seem to be the case anymore. Please advice where to look.

@dimpase
Copy link
Member

dimpase commented Oct 28, 2018

comment:42

a trivial merge

@dimpase
Copy link
Member

dimpase commented Oct 28, 2018

Changed branch from u/embray/build/ticket-25188 to public/build/ticket-25188

@dimpase
Copy link
Member

dimpase commented Oct 28, 2018

Changed commit from f062068 to 5d0a981

@vbraun
Copy link
Member

vbraun commented Oct 29, 2018

Changed branch from public/build/ticket-25188 to 5d0a981

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