Opened 3 years ago
Closed 3 years ago
#27422 closed defect (fixed)
local/bin/sageenvconfig may not be up to date
Reported by:  jdemeyer  Owned by:  

Priority:  blocker  Milestone:  sage8.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: 
Description (last modified by )
The toplevel configure
writes src/bin/sageenvconfig
. However, the file local/bin/sageenvconfig
is what is actually used by the build system. At some point during the build, src/bin/sageenvconfig
is copied to local/bin/sageenvconfig
but this does not happen immediately. This leads to race conditions where packages may be built with the wrong sageenvconfig
.
A very simple solution is to prefer src/bin/sageenvconfig
in case that both are present.
Change History (45)
comment:1 Changed 3 years ago by
 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
 Cc fbissey added
comment:3 followup: ↓ 5 Changed 3 years ago by
 Cc fbissey removed
comment:4 Changed 3 years ago by
 Cc fbissey added
comment:5 in reply to: ↑ 3 Changed 3 years ago by
comment:6 Changed 3 years ago by
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 sageenvconfig
from one of these broken systems here?
comment:7 Changed 3 years ago by
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
Perhaps for some reason ./configure
didn't get run (or perhaps ./bootstrap
)? My understanding is that it ./configure
would fill in the template in sageenvconfig.in
, producing sageenvconfig
with the correct settings, and sageenv
will set CXX
to the right value.
comment:9 Changed 3 years ago by
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
I can think of a number of ways to interpret "upgrading". Please be more specific.
comment:11 followup: ↓ 14 Changed 3 years ago by
I am guessing just doing make
after synchronising didn't run configure
.
comment:12 Changed 3 years ago by
 Milestone changed from sage8.7 to sageduplicate/invalid/wontfix
 Resolution set to worksforme
 Status changed from new to closed
I cannot reproduce this.
comment:13 Changed 3 years ago by
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 ; followup: ↓ 15 Changed 3 years ago by
Replying to fbissey:
I am guessing just doing
make
after synchronising didn't runconfigure
.
But it should according to the toplevel Makefile
.
comment:15 in reply to: ↑ 14 Changed 3 years ago by
Replying to jdemeyer:
Replying to fbissey:
I am guessing just doing
make
after synchronising didn't runconfigure
.But it should according to the toplevel
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
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 followup: ↓ 19 Changed 3 years ago by
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
I tried to reproduce, but again I didn't manage.
comment:19 in reply to: ↑ 17 Changed 3 years ago by
comment:20 Changed 3 years ago by
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
 Description modified (diff)
 Milestone changed from sageduplicate/invalid/wontfix to sage8.7
 Summary changed from gfan and lcalc no longer build with old g++ to local/bin/sageenvconfig 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
 Cc embray added
 Resolution worksforme deleted
 Status changed from closed to new
comment:23 Changed 3 years ago by
 Description modified (diff)
comment:24 Changed 3 years ago by
 Description modified (diff)
comment:25 Changed 3 years ago by
 Description modified (diff)
comment:26 followup: ↓ 28 Changed 3 years ago by
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
 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
 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/sageenvconfig
and src/bin/sageenvconfig
are present. It used to prefer local/bin/sageenvconfig
which is not guaranteed to be uptodate. Instead, it should prefer src/bin/sageenvconfig
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:
939177a  Look for src/bin/sageenvconfig first

comment:29 Changed 3 years ago by
 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
 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
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
 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
 Reviewers set to Volker Braun
comment:34 Changed 3 years ago by
Amusingly, I just got bitten by this myself, but only because I copied an existing source tree to a new location.
comment:35 followup: ↓ 36 Changed 3 years ago by
 Milestone changed from sage8.7 to sage8.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 sageenvconfig
? Shouldn't src/bin/sageenvconfig
actually not be there at all?!
Why do we keep mixing up build trees with source...
comment:36 in reply to: ↑ 35 ; followup: ↓ 37 Changed 3 years ago by
 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/sageenvconfig
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/sageenvconfig
.
I'm setting this back to needs_review since I don't really understand your complaint.
comment:37 in reply to: ↑ 36 ; followups: ↓ 38 ↓ 40 Changed 3 years ago by
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/sageenvconfig
actually not be there at all?!But then how is
$SAGE_LOCAL
determined? If you useprefix
, there needs to be at least one place where the location of$SAGE_LOCAL
is stored and that place issrc/bin/sageenvconfig
.
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 ; followup: ↓ 42 Changed 3 years ago by
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:
 you need to know
$SAGE_LOCAL
in order to find the file$SAGE_LOCAL/config/sageenvconfig
 you need to read
$SAGE_LOCAL/config/sageenvconfig
to know$SAGE_LOCAL
comment:39 followup: ↓ 43 Changed 3 years ago by
 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 asis.
comment:40 in reply to: ↑ 37 Changed 3 years ago by
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/sageenvconfig
actually not be there at all?!But then how is
$SAGE_LOCAL
determined? If you useprefix
, there needs to be at least one place where the location of$SAGE_LOCAL
is stored and that place issrc/bin/sageenvconfig
.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/sageenv
, which in turn should source $SAGE_LOCAL/bin/sageenvconfig
(which just sets a few variables that are determined by ./configure
and are placed in a separate file from the main sageenv
mostly for clarity's sake...)
comment:41 Changed 3 years ago by
I should also note that after installation the contents of $SAGE_SRC/bin/sageenvconfig
and $SAGE_LOCAL/bin/sageenvconfig
are identical. But this ticket is meant to fix sageenvconfig
at build time, possibly before $SAGE_SRC/bin/sageenvconfig
is copied $SAGE_LOCAL/bin/sageenvconfig
.
comment:42 in reply to: ↑ 38 Changed 3 years ago by
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:
 you need to know
$SAGE_LOCAL
in order to find the file$SAGE_LOCAL/config/sageenvconfig
You just said that this is available from src/bin/sageenvconfig
, no?
 you need to read
$SAGE_LOCAL/config/sageenvconfig
to know$SAGE_LOCAL
No, this is in src/bin/sageenvconfig
, 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 ; followup: ↓ 44 Changed 3 years ago by
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 asis.
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
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 withX != $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 sagethedistribution 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
 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
But the
auto
is new C++, how did it work before? Is it because this sets C++ to too old a standard?