Opened 3 years ago

Closed 3 years ago

#27422 closed defect (fixed)

local/bin/sage-env-config may not be up to date

Reported by: jdemeyer Owned by:
Priority: blocker Milestone: sage-8.8
Component: build Keywords:
Cc: fbissey, embray Merged in:
Authors: Jeroen Demeyer Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: 939177a (Commits, GitHub, GitLab) Commit: 939177af6646f9c91c32444ed5da1104d43655fb
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

The top-level configure writes src/bin/sage-env-config. However, the file local/bin/sage-env-config is what is actually used by the build system. At some point during the build, src/bin/sage-env-config is copied to local/bin/sage-env-config but this does not happen immediately. This leads to race conditions where packages may be built with the wrong sage-env-config.

A very simple solution is to prefer src/bin/sage-env-config in case that both are present.

Change History (45)

comment:1 Changed 3 years ago by jdemeyer

  • Summary changed from gfan and lcalc no longer build with old gcc to gfan and lcalc no longer build with old g++

comment:2 Changed 3 years ago by jdemeyer

  • Cc fbissey added

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

  • Cc fbissey removed

But the auto is new C++, how did it work before? Is it because this sets C++ to too old a standard?

comment:4 Changed 3 years ago by dimpase

  • Cc fbissey added

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

Replying to dimpase:

But the auto is new C++, how did it work before?

Before #26787, these packages were compiled with -std=c++11. So the obvious fix is simply to put back those flags which were removed in #26787.

comment:6 Changed 3 years ago by dimpase

Francois says on #26787 that it's meant to be that CXX becomes $CXX -std=gnu++11 But somehow this does not happen here for some reason.

Could you post the sage-env-config from one of these broken systems here?

comment:7 Changed 3 years ago by jdemeyer

After trying again, it now works correctly...

So maybe there is a problem with dependency checking, that the settings for CXX are not propagated correctly?

comment:8 Changed 3 years ago by dimpase

Perhaps for some reason ./configure didn't get run (or perhaps ./bootstrap)? My understanding is that it ./configure would fill in the template in sage-env-config.in, producing sage-env-config with the correct settings, and sage-env will set CXX to the right value.

comment:9 Changed 3 years ago by jdemeyer

This happened when upgrading from 8.6. I can try to again to see if I can reproduce it.

comment:10 Changed 3 years ago by dimpase

I can think of a number of ways to interpret "upgrading". Please be more specific.

comment:11 follow-up: Changed 3 years ago by fbissey

I am guessing just doing make after synchronising didn't run configure.

comment:12 Changed 3 years ago by jdemeyer

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

I cannot reproduce this.

comment:13 Changed 3 years ago by jdemeyer

I just got this issue again. This time, I remember better which steps I took, so I'll try to reproduce it.

comment:14 in reply to: ↑ 11 ; follow-up: Changed 3 years ago by jdemeyer

Replying to fbissey:

I am guessing just doing make after synchronising didn't run configure.

But it should according to the top-level Makefile.

comment:15 in reply to: ↑ 14 Changed 3 years ago by fbissey

Replying to jdemeyer:

Replying to fbissey:

I am guessing just doing make after synchronising didn't run configure.

But it should according to the top-level Makefile.

The glob build/pkgs/*/* in the build/make/Makefile target should indeed trigger it. At least that's what we expect. But you should have logs showing whether it happened or not.

comment:16 Changed 3 years ago by jdemeyer

If I recall correctly, then both this time and the last time, the build worked on the third invocation of make build. So there were 2 failed builds of gfan and the third one worked. So somewhere in between, ./configure was run correctly.

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

I guess that's all that toolchain things that should have been removed from Sage proper many years ago. Now the build system is so complicated that it slows the development down.

Once #27212, #27258, and #27259 are in, every toolchain part can come from the system (or an external custom package), and I see no reason for not moving the toolchain and its deps into a separate custom package.

comment:18 Changed 3 years ago by jdemeyer

I tried to reproduce, but again I didn't manage.

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

Replying to dimpase:

Once #27212, #27258, and #27259 are in

Why are you bringing up mpir/mpfr/mpc? There is no evidence that these packages have anything to do with the problem that I'm seeing here.

comment:20 Changed 3 years ago by dimpase

I am bringing this up as our build system badly needs to be simplified. We waste time on things that should just work. See #27462.

comment:21 Changed 3 years ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-duplicate/invalid/wontfix to sage-8.7
  • Summary changed from gfan and lcalc no longer build with old g++ to local/bin/sage-env-config may not be up to date

I think I finally figured out the root cause, I updated the description.

comment:22 Changed 3 years ago by jdemeyer

  • Cc embray added
  • Resolution worksforme deleted
  • Status changed from closed to new

comment:23 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:24 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:25 Changed 3 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)

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

I haven't actually looked at the code, but if what you write in the description is the case then that's definitely a bug. It should only use the one in $SAGE_ROOT/src/bin when it's present.

comment:27 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/local_bin_sage_env_config_may_not_be_up_to_date

comment:28 in reply to: ↑ 26 Changed 3 years ago by jdemeyer

  • Commit set to 939177af6646f9c91c32444ed5da1104d43655fb
  • Status changed from new to needs_review

Replying to embray:

It should only use the one in $SAGE_ROOT/src/bin when it's present.

I think there is a misunderstanding.

The problem is when both local/bin/sage-env-config and src/bin/sage-env-config are present. It used to prefer local/bin/sage-env-config which is not guaranteed to be up-to-date. Instead, it should prefer src/bin/sage-env-config which is directly generated by configure.

I went for the most simple solution here. There are probably better solutions, for example having only one such file. But those require bigger changes and I didn't want to do that for a blocker ticket.


New commits:

939177aLook for src/bin/sage-env-config first

comment:29 Changed 3 years ago by vbraun

  • Priority changed from blocker to major

Imho its better to ship 8.7 now, realistically we can't make any promises about upgrading from old versions.

comment:30 Changed 3 years ago by jdemeyer

  • Priority changed from major to blocker

It's true that we can't make promises, but in general upgrades do work quite well. But this ticket has broken upgrading builds several times for me. It's also a regression since 8.6, so it's very sensible to make it a blocker. Moreover, this fix is easy, why not apply it?

In the end, you have the last word, but I would like you to reconsider this ticket.

comment:31 Changed 3 years ago by jdemeyer

If you don't like the fix itself, there are plenty of other ways to fix the same problem. But I do think that the problem must be fixed for 8.7.

comment:32 Changed 3 years ago by vbraun

  • Status changed from needs_review to positive_review

I see your point, but I also think its fairly low impact. The perfect is the enemy of the good...

comment:33 Changed 3 years ago by chapoton

  • Reviewers set to Volker Braun

comment:34 Changed 3 years ago by embray

Amusingly, I just got bitten by this myself, but only because I copied an existing source tree to a new location.

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

  • Milestone changed from sage-8.7 to sage-8.8
  • Status changed from positive_review to needs_work

This whole thing is totally insane - does it support ./configure --prefix= ?

Shouldn't it be possible to have a number of build trees, with potentially different sage-env-config ? Shouldn't src/bin/sage-env-config actually not be there at all?! Why do we keep mixing up build trees with source...

Last edited 3 years ago by dimpase (previous) (diff)

comment:36 in reply to: ↑ 35 ; follow-up: Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Replying to dimpase:

This whole thing is totally insane - does it support ./configure --prefix= ?

Of course it does, why should it not?

Shouldn't src/bin/sage-env-config actually not be there at all?!

But then how is $SAGE_LOCAL determined? If you use --prefix, there needs to be at least one place where the location of $SAGE_LOCAL is stored and that place is src/bin/sage-env-config.

I'm setting this back to needs_review since I don't really understand your complaint.

comment:37 in reply to: ↑ 36 ; follow-ups: Changed 3 years ago by dimpase

Replying to jdemeyer:

Replying to dimpase:

This whole thing is totally insane - does it support ./configure --prefix= ?

Of course it does, why should it not?

Shouldn't src/bin/sage-env-config actually not be there at all?!

But then how is $SAGE_LOCAL determined? If you use --prefix, there needs to be at least one place where the location of $SAGE_LOCAL is stored and that place is src/bin/sage-env-config.

OK, fine, but why does this even get copied into $SAGE_LOCAL/bin/?

Its place is not there, not in somewhere that is in the $PATH. but rather in something like $SAGE_LOCAL/config/ or so...

I hope this explains.

I'm setting this back to needs_review since I don't really understand your complaint.

comment:38 in reply to: ↑ 37 ; follow-up: Changed 3 years ago by jdemeyer

Replying to dimpase:

Its place is not there, not in somewhere that is in the $PATH. but rather in something like $SAGE_LOCAL/config/ or so...

But then you have a circular problem:

  1. you need to know $SAGE_LOCAL in order to find the file $SAGE_LOCAL/config/sage-env-config
  1. you need to read $SAGE_LOCAL/config/sage-env-config to know $SAGE_LOCAL

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

  • Status changed from needs_review to positive_review

Sage is traditionally built/run out of its source tree by many users, who are also developers. But it can also be run, for the most part, outside of its source tree (running sage -i doesn't work outside the source tree, which is something I believe should be fixed, but that's a bigger topic).

Having a file that is loaded from one place when running out of the source, but a different place when "installed" is normal, and there are many cases like that in Sage. This makes perfect sense to me as-is.

comment:40 in reply to: ↑ 37 Changed 3 years ago by embray

Replying to dimpase:

Replying to jdemeyer:

Replying to dimpase:

This whole thing is totally insane - does it support ./configure --prefix= ?

Of course it does, why should it not?

Shouldn't src/bin/sage-env-config actually not be there at all?!

But then how is $SAGE_LOCAL determined? If you use --prefix, there needs to be at least one place where the location of $SAGE_LOCAL is stored and that place is src/bin/sage-env-config.

OK, fine, but why does this even get copied into $SAGE_LOCAL/bin/?

When running $SAGE_LOCAL/bin/sage for example, it uses $SAGE_LOCAL/bin/sage-env, which in turn should source $SAGE_LOCAL/bin/sage-env-config (which just sets a few variables that are determined by ./configure and are placed in a separate file from the main sage-env mostly for clarity's sake...)

comment:41 Changed 3 years ago by jdemeyer

I should also note that after installation the contents of $SAGE_SRC/bin/sage-env-config and $SAGE_LOCAL/bin/sage-env-config are identical. But this ticket is meant to fix sage-env-config at build time, possibly before $SAGE_SRC/bin/sage-env-config is copied $SAGE_LOCAL/bin/sage-env-config.

comment:42 in reply to: ↑ 38 Changed 3 years ago by dimpase

Replying to jdemeyer:

Replying to dimpase:

Its place is not there, not in somewhere that is in the $PATH. but rather in something like $SAGE_LOCAL/config/ or so...

But then you have a circular problem:

  1. you need to know $SAGE_LOCAL in order to find the file $SAGE_LOCAL/config/sage-env-config

You just said that this is available from src/bin/sage-env-config, no?

  1. you need to read $SAGE_LOCAL/config/sage-env-config to know $SAGE_LOCAL

No, this is in src/bin/sage-env-config, that's how you can find it.

Well, maybe there is my aversion to changing the environment without running ./configure that speaks here, but this is really the same issue as in #27373.

comment:43 in reply to: ↑ 39 ; follow-up: Changed 3 years ago by dimpase

Replying to embray:

Sage is traditionally built/run out of its source tree by many users, who are also developers. But it can also be run, for the most part, outside of its source tree (running sage -i doesn't work outside the source tree, which is something I believe should be fixed, but that's a bigger topic).

Do you say that sage -i doesn't work if ./configure --prefix=X was run with X != $SAGE_LOCAL?

Having a file that is loaded from one place when running out of the source, but a different place when "installed" is normal, and there are many cases like that in Sage. This makes perfect sense to me as-is.

In this case "installed" ought to happen at the end, after the build is done and dusted.

comment:44 in reply to: ↑ 43 Changed 3 years ago by embray

Replying to dimpase:

Replying to embray:

Sage is traditionally built/run out of its source tree by many users, who are also developers. But it can also be run, for the most part, outside of its source tree (running sage -i doesn't work outside the source tree, which is something I believe should be fixed, but that's a bigger topic).

Do you say that sage -i doesn't work if ./configure --prefix=X was run with X != $SAGE_LOCAL?

No, that has nothing to do with it. I'm just saying it doesn't work if you don't run Sage out of a source tree, because manually installing packages still depends on the whole sage-the-distribution machinery which does not get "installed" in any sense. It was just an aside; I'm sorry if my mentioning it confused matters.

comment:45 Changed 3 years ago by vbraun

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