Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#12647 closed enhancement (fixed)

Add support for a "sagerc" script

Reported by: jdemeyer Owned by: leif
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:

Description (last modified by jhpalmieri)

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 jdemeyer 7 years ago.
trac_12647-review.patch (1.2 KB) - added by jhpalmieri 7 years ago.
12647_sagerc.patch (2.1 KB) - added by jdemeyer 7 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 7 years ago by jdemeyer

  • Authors set to Jeroen Demeyer

This still needs a documentation patch.

comment:2 follow-up: Changed 7 years ago by jhpalmieri

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

Changed 7 years ago by jdemeyer

comment:3 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:4 in reply to: ↑ 2 Changed 7 years ago by jdemeyer

  • Status changed from new to needs_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 follow-up: Changed 7 years ago by jhpalmieri

  • Description modified (diff)
  • Reviewers set to 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 7 years ago by jhpalmieri

comment:6 Changed 7 years ago by jhpalmieri

(The "review patch" needs review.)

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

  • Status changed from needs_review to positive_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 7 years ago by leif

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 follow-up: Changed 7 years ago by leif

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 7 years ago by jdemeyer

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 follow-up: Changed 7 years ago by 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.

Fixed the minor flaw, thanks for that.

Changed 7 years ago by jdemeyer

comment:12 follow-up: Changed 7 years ago by 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,\}.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 7 years ago by 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 \+.

comment:14 in reply to: ↑ 13 Changed 7 years ago by leif

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 follow-up: Changed 7 years ago by 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.

comment:16 in reply to: ↑ 15 Changed 7 years ago by leif

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 7 years ago by leif

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 7 years ago by jdemeyer

  • Merged in set to sage-5.0.beta9
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:19 follow-ups: Changed 7 years ago by 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).

comment:20 in reply to: ↑ 19 ; follow-up: Changed 7 years ago by 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?

comment:21 in reply to: ↑ 19 Changed 7 years ago by leif

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 7 years ago by leif

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 follow-up: Changed 7 years ago by 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=

comment:24 in reply to: ↑ 23 Changed 7 years ago by leif

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.