Opened 22 months ago
Closed 22 months ago
#30128 closed defect (fixed)
Replace bashism in src/bin/sageenv
Reported by:  mjo  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  build  Keywords:  
Cc:  jhpalmieri  Merged in:  
Authors:  Michael Orlitzky  Reviewers:  Matthias Koeppe 
Report Upstream:  N/A  Work issues:  
Branch:  554282a (Commits, GitHub, GitLab)  Commit:  554282ac3c38667cf0e83488872aa488467de1f2 
Dependencies:  Stopgaps: 
Description
The sageenv script is run under /bin/sh
but contains bashisms:
SAGE_SCRIPTS_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
This causes a build failure when /bin/sh
is not bash:
cd '/home/mjo/src/sage.git/build/pkgs/sage_conf' && . '/home/mjo/src/sage.git/src/bin/sageenv' && . '/home/mjo/src/sage.git/build/bin/sagebuildenvconfig' && sagelogger p '/home/mjo/src/sage.git/build/pkgs/sage_conf/spkginstall' '/home/mjo/src/sage.git/logs/pkgs/sage_confnone.log' /bin/sh: 123: /home/mjo/src/sage.git/src/bin/sageenv: Bad substitution Error: SAGE_SCRIPTS_DIR is set to a bad value: SAGE_SCRIPTS_DIR=/home/mjo/src/sage.git/build/pkgs/sage_conf You must correct it or erase it and rerun this script make[3]: *** [Makefile:2022: /home/mjo/src/sage.git/local/var/lib/sage/installed/sage_confnone] Error 1
The comment at the top of sageenv about using bash features should be removed afterwards. For bonus points, it would be nice if we could add a nonbash shell to one of the CI runs.
Change History (27)
comment:1 Changed 22 months ago by
comment:2 Changed 22 months ago by
That documentation was wrong after #29345, I just never noticed the comment to remove it.
The "make" process itself is run with /bin/sh
and attempts to source sageenv,
$$(INST)/$(1)$(2): $(3) $(AM_V_at)cd '$$(SAGE_ROOT)/build/pkgs/$(1)' && \ . '$$(SAGE_ROOT)/src/bin/sageenv' && . '$$(SAGE_ROOT)/build/bin/sagebuildenvconfig' && \ sagelogger p '$$(SAGE_ROOT)/build/pkgs/$(1)/spkginstall' '$$(SAGE_LOGS)/$(1)$(2).log' touch "$$@"
in build/make/Makefile.in
.
comment:3 followup: ↓ 4 Changed 22 months ago by
Ah ok, this is the code for script packages? I want to revise that anyway.
Is there another place?
comment:4 in reply to: ↑ 3 Changed 22 months ago by
comment:5 Changed 22 months ago by
A quick fix is just to set SAGE_SCRIPTS_DIR=$(SAGE_ROOT)/src/bin
before sourcing sageenv
. The bash feature is only needed for locating this directory.
comment:6 Changed 22 months ago by
I think I may be able to solve this pretty easily by changing all instances of . src/bin/sageenv
to . src/bin/sageenvconfig && . src/bin/sageenv
, and then requiring sageenvconfig to always be sourced before sageenv (sageenv can die if SAGE_ENV_CONFIG_SOURCED neq 1
).
sageenv really shouldn't know about sageenvconfig at all, the split between the two was made at another level of abstraction. But practically, whoever is sourcing sageenv also knows the path to sageenvconfig, and sageenvconfig knows SAGE_ROOT
.
comment:7 followup: ↓ 8 Changed 22 months ago by
Great point, please go ahead.
Note there's another related ticket  #29951 (src/bin/sageenv
: Make sageenvconfig
and SAGE_LOCAL
optional). Perhaps it makes sense to do this at the same time?
comment:8 in reply to: ↑ 7 ; followup: ↓ 9 Changed 22 months ago by
Replying to mkoeppe:
Perhaps it makes sense to do this at the same time?
If for no other reason than because I will temporarily understand how all of this works. I think I have the bashism thing fixed, waiting for a full rebuild of the distribution to be sure. After that #29951 looks pretty easy. Then maybe #29850... but trying to source sageenvconfig
so that we can install sage_conf is going to cause a chickenandegg problem there.
comment:9 in reply to: ↑ 8 Changed 22 months ago by
Replying to mjo:
Then maybe #29850... but trying to source
sageenvconfig
so that we can install sage_conf is going to cause a chickenandegg problem there.
I don't think this is a problem because at build time I think (hope) you are sourcing src/bin/sageenvconfig
, not $SAGE_LOCAL/bin/sageenvconfig
.
comment:10 Changed 22 months ago by
Ok, so we can just source it from build/pkgs/sage_conf/...
instead. That feels wrong, but objectively it's no worse than pulling it from src/bin
.
comment:11 Changed 22 months ago by
I would prefer sourcing from src/bin/
, not build/pkgs/sage_conf/...
, but it makes no factual difference.
comment:12 Changed 22 months ago by
Ultimately I am hoping that we can remove the dependence of the build on src/bin/sageenv
. But not on this ticket... See #21707.
comment:13 Changed 22 months ago by
 Branch set to u/mjo/ticket/30128
 Commit set to 624e32674aef4947aa36d8c5d8051a7c6816fd62
 Status changed from new to needs_review
