Opened 12 years ago

Last modified 10 years ago

#10469 closed defect

Don't (effectively) source sage-env more than once — at Version 11

Reported by: Leif Leonhardy Owned by: Georg S. Weber
Priority: major Milestone: sage-4.7.1
Component: build Keywords: environment variables sage-sage scripts
Cc: John Palmieri, Mitesh Patel, Ivan Andrus Merged in:
Authors: Ivan Andrus, John Palmieri, Keshav Kini Reviewers: Ivan Andrus
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

This can currently happen because

  • some scripts source it themselves (like e.g. at least sage-spkg, which is necessary since sage-spkg isn't always called through sage-sage, which also sources it),
  • some receipts in the top-level Makefile source it before some other script is called through sage-sage,
  • sage-sage itself or other (e.g. Python) code may run sage ... commands such that sage-sage is then called recursively, again sourcing sage-env.

To achieve this, we could simply add

# Don't execute the commands below more than once:
if [ -z "$SAGE_ENV_SOURCED" ]; then
    export SAGE_ENV_SOURCED=1  # or "yes", "true" or alike, but see below (versioning)
else
    # Already sourced, nothing to do.
    return 0
fi

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" ], since SAGE_LOCAL being defined doesn't really imply sage-env was sourced, but we usually intend to ensure that some environment variables (like e.g. CC or UNAME) are properly set up, without testing each of these individually.

Effectively sourcing (at least parts of) sage-env only once also

  • avoids redundant entries in PATH etc.,
  • avoids special tests if some variable was already defined / modified (like e.g. SAGE_ORIG_LD_LIBRARY_PATH), also simplifying other scripts (cf. #10286),
  • IMHO reduces the risk of unintentional side-effects, and
  • speeds up execution.

Also, we should replace the useless

#!/usr/bin/env bash

at its top by (e.g.)

#!/this/script/must/be/sourced

which causes an error if sage-env is called rather than sourced (which makes no sense and therefore is indeed an error), with an "appropriate" error message since such an interpreter certainly does not exist.

Apply trac_10469.v4.patch to local/bin and change permissions of local/bin/sage-env to 0644.

Change History (15)

comment:1 Changed 12 years ago by Leif Leonhardy

Thoughts?

comment:2 Changed 12 years ago by Leif Leonhardy

Description: modified (diff)

Changed 12 years ago by Ivan Andrus

Attachment: trac_10469.patch added

comment:3 Changed 12 years ago by Ivan Andrus

Authors: Ivan Andrus
Status: newneeds_review

It seems reasonable to me, so I've included a patch which does everything except versioning. This will obsolete #4029.

Changed 12 years ago by John Palmieri

Attachment: trac_10469-scripts.patch added

scripts repo; replaces other patch

Changed 12 years ago by Keshav Kini

Attachment: trac_10469.v3.patch added

comment:4 Changed 12 years ago by Keshav Kini

Description: modified (diff)

While we're at it, why don't we just make sage-env non-executable? Patch attached.

comment:5 Changed 12 years ago by Ivan Andrus

What is the purpose of checking SAGE_ROOT but not doing anything with it? Is it just so that it prints a warning? It seems that if sage-env has been sourced SAGE_ROOT should be set correctly.

comment:6 in reply to:  5 Changed 12 years ago by John Palmieri

Replying to iandrus:

What is the purpose of checking SAGE_ROOT but not doing anything with it? Is it just so that it prints a warning? It seems that if sage-env has been sourced SAGE_ROOT should be set correctly.

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.

comment:7 Changed 12 years ago by John Palmieri

Authors: Ivan AndrusIvan Andrus, John Palmieri, Keshav Kini

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 Changed 12 years ago by Ivan Andrus

Reviewers: Ivan Andrus
Status: needs_reviewpositive_review

I'm happy to give it a positive review.

Changed 12 years ago by Jeroen Demeyer

Attachment: trac_10469.v4.patch added

comment:9 Changed 12 years ago by Jeroen Demeyer

Status: positive_reviewneeds_work

I don't like the #!/this/script/must/be/sourced In my v4 patch, I removed that line. Instead, I think a much better solution is simply to make sage-env not executable.

comment:10 Changed 12 years ago by Jeroen Demeyer

Status: needs_workneeds_review

comment:11 Changed 12 years ago by Jeroen Demeyer

Description: modified (diff)
Note: See TracTickets for help on using tickets.