Opened 10 years ago

Closed 6 years ago

#11687 closed defect (fixed)

Sanitize `sage-env`

Reported by: leif Owned by: leif
Priority: minor Milestone: sage-6.8
Component: scripts Keywords: BUILD sage-spkg pkg-config .pc pkgconfig PKG_CONFIG_PATH SAGE_PATH Cygwin environment variables
Cc: kini Merged in:
Authors: Frédéric Chapoton Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 1286497 (Commits, GitHub, GitLab) Commit: 12864975d421ecbe63a536725478eadb53fb7565
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

There are currently a couple of issues with sage-env; approximately ordered by importance:

  • The definition of UNAME (currently on line 458) has to be moved up, preferably right before PATH etc. get set / modified (currently around line 135), and $UNAME should be used consistently.
  • We should save the original, unmodified PATH (to SAGE_ORIG_PATH), analoguous to SAGE_ORIG_LD_LIBRARY_PATH etc., and perhaps record this in SAGE_ORIG_PATH_SET. This should also be done close to the top, of course before modifying PATH. (#10202 also suggests / introduces this.)
  • There's partially useless code like
    if [ "$LDFLAGS" = "" ]; then
        LDFLAGS=""          && export LDFLAGS
    fi
    
  • All variables that are expected to be either "yes", "no" or empty / unset should be checked for [in]valid settings, and perhaps normalized (i.e., for example set to "no" in case they're empty or not set).
    We currently only do this for SAGE64
  • The default for CFLAG64 (-m64) and similar should be set in sage-env, not each and every spkg-install.

We should IMHO also save the original settings of CC, CFLAGS, LD, LDFLAGS, MAKE etc., since it is otherwise impossible for scripts (like the spkg-installs) to determine whether or which parts of the current settings were made by Sage, and which originate from the user (intentionally or not, e.g. from system-wide or default .*rc files).

A better approach would be to have and use SAGE_CC etc. instead, but this would require changes to all spkgs and other build-related files.


Some of the rather minor issues can be fixed on their own / follow-up tickets, but it would be odd to have a dozen "concurring" tickets all modifying sage-env at the same time.

Change History (25)

comment:1 Changed 10 years ago by leif

We could also define (and export) PKG_CONFIG_TOP_BUILD_DIR, namely to ${SAGE_ROOT}/local, and use

prefix=${pc_top_builddir}

or, a bit more flexible, define it to just ${SAGE_ROOT} and use

prefix=${pc_top_builddir}/local

in all pkg-config (*.pc) files, which avoids wrapping pkg-config just to add --define-variable=SAGE_ROOT="${SAGE_ROOT}" to each invocation.

This is even safer than using

prefix=$${SAGE_ROOT}/local

though I haven't [yet] encountered problems with the latter. (The $$ has the same meaning as in Makefiles, i.e. escapes the $ such that the resulting environment variable will be interpreted -- or substituted -- later, by the shell.)

comment:2 Changed 10 years ago by kini

  • Cc kini added

comment:3 Changed 10 years ago by leif

  • Description modified (diff)

comment:4 follow-up: Changed 10 years ago by jhpalmieri

Since there is already a lot of progress on #11021, can we finish that one off and have this ticket depend on that one?

comment:5 Changed 10 years ago by leif

  • Description modified (diff)

comment:6 in reply to: ↑ 4 ; follow-up: Changed 10 years ago by leif

Replying to jhpalmieri:

Since there is already a lot of progress on #11021, can we finish that one off and have this ticket depend on that one?

Currently sorting out what to fix where and in which order.

#11021 doesn't change much, and I intended to attach a couple of small patches to sage-env here for independent issues, so that they should all apply in almost random order, with just line numbers changing.

We can then leave out individual patches here in case other tickets already fix things.


I'll hopefully return to #11021 today and [try to] finish it, though I originally planned other things... ;-)

comment:7 in reply to: ↑ 6 Changed 10 years ago by leif

Replying to leif:

Replying to jhpalmieri:

Since there is already a lot of progress on #11021, can we finish that one off and have this ticket depend on that one?

Currently sorting out what to fix where and in which order.

I think I'll also add pkg-config-related code there (to sage-spkg, right before sage-make_relative is called).

comment:8 in reply to: ↑ description Changed 10 years ago by leif

  • Description modified (diff)

Replying to leif:

  • BUILD (used by sage-spkg) isn't exported, which wasn't really a problem until we started to (fully) source sage-env only once (#10469, cf. #11021 and #4949 for the impact on sage-spkg).

#11765 adds a note on the necessity to always export variables in sage-env.


  • PKG_CONFIG_PATH is only changed if it is empty / undefined. We have to prepend $SAGE_ROOT/local/lib/pkgconfig in case it is not. (Cf. #11681, #11686; #10202).

This is now fixed by a small patch at #11765, currently needing review.

comment:9 follow-up: Changed 10 years ago by jdemeyer

  • Description modified (diff)

Removed some points which is no longer relevant.

comment:10 in reply to: ↑ 9 Changed 10 years ago by leif

Replying to jdemeyer:

Removed some points which is no longer relevant.

I guess you meant "are". ;-)

Progress! (Although I'd just add "Fixed by #.....", so one immediately sees where something got fixed and whether the corresponding ticket is already merged, e.g. if a problem reappears.)

comment:11 Changed 9 years ago by jdemeyer

  • Priority changed from critical to minor

comment:12 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:13 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:14 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:15 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:16 Changed 8 years ago by jdemeyer

See #15652 for the -fno-strict-aliasing issue.

comment:17 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:18 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:19 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:20 Changed 7 years ago by chapoton

  • Branch set to u/chapoton/11687
  • Commit set to 2dd3b0eb2fb9d5a70b319389d01e8506387d3381
  • Status changed from new to needs_review

Here is a first proposal, gathering the UNAME setting in one single place.


New commits:

2dd3b0etrac #11687 proposal

comment:21 Changed 7 years ago by git

  • Commit changed from 2dd3b0eb2fb9d5a70b319389d01e8506387d3381 to f7f27ebe7d0648c1df1f6c6bc059d80f95b0b5c5

Branch pushed to git repo; I updated commit sha1. New commits:

f7f27ebtrac #11687 typo

comment:22 Changed 7 years ago by git

  • Commit changed from f7f27ebe7d0648c1df1f6c6bc059d80f95b0b5c5 to 12864975d421ecbe63a536725478eadb53fb7565

Branch pushed to git repo; I updated commit sha1. New commits:

1286497Merge branch 'u/chapoton/11687' into 6.5.b2

comment:23 Changed 6 years ago by chapoton

  • Milestone changed from sage-6.4 to sage-6.8

comment:24 Changed 6 years ago by jdemeyer

  • Authors set to Frédéric Chapoton
  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:25 Changed 6 years ago by vbraun

  • Branch changed from u/chapoton/11687 to 12864975d421ecbe63a536725478eadb53fb7565
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.