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
Don't (effectively) source sage-env more than once #10469
Comments
comment:1
Thoughts? |
This comment has been minimized.
This comment has been minimized.
Attachment: trac_10469.patch.gz |
comment:3
It seems reasonable to me, so I've included a patch which does everything except versioning. This will obsolete #4029. |
Author: Ivan Andrus |
scripts repo; replaces other patch |
Attachment: trac_10469-scripts.patch.gz Attachment: trac_10469.v3.patch.gz |
comment:4
While we're at it, why don't we just make sage-env non-executable? Patch attached. |
This comment has been minimized.
This comment has been minimized.
comment:5
What is the purpose of checking |
comment:6
Replying to @gvol:
You're probably right, and if this is always called via a call to sage, this could probably be deleted. As it stands, the changes here look pretty safe to me, but removing that check seems a bit riskier, and something which would require more serious testing. |
Changed author from Ivan Andrus to Ivan Andrus, John Palmieri, Keshav Kini |
comment:7
I'm happy with all of the changes here, but since I helped to write them, I don't think I should give this a positive review. Anyone else? Ivan or Keshav? |
comment:8
I'm happy to give it a positive review. |
Reviewer: Ivan Andrus |
Attachment: trac_10469.v4.patch.gz |
comment:9
I don't like the #!/this/script/must/be/sourced |
This comment has been minimized.
This comment has been minimized.
comment:12
John, Mercurial tracks file modes. The diffs it exports (and thus the patches mq exports) do not mention them unless you put "[diff]\ngit = true" in your .hgrc. My patch does this - you can just remove the hashbang line from it. Generally putting "[diff]\ngit = true" in your .hgrc is a good idea, since it frees Mercurial from having to remain compatible with ancient pre-dvcs diff formats. |
comment:13
Oh, I'm sorry Jeroen. Somehow I thought you were John Palmieri... |
git format of Jeroen's v4 patch, which tracks the filemode change |
comment:14
Attachment: trac_10469.v5[1].patch.gz Added a patch as described and simplified the application instructions so the patchbot can read them more easily (?) |
This comment has been minimized.
This comment has been minimized.
comment:15
Doctest error:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
comment:17
Okay, I know what's going on. This is a bug in a different aspect of Sage, but perhaps it can be fixed on this ticket. On sage.math.washington.edu, the directory Here's a patch (to the Sage library) which applies os.path.realpath to both the value of SAGE_ROOT and the output from "which gap" (etc.). This fixes the problem for me. |
Attachment: trac_10469-sage-repo.patch.gz main Sage repo |
comment:18
The patch makes sense to me, I will test that it works. |
Changed reviewer from Ivan Andrus to Ivan Andrus, Jeroen Demeyer |
comment:20
For reviewing this, my opinion is that only attachment: trac_10469-sage-repo.patch needs to be reviewed: the other patch has a positive review already. For this Sage repo patch, make sure to test it on sage.math, and in particular, build and test Sage in a subdirectory of the /scratch directory (since /scratch is symbolically linked to another directory). It might be a good idea to build it on some other platforms in directories which are symbolic links, to make sure os.path.realpath(...) is doing the right thing. |
comment:21
I promised to test it and I'm actually going to do it for real this time :-) |
comment:22
Works for me, on |
Merged: sage-4.7.1.alpha0 |
comment:23
See #11449 for a follow-up (do not make |
comment:24
Some funny side effect, i.e. a bug that only gets really visible with #11021 (substituting almost all instances of
The error originates from The following Note that previously, after merging this ticket (#10469), the error just didn't show up because during the build Long story, for short: If (At least) I'll provide a patch to |
comment:25
The idea of versioning |
This can currently happen because
some scripts source it themselves (like e.g. at least
sage-spkg
, which is necessary sincesage-spkg
isn't always called throughsage-sage
, which also sources it),some receipts in the top-level
Makefile
source it before some other script is called throughsage-sage
,sage-sage
itself or other (e.g. Python) code may runsage ...
commands such thatsage-sage
is then called recursively, again sourcingsage-env
.To achieve this, we could simply add
near its top.
We may even put a version number into that variable and execute (perhaps only some of) the commands in
sage-env
"again" in case it was modified during running scripts, which could be helpful when upgrading Sage.Such a variable also allows still generic, but safer tests than the usual
[ -z "$SAGE_LOCAL" ]
, sinceSAGE_LOCAL
being defined doesn't really implysage-env
was sourced, but we usually intend to ensure that some environment variables (like e.g.CC
orUNAME
) are properly set up, without testing each of these individually.Effectively sourcing (at least parts of)
sage-env
only once alsoPATH
etc.,SAGE_ORIG_LD_LIBRARY_PATH
), also simplifying other scripts (cf. sage-native-execute does not unset path etc. #10286),Also, we should change the permissions of
sage-env
to non-executable (it must be sourced, not executed).CC: @jhpalmieri @qed777 @gvol
Component: build
Keywords: environment variables sage-sage scripts
Author: Ivan Andrus, John Palmieri, Keshav Kini
Reviewer: Ivan Andrus, Jeroen Demeyer
Merged: sage-4.7.1.alpha0
Issue created by migration from https://trac.sagemath.org/ticket/10469
The text was updated successfully, but these errors were encountered: