Opened 2 years ago
Closed 2 years ago
#30128 closed defect (fixed)
Replace bashism in src/bin/sageenv
Reported by:  Michael Orlitzky  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  build  Keywords:  
Cc:  John Palmieri  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 2 years ago by
comment:2 Changed 2 years 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 2 years ago by
Ah ok, this is the code for script packages? I want to revise that anyway.
Is there another place?
comment:4 Changed 2 years ago by
comment:5 Changed 2 years 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 2 years 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 2 years 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 followup: 9 Changed 2 years 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 Changed 2 years 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 2 years 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 2 years ago by
I would prefer sourcing from src/bin/
, not build/pkgs/sage_conf/...
, but it makes no factual difference.
comment:12 Changed 2 years 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 2 years ago by
Authors:  → Michael Orlitzky 

Branch:  → u/mjo/ticket/30128 
Commit:  → 624e32674aef4947aa36d8c5d8051a7c6816fd62 
Status:  new → 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:15 followup: 16 Changed 2 years ago by
sagespkg
installs uninstall scripts for some packages in $SAGE_LOCAL/
.
Are these handled correctly?
comment:16 Changed 2 years 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 2 years 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 2 years ago by
Definitely the prerm and postrm scripts are installed in $SAGE_LOCAL/var/lib/sage/scripts
.
comment:19 Changed 2 years ago by
Commit:  624e32674aef4947aa36d8c5d8051a7c6816fd62 → 554282ac3c38667cf0e83488872aa488467de1f2 

comment:20 Changed 2 years 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 2 years 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:23 Changed 2 years ago by
Cc:  John Palmieri added 

Reviewers:  → Matthias Koeppe 
comment:24 followup: 26 Changed 2 years 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 2 years ago by
Status:  needs_review → positive_review 

comment:26 Changed 2 years 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 2 years ago by
Branch:  u/mjo/ticket/30128 → 554282ac3c38667cf0e83488872aa488467de1f2 

Resolution:  → fixed 
Status:  positive_review → closed 
Which scripts runs sageenv under /bin/sh? It's clearly documented that it's to be run in bash