#25188 closed defect (fixed)

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

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.5
Component: build Keywords:
Cc: embray Merged in:
Authors: Erik Bray Reviewers: Volker Braun, Julian Rüth, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 5d0a981 (Commits) Commit: 5d0a98124f3e2b6d32de6fc1ffffc6ea660ea2b4
Dependencies: #25857 Stopgaps:

Description (last modified by jdemeyer)

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).

Attachments (1)

Makefile (58.9 KB) - added by jdemeyer 21 months ago.
Auto-generated build/make/Makefile

Download all attachments as: .zip

Change History (45)

Changed 21 months ago by jdemeyer

Auto-generated build/make/Makefile

comment:1 Changed 21 months ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 21 months ago by jdemeyer

  • Description modified (diff)

comment:3 follow-ups: Changed 21 months ago by embray

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.

comment:4 follow-up: Changed 21 months ago by embray

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.

comment:5 Changed 21 months ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/build/ticket-25188
  • Commit set to 2f97ecfef40f234c2a3b3ff40efe59dce967cb7b
  • Status changed from new to needs_review
  • Summary changed from DUMMY_PACKAGES does not work to Repeated configure run with gcc already installed treats gcc as not installed

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

comment:6 Changed 21 months ago by git

  • Commit changed from 2f97ecfef40f234c2a3b3ff40efe59dce967cb7b to 0baf86234801c1da822ed8e2aa946ebe6a141d9b

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

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

comment:7 in reply to: ↑ 3 ; follow-up: Changed 21 months ago by 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.

comment:8 in reply to: ↑ 4 Changed 21 months ago by jdemeyer

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.

comment:9 in reply to: ↑ 3 ; follow-up: Changed 21 months ago by jdemeyer

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.

comment:10 Changed 21 months ago by jdemeyer

  • Milestone changed from sage-8.2 to sage-duplicate/invalid/wontfix
  • Resolution set to worksforme
  • Status changed from needs_review to closed

Never mind, user error...

comment:11 in reply to: ↑ 7 Changed 21 months ago by embray

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.

comment:12 Changed 21 months ago by embray

  • Resolution worksforme deleted
  • Status changed from closed to new

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).

comment:13 Changed 21 months ago by embray

  • Status changed from new to needs_review

comment:14 in reply to: ↑ 9 Changed 21 months ago by embray

Replying to jdemeyer:

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.

