Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#12647 closed enhancement (fixed)

Add support for a "sagerc" script

Reported by: Jeroen Demeyer Owned by: Leif Leonhardy
Priority: major Milestone: sage-5.0
Component: scripts Keywords:
Cc: Merged in: sage-5.0.beta9
Authors: Jeroen Demeyer Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by John Palmieri)

This deals with adding support for a file

$DOT_SAGE/sagerc

which will be sourced after sage-env, so it can be used to override the environment variables used in Sage.

Apply:

  1. 12647_sagerc.patch to SAGE_ROOT.
  2. 12647_sagerc_doc.patch, trac_12647-review.patch to the Sage library.

Attachments (3)

12647_sagerc_doc.patch (2.8 KB) - added by Jeroen Demeyer 11 years ago.
trac_12647-review.patch (1.2 KB) - added by John Palmieri 11 years ago.
12647_sagerc.patch (2.1 KB) - added by Jeroen Demeyer 11 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 11 years ago by Jeroen Demeyer

Authors: Jeroen Demeyer

This still needs a documentation patch.

comment:2 Changed 11 years ago by John Palmieri

Where do you suggest adding documentation? The reference manual (here or here, perhaps)? The top-level README.txt?

Changed 11 years ago by Jeroen Demeyer

Attachment: 12647_sagerc_doc.patch added

comment:3 Changed 11 years ago by Jeroen Demeyer

Description: modified (diff)

comment:4 in reply to:  2 Changed 11 years ago by Jeroen Demeyer

Status: newneeds_review

Replying to jhpalmieri:

Where do you suggest adding documentation? The reference manual (here or here, perhaps)? The top-level README.txt?

I created a new file in the reference manual mentioning sagerc and init.sage.

comment:5 Changed 11 years ago by John Palmieri

Description: modified (diff)
Reviewers: John Palmieri

In a line like

if [ "$SAGE_RC_FILE" = "" ]; then

is there any advantage to replacing the test with [ -z "$SAGE_RC_FILE" ]? (I guess you did it the way you did for consistency with other parts of the script?)

See the review patch for two minor changes to the documentation.

Otherwise, positive review.

Changed 11 years ago by John Palmieri

Attachment: trac_12647-review.patch added

comment:6 Changed 11 years ago by John Palmieri

(The "review patch" needs review.)

comment:7 in reply to:  5 Changed 11 years ago by Jeroen Demeyer

Status: needs_reviewpositive_review

Replying to jhpalmieri:

In a line like

if [ "$SAGE_RC_FILE" = "" ]; then

is there any advantage to replacing the test with [ -z "$SAGE_RC_FILE" ]?

As far as I know, these two tests are identical. But I changed it to use -z.

Review patch is obviously fine.

comment:8 Changed 11 years ago by Leif Leonhardy

How about adding a commented template rc file, with examples for CC, CFLAGS, MAKE (e.g. conditionally, depending on the hostname), SAGE_BROWSER, SAGE_ATLAS_LIB etc.?

Maybe on a follow-up ticket?

(Unfortunately I meanwhile forgot most of what I had in mind... :-/ )

comment:9 Changed 11 years ago by Leif Leonhardy

Minor flaw:

        echo >&2 "Error sourcing $DOT_SAGE/sagerc"

should read

        echo >&2 "Error sourcing $SAGE_RC_FILE"

AFAIK . is more portable than source; does Bash 2.04 support the latter?

comment:10 in reply to:  9 Changed 11 years ago by Jeroen Demeyer

Replying to leif:

AFAIK . is more portable than source; does Bash 2.04 support the latter?

GNU bash, version 2.05b.0(1)-release (powerpc-apple-darwin8.0)
Copyright (C) 2002 Free Software Foundation, Inc.

does support . and source.

comment:11 Changed 11 years ago by Jeroen Demeyer

Setting MAKE in sagerc is a bad idea by the way: currently, sage-env is not sourced by spkg/install which runs the top-level $MAKE. So, setting MAKE=make -j4 in sagerc would only build individual packages in parallel, it would not build multiple packages concurrently.

Fixed the minor flaw, thanks for that.

Changed 11 years ago by Jeroen Demeyer

Attachment: 12647_sagerc.patch added

comment:12 Changed 11 years ago by Punarbasu Purkayastha

In some other ticket there were some concerns raised about the non-portability of x\+ in sed. Can't remember which ticket it was. The portable solutions were either xx* or x\{1,\}.

comment:13 in reply to:  12 ; Changed 11 years ago by Jeroen Demeyer

Replying to ppurka:

In some other ticket there were some concerns raised about the non-portability of x\+ in sed. Can't remember which ticket it was. The portable solutions were either xx* or x\{1,\}.

Presumably, the sed on Cygwin (which is all that matters for this) does support \+. I know the OS X 10.4 sed does not support \+.

comment:14 in reply to:  13 Changed 11 years ago by Leif Leonhardy

Replying to jdemeyer:

Replying to ppurka:

In some other ticket there were some concerns raised about the non-portability of x\+ in sed. Can't remember which ticket it was. The portable solutions were either xx* or x\{1,\}.

Presumably, the sed on Cygwin (which is all that matters for this) does support \+. I know the OS X 10.4 sed does not support \+.

Sun's "default" sed (not the one in /usr/xpg*/bin/) doesn't either IIRC.

Probably even the XPG one doesn't; I don't recall. It at least used to be a GNU extension.