This seems to work:
 The build finished with
/bin/sh > /bin/dash
.  I can run
~/src/sage.git/local/bin/sage
from my home directory afterwards and it works.
New commits:
b8a2dcc  Trac #30128: always source sageenvconfig before sageenv.

624e326  Trac #30128: enforce sourcing of sageenvconfig before src/bin/sageenv.

comment:14 Changed 22 months ago by
Looking great. I'll try it out later today
comment:15 followup: ↓ 16 Changed 22 months ago by
sagespkg
installs uninstall scripts for some packages in $SAGE_LOCAL/
.
Are these handled correctly?
comment:16 in reply to: ↑ 15 Changed 22 months ago by
Replying to mkoeppe:
sagespkg
installs uninstall scripts for some packages in$SAGE_LOCAL/
. Are these handled correctly?
I dunno. Do you know the name of one of those packages? I don't have anything uninstallrelated in $SAGE_LOCAL
that I can see after a "default" install (with many system packages used).
comment:17 Changed 22 months ago by
$ ls 1 build/pkgs/*/spkg*leg* build/pkgs/*/spkg*rm* build/pkgs/boost_cropped/spkglegacyuninstall.in build/pkgs/brial/spkglegacyuninstall.in build/pkgs/cunningham_tables/spkglegacyuninstall.in build/pkgs/ecl/spkglegacyuninstall.in build/pkgs/gap/spkglegacyuninstall.in build/pkgs/gap/spkgprerm.in build/pkgs/gcc/spkgpostrm.in build/pkgs/gfan/spkglegacyuninstall.in build/pkgs/gfortran/spkgpostrm.in build/pkgs/gsl/spkglegacyuninstall.in build/pkgs/jmol/spkglegacyuninstall.in build/pkgs/lcalc/spkglegacyuninstall.in build/pkgs/libgd/spkglegacyuninstall.in build/pkgs/mpfrcx/spkglegacyuninstall.in build/pkgs/numpy/spkglegacyuninstall.in build/pkgs/pkgconf/spkgpostrm.in build/pkgs/polymake/spkglegacyuninstall.in build/pkgs/polytopes_db_4d/spkglegacyuninstall.in build/pkgs/pplpy/spkgpostrm.in build/pkgs/r/spkglegacyuninstall.in build/pkgs/sage_brial/spkglegacyuninstall.in
comment:18 Changed 22 months ago by
Definitely the prerm and postrm scripts are installed in $SAGE_LOCAL/var/lib/sage/scripts
.
comment:19 Changed 22 months ago by
 Commit changed from 624e32674aef4947aa36d8c5d8051a7c6816fd62 to 554282ac3c38667cf0e83488872aa488467de1f2
comment:20 Changed 22 months ago by
I did have to fix the prerm/postrm script generator, I had missed one sourcing of sageenv in there. The only two packages that had those scripts installed were gap and pplpy, both of which can be uninstalled afterwards:
$ make pplpyclean ... *********************************************** make[1]: Entering directory '/home/mjo/src/sage.git/build/make' sagespkguninstall pplpy '/home/mjo/src/sage.git/local' Uninstalling existing 'pplpy' Running postuninstall script for 'pplpy' Remove /home/mjo/src/sage.git/local/share/doc/pplpy directory. Removing stamp file '/home/mjo/src/sage.git/local/var/lib/sage/installed/pplpy0.8.4' make[1]: Leaving directory '/home/mjo/src/sage.git/build/make'
comment:21 Changed 22 months ago by
The thing is that these scripts are installed with the intention that if you switch the source tree to a different version, the uninstall will still be done with the script that installed that version.
comment:22 Changed 22 months ago by
Ah OK  I see that you have taken care of my concern already.
comment:23 Changed 22 months ago by
 Cc jhpalmieri added
 Reviewers set to Matthias Koeppe
comment:24 followup: ↓ 26 Changed 22 months ago by
That test for a "broken gcc" in $SAGE_LOCAL does not look right (before or after this ticket). If the source tree is clean (so no src/bin/sageenvconfig
) but $SAGE_LOCAL is already populated (with a $SAGE_LOCAL/bin/sage
and a "broken gcc"), it will fail to detect the "broken gcc" situation.
comment:25 Changed 22 months ago by
 Status changed from needs_review to positive_review
comment:26 in reply to: ↑ 24 Changed 22 months ago by
Replying to mkoeppe:
That test for a "broken gcc" in $SAGE_LOCAL does not look right (before or after this ticket). If the source tree is clean (so no
src/bin/sageenvconfig
) but $SAGE_LOCAL is already populated (with a$SAGE_LOCAL/bin/sage
and a "broken gcc"), it will fail to detect the "broken gcc" situation.
There are many unspeakable things in the older spkgconfigure files. This one, at least, I will eventually be drawn to fix as I git grep bash
and try to eliminate the hits.
comment:27 Changed 22 months ago by
 Branch changed from u/mjo/ticket/30128 to 554282ac3c38667cf0e83488872aa488467de1f2
 Resolution set to fixed
 Status changed from positive_review to closed
Which scripts runs sageenv under /bin/sh? It's clearly documented that it's to be run in bash