Right, I didn't think you were doing that. I'm just explaining that that capability exists (even if it's sometimes not a good idea), and that that's nothing new.

comment:15 Changed 21 months ago by jdemeyer

  • Description modified (diff)

comment:16 Changed 21 months ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-duplicate/invalid/wontfix to sage-8.2

comment:17 Changed 21 months ago by vbraun

  • Reviewers set to Volker Braun

Lgtm; Jeroen, any comments?

comment:18 Changed 21 months ago by jdemeyer

I honestly don't understand the issue here. Which problem is this trying to solve?

comment:19 follow-up: Changed 21 months ago by embray

It's resolving the thing I at least thought you were confused about.

Say you want to install gcc in Sage (whether explicitly, or by requirement of configure):

$ SAGE_INSTALL_GCC=yes ./configure

then ./configure correctly outputs:

checking package versions...
...
    gcc-7.2.0
...

Then you install the gcc SPKG (explicitly or otherwise):

$ make gcc

And in the Makefile you see

BUILT_PACKAGES =\
...
gcc \
...

and

DUMMY_PACKAGES = \
    curl \
    gfortran \
    git \

However, if you then re-run ./configure (without explicitly supplying SAGE_INSTALL_GCC=yes):

checking package versions...
...
    gcc-7.2.0 not installed (configure check)
...

it now says gcc spkg will not be installed (even though it is in fact already installed, and should continue to be installed). Furthermore this means:

DUMMY_PACKAGES = \
    curl \
    gcc \
    gfortran \
    git \

so the stamp file for the gcc spkg is now $SAGE_SPKG_INST/.dummy and not the real stamp file for the actual installed gcc. This obviously isn't the intended behavior, nor the behavior for any other package. This bug is just unique to the gcc package because of how its installation state is being managed by the configure script.

comment:20 in reply to: ↑ 19 Changed 21 months ago by jdemeyer

Replying to embray:

This obviously isn't the intended behavior

I actually think that it is intended.

comment:21 Changed 21 months ago by vbraun

  • Branch changed from u/embray/build/ticket-25188 to 0baf86234801c1da822ed8e2aa946ebe6a141d9b
  • Resolution set to fixed
  • Status changed from needs_review to closed

comment:22 follow-up: Changed 21 months ago by vbraun

  • Branch changed from 0baf86234801c1da822ed8e2aa946ebe6a141d9b to u/embray/build/ticket-25188
  • Resolution fixed deleted
  • Status changed from closed to new

I think the affected users would prefer not to recompile gcc if they can avoid it... Though I'll leave it to you two to figure it out. But if there is no resultion real soon (TM) then it won't be in 8.2.

comment:23 Changed 21 months ago by vbraun

  • Status changed from new to needs_review

comment:24 in reply to: ↑ 22 Changed 21 months ago by jdemeyer

Replying to vbraun:

I think the affected users would prefer not to recompile gcc if they can avoid it...

Indeed. that's the idea. We never recompile GCC just for fun.

comment:25 follow-up: Changed 21 months ago by embray

It's not that it's recompiling GCC. It's a reminder that GCC is supposed to be installed in Sage in the first place. It's really wrong--it means that if you delete $SAGE_LOCAL/var/lib/sage/installed/gcc-* then the gcc package won't get rebuilt. You do want that because otherwise the package is for all intents and purposes broken. I also don't know why you would want to treat gcc differently from any other package in this regard.

The only way I could see this remotely making sense is if proper uninstallation were also implemented. Otherwise you're effectively saying "gcc is not part of this Sage distribution" even though it is installed.

comment:26 Changed 21 months ago by vbraun

  • Priority changed from blocker to major

Either way, doesn't strike me as a blocker.

comment:27 in reply to: ↑ 25 Changed 21 months ago by jdemeyer

Replying to embray:

I also don't know why you would want to treat gcc differently from any other package in this regard.

Because GCC tends to give cyclic build dependencies: GCC depends on MPIR but MPIR depends on GCC.

comment:28 Changed 21 months ago by embray

Okay, but the current behavior doesn't seem like the correct way to resolve that.

comment:29 Changed 18 months ago by saraedum

  • Reviewers changed from Volker Braun to Volker Braun, Julian Rüth

This seems like a reasonable change to me. Feel free to set this to positive review from my end. What's the current status here embray?

comment:30 Changed 18 months ago by saraedum

  • Status changed from needs_review to needs_info

comment:31 Changed 18 months ago by embray

Jeroen has expressed some concerns that this has problems related to cyclic dependencies. I think it's the fact that when you install sage-gcc, mpir, mpc, mpfr, and other dependencies of gcc are installed first in order to bootstrap it, then gcc is build, then its dependencies are rebuilt with the new gcc. After this point, if dependencies of gcc get rebuilt, it should not trigger gcc to be rebuilt as well.

I think, with this patch, that still would not happen, but I need to make sure.

Either way, I don't think pretending that sage-gcc is not selected is a good way to deal with this. Better to see if a special case can be made for toolchain dependencies (as in the more recent ticket #25857).

comment:32 Changed 18 months ago by git

  • Commit changed from 0baf86234801c1da822ed8e2aa946ebe6a141d9b to d71c88f0f54243300aa5968ecf4827606a642e80

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

b3d4e6ebuild/make/Makefile should be rebuilt when build/make/Makefile.in is updated
0c5fd1eIf sage-gcc is already installed there is no guarantee these macros will be run
2965003add vim modeline for this file
f5f68daAdd a package that by all rights should be included in this list
15c7b88move this list into a TOOLCHAIN_DEPS variable we can use to inspect this list elsewhere
1954e5eAdd -k/--keep-existing to sage-spkg
f1411c5From <spkg>-clean targets in the Makefile, just remove the package's
9976091Use sage-spkg --keep-existing when installing/re-installing packages in
22a1f34Revert to using sage-spkg-uninstall in <spkg>-clean targets, but add a
d71c88fMerge branch 'u/embray/ticket-25857' into u/embray/build/ticket-25188

comment:33 Changed 18 months ago by git

  • Commit changed from d71c88f0f54243300aa5968ecf4827606a642e80 to f062068a46d4e8a3d4e964cd2cac1ffd63e45644

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

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

comment:34 Changed 18 months ago by embray

  • Dependencies set to #25857
  • Status changed from needs_info to needs_review

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.

comment:35 Changed 18 months ago by embray

  • Milestone changed from sage-8.2 to sage-8.4

comment:36 Changed 17 months ago by dimpase

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

comment:37 Changed 17 months ago by embray

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

comment:38 Changed 15 months ago by dimpase

  • Reviewers changed from Volker Braun, Julian Rüth to Volker Braun, Julian Rüth, Dima Pasechnik
  • Status changed from needs_review to positive_review

comment:39 Changed 15 months ago by vbraun

Merge conflict

comment:40 follow-up: Changed 15 months ago by vbraun

  • Status changed from positive_review to needs_work

comment:41 in reply to: ↑ 40 Changed 15 months ago by dimpase

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.

comment:42 Changed 15 months ago by dimpase

  • Branch changed from u/embray/build/ticket-25188 to public/build/ticket-25188
  • Commit changed from f062068a46d4e8a3d4e964cd2cac1ffd63e45644 to 5d0a98124f3e2b6d32de6fc1ffffc6ea660ea2b4
  • Status changed from needs_work to positive_review

a trivial merge

comment:43 Changed 15 months ago by dimpase

  • Milestone changed from sage-8.4 to sage-8.5

comment:44 Changed 15 months ago by vbraun

  • Branch changed from public/build/ticket-25188 to 5d0a98124f3e2b6d32de6fc1ffffc6ea660ea2b4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.