comment:15 Changed 11 years ago by John Palmieri

Regarding the use of sed here, this script is used every time Sage is run, and it hasn't caused any problems on any platform, presumably because (as Jeroen points out) this part only matters on Cygwin. So I don't see any need to change anything.

comment:16 in reply to:  15 Changed 11 years ago by Leif Leonhardy

Replying to jhpalmieri:

Regarding the use of sed here, this script is used every time Sage is run, and it hasn't caused any problems on any platform, presumably because (as Jeroen points out) this part only matters on Cygwin. So I don't see any need to change anything.

Oh, I thought this discussion was unrelated to the ticket anyway... 8)

But now I see it is on topic... In this case, s/WIN.*/WIN/ would work as well by the way, and there shouldn't be two invocations of uname. (And using the shell-builtin pattern matching, case ... in ..., would be faster and safer / more portable anyway.)

But never mind...

comment:17 in reply to:  11 Changed 11 years ago by Leif Leonhardy

Replying to jdemeyer:

Setting MAKE in sagerc is a bad idea by the way: currently, sage-env is not sourced by spkg/install which runs the top-level $MAKE. So, setting MAKE=make -j4 in sagerc would only build individual packages in parallel, it would not build multiple packages concurrently.

I see. I was pretty sure we'd source sage-env (at least) in the build rule (of course on the same line we call spkg/install) of the top-level Makefile ($SAGE_ROOT/Makefile).

We could of course implement ./sage --make ... :-)

(But perhaps the easiest way is to make make an alias to a script that behaves differently in case the current directory is the root of a Sage installation, and just calls make otherwise.)

comment:18 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-5.0.beta9
Resolution: fixed
Status: positive_reviewclosed

comment:19 Changed 10 years ago by Leif Leonhardy

The patch to sage-env causes a beta9 regression since CFLAGS are now no longer exported, which at least some spkgs relied on (i.e., their spkg-install sets or modifies them, but doesn't export them by itself since this used to be redundant).

comment:20 in reply to:  19 ; Changed 10 years ago by John Palmieri

Replying to leif:

The patch to sage-env causes a beta9 regression since CFLAGS are now no longer exported, which at least some spkgs relied on (i.e., their spkg-install sets or modifies them, but doesn't export them by itself since this used to be redundant).

Can you explain that? I thought that first sage-env was run, then spkg-install was run (via sage-spkg). So how can exporting (an empty) CFLAGS in sage-env help if that variable is later modified in spkg-install?

comment:21 in reply to:  19 Changed 10 years ago by Leif Leonhardy

Replying to leif:

The patch to sage-env causes a beta9 regression since CFLAGS are now no longer exported, which at least some spkgs relied on (i.e., their spkg-install sets or modifies them, but doesn't export them by itself since this used to be redundant).

... or probably not (it's late).

Actually any spkg-install modifying CFLAGS should export them (at least if they've been empty / unset) ...

comment:22 in reply to:  20 Changed 10 years ago by Leif Leonhardy

Replying to jhpalmieri:

Replying to leif:

The patch to sage-env causes a beta9 regression since CFLAGS are now no longer exported, which at least some spkgs relied on (i.e., their spkg-install sets or modifies them, but doesn't export them by itself since this used to be redundant).

Can you explain that? I thought that first sage-env was run, then spkg-install was run (via sage-spkg). So how can exporting (an empty) CFLAGS in sage-env help if that variable is later modified in spkg-install?

It would have helped only if CFLAGS were (e.g.) set to "" as well (if they had been empty or unset before), which on the other hand would currently break building the Sage library, which needs -fno-strict-aliasing, which [also] comes from Python's CFLAGS, but only if CFLAGS are not set to "", i.e., not set / exported at all.

comment:23 Changed 10 years ago by Jeroen Demeyer

Exporting a variable which you don't set has no effect. Since CFLAGS wasn't (and still isn't) set in sage-env, the export did nothing.

Example (GNU bash version 4.0.35(1)-release if it matters):

jdemeyer@arcanis:~$ ( export FOO; env ) | grep FOO
jdemeyer@arcanis:~$ ( export FOO; FOO=; env ) | grep FOO
FOO=

comment:24 in reply to:  23 Changed 10 years ago by Leif Leonhardy

Replying to jdemeyer:

Exporting a variable which you don't set has no effect. Since CFLAGS wasn't (and still isn't) set in sage-env, the export did nothing.

Example (GNU bash version 4.0.35(1)-release if it matters):

jdemeyer@arcanis:~$ ( export FOO; env ) | grep FOO
jdemeyer@arcanis:~$ ( export FOO; FOO=; env ) | grep FOO
FOO=

Of course. But we [still] have things like

if [ "$LDFLAGS" = "" ]; then
    LDFLAGS=""          && export LDFLAGS
fi


if [ "$CXXFLAGS" = "" ]; then
    export CXXFLAGS="$CFLAGS"
fi

in sage-env, which will always cause LDFLAGS and CXXFLAGS to be exported, no matter whether they've previously been unset, or exported empty. So currently (re)exporting these in e.g. spkg-install scripts is indeed redundant.

I think we should either do that there for all such variables, or none of them. Especially setting CXXFLAGS to $CFLAGS even if a user did export CXXFLAGS="" may have somewhat surprising effects, or is (I think) at least unexpected behaviour.

Note: See TracTickets for help on using tickets.