Opened 10 years ago
Closed 6 years ago
#11687 closed defect (fixed)
Sanitize `sageenv`
Reported by:  leif  Owned by:  leif 

Priority:  minor  Milestone:  sage6.8 
Component:  scripts  Keywords:  BUILD sagespkg pkgconfig .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: 
Description (last modified by )
There are currently a couple of issues with sageenv
; approximately ordered by importance:
 The definition of
UNAME
(currently on line 458) has to be moved up, preferably right beforePATH
etc. get set / modified (currently around line 135), and$UNAME
should be used consistently.  We should save the original, unmodified
PATH
(toSAGE_ORIG_PATH
), analoguous toSAGE_ORIG_LD_LIBRARY_PATH
etc., and perhaps record this inSAGE_ORIG_PATH_SET
. This should also be done close to the top, of course before modifyingPATH
. (#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 forSAGE64
 The default for
CFLAG64
(m64
) and similar should be set insageenv
, not each and everyspkginstall
.
We should IMHO also save the original settings of CC
, CFLAGS
, LD
, LDFLAGS
, MAKE
etc., since it is otherwise impossible for scripts (like the spkginstall
s) 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 systemwide 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 buildrelated files.
Some of the rather minor issues can be fixed on their own / followup tickets, but it would be odd to have a dozen "concurring" tickets all modifying sageenv
at the same time.
Change History (25)
comment:1 Changed 10 years ago by
comment:2 Changed 10 years ago by
 Cc kini added
comment:3 Changed 10 years ago by
 Description modified (diff)
comment:4 followup: ↓ 6 Changed 10 years ago by
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
 Description modified (diff)
comment:6 in reply to: ↑ 4 ; followup: ↓ 7 Changed 10 years ago by
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 sageenv
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
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 pkgconfig
related code there (to sagespkg
, right before sagemake_relative
is called).
comment:8 in reply to: ↑ description Changed 10 years ago by
 Description modified (diff)
comment:9 followup: ↓ 10 Changed 10 years ago by
 Description modified (diff)
Removed some points which is no longer relevant.
comment:10 in reply to: ↑ 9 Changed 10 years ago by
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
 Priority changed from critical to minor
comment:12 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:13 Changed 8 years ago by
 Description modified (diff)
comment:14 Changed 8 years ago by
 Description modified (diff)
comment:15 Changed 8 years ago by
 Description modified (diff)
comment:16 Changed 8 years ago by
See #15652 for the fnostrictaliasing
issue.
comment:17 Changed 8 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:18 Changed 8 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:19 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:20 Changed 7 years ago by
 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:
2dd3b0e  trac #11687 proposal

comment:21 Changed 7 years ago by
 Commit changed from 2dd3b0eb2fb9d5a70b319389d01e8506387d3381 to f7f27ebe7d0648c1df1f6c6bc059d80f95b0b5c5
Branch pushed to git repo; I updated commit sha1. New commits:
f7f27eb  trac #11687 typo

comment:22 Changed 7 years ago by
 Commit changed from f7f27ebe7d0648c1df1f6c6bc059d80f95b0b5c5 to 12864975d421ecbe63a536725478eadb53fb7565
Branch pushed to git repo; I updated commit sha1. New commits:
1286497  Merge branch 'u/chapoton/11687' into 6.5.b2

comment:23 Changed 6 years ago by
 Milestone changed from sage6.4 to sage6.8
comment:24 Changed 6 years ago by
 Reviewers set to Jeroen Demeyer
 Status changed from needs_review to positive_review
comment:25 Changed 6 years ago by
 Branch changed from u/chapoton/11687 to 12864975d421ecbe63a536725478eadb53fb7565
 Resolution set to fixed
 Status changed from positive_review to closed
We could also define (and export)
PKG_CONFIG_TOP_BUILD_DIR
, namely to${SAGE_ROOT}/local
, and useor, a bit more flexible, define it to just
${SAGE_ROOT}
and usein all
pkgconfig
(*.pc
) files, which avoids wrappingpkgconfig
just to adddefinevariable=SAGE_ROOT="${SAGE_ROOT}"
to each invocation.This is even safer than using
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.)