Replace bashism in src/bin/sage-env
Closed this issue · 30 comments
The sage-env 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/sage-env' && . '/home/mjo/src/sage.git/build/bin/sage-build-env-config' && sage-logger -p '/home/mjo/src/sage.git/build/pkgs/sage_conf/spkg-install' '/home/mjo/src/sage.git/logs/pkgs/sage_conf-none.log'
/bin/sh: 123: /home/mjo/src/sage.git/src/bin/sage-env: 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_conf-none] Error 1
The comment at the top of sage-env about using bash features should be removed afterwards. For bonus points, it would be nice if we could add a non-bash shell to one of the CI runs.
CC: @jhpalmieri
Component: build
Author: Michael Orlitzky
Branch/Commit: 554282a
Reviewer: Matthias Koeppe
Issue created by migration from https://trac.sagemath.org/ticket/30128
Which scripts runs sage-env under /bin/sh? It's clearly documented that it's to be run in bash
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 sage-env,
$$(INST)/$(1)-$(2): $(3)
$(AM_V_at)cd '$$(SAGE_ROOT)/build/pkgs/$(1)' && \
. '$$(SAGE_ROOT)/src/bin/sage-env' && . '$$(SAGE_ROOT)/build/bin/sage-build-env-config' && \
sage-logger -p '$$(SAGE_ROOT)/build/pkgs/$(1)/spkg-install' '$$(SAGE_LOGS)/$(1)-$(2).log'
touch "$$@"
in build/make/Makefile.in.
Ah ok, this is the code for script packages? I want to revise that anyway.
Is there another place?
A quick fix is just to set SAGE_SCRIPTS_DIR=$(SAGE_ROOT)/src/bin before sourcing sage-env. The bash feature is only needed for locating this directory.
I think I may be able to solve this pretty easily by changing all instances of . src/bin/sage-env to . src/bin/sage-env-config && . src/bin/sage-env, and then requiring sage-env-config to always be sourced before sage-env (sage-env can die if SAGE_ENV_CONFIG_SOURCED -neq 1).
sage-env really shouldn't know about sage-env-config at all, the split between the two was made at another level of abstraction. But practically, whoever is sourcing sage-env also knows the path to sage-env-config, and sage-env-config knows SAGE_ROOT.
Great point, please go ahead.
Note there's another related ticket - #29951 (src/bin/sage-env: Make sage-env-config and SAGE_LOCAL optional). Perhaps it makes sense to do this at the same time?
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 sage-env-config so that we can install sage_conf is going to cause a chicken-and-egg problem there.
Replying to @orlitzky:
Then maybe #29850... but trying to source
sage-env-configso that we can install sage_conf is going to cause a chicken-and-egg problem there.
I don't think this is a problem because at build time I think (hope) you are sourcing src/bin/sage-env-config, not $SAGE_LOCAL/bin/sage-env-config.
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.
I would prefer sourcing from src/bin/, not build/pkgs/sage_conf/..., but it makes no factual difference.
Ultimately I am hoping that we can remove the dependence of the build on src/bin/sage-env. But not on this ticket... See #21707.
Branch: u/mjo/ticket/30128
This seems to work:
- The build finished with
/bin/sh -> /bin/dash. - I can run
~/src/sage.git/local/bin/sagefrom my home directory afterwards and it works.
New commits:
b8a2dcc | Trac #30128: always source sage-env-config before sage-env. |
624e326 | Trac #30128: enforce sourcing of sage-env-config before src/bin/sage-env. |
Author: Michael Orlitzky
Looking great. I'll try it out later today
sage-spkg installs uninstall scripts for some packages in $SAGE_LOCAL/.
Are these handled correctly?
Replying to @mkoeppe:
sage-spkginstalls 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 uninstall-related in $SAGE_LOCAL that I can see after a "default" install (with many system packages used).
$ ls -1 build/pkgs/*/spkg-*leg* build/pkgs/*/spkg-*rm*
build/pkgs/boost_cropped/spkg-legacy-uninstall.in
build/pkgs/brial/spkg-legacy-uninstall.in
build/pkgs/cunningham_tables/spkg-legacy-uninstall.in
build/pkgs/ecl/spkg-legacy-uninstall.in
build/pkgs/gap/spkg-legacy-uninstall.in
build/pkgs/gap/spkg-prerm.in
build/pkgs/gcc/spkg-postrm.in
build/pkgs/gfan/spkg-legacy-uninstall.in
build/pkgs/gfortran/spkg-postrm.in
build/pkgs/gsl/spkg-legacy-uninstall.in
build/pkgs/jmol/spkg-legacy-uninstall.in
build/pkgs/lcalc/spkg-legacy-uninstall.in
build/pkgs/libgd/spkg-legacy-uninstall.in
build/pkgs/mpfrcx/spkg-legacy-uninstall.in
build/pkgs/numpy/spkg-legacy-uninstall.in
build/pkgs/pkgconf/spkg-postrm.in
build/pkgs/polymake/spkg-legacy-uninstall.in
build/pkgs/polytopes_db_4d/spkg-legacy-uninstall.in
build/pkgs/pplpy/spkg-postrm.in
build/pkgs/r/spkg-legacy-uninstall.in
build/pkgs/sage_brial/spkg-legacy-uninstall.in
Definitely the prerm and postrm scripts are installed in $SAGE_LOCAL/var/lib/sage/scripts.
I did have to fix the prerm/postrm script generator, I had missed one sourcing of sage-env in there. The only two packages that had those scripts installed were gap and pplpy, both of which can be uninstalled afterwards:
$ make pplpy-clean
...
***********************************************
make[1]: Entering directory '/home/mjo/src/sage.git/build/make'
sage-spkg-uninstall pplpy '/home/mjo/src/sage.git/local'
Uninstalling existing 'pplpy'
Running post-uninstall 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/pplpy-0.8.4'
make[1]: Leaving directory '/home/mjo/src/sage.git/build/make'
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.
Ah OK - I see that you have taken care of my concern already.
Reviewer: Matthias Koeppe
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/sage-env-config) 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.
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/sage-env-config) but $SAGE_LOCAL is already populated (with a$SAGE_LOCAL/bin/sageand a "broken gcc"), it will fail to detect the "broken gcc" situation.
There are many unspeakable things in the older spkg-configure files. This one, at least, I will eventually be drawn to fix as I git grep bash and try to eliminate the hits.
Changed branch from u/mjo/ticket/30128 to 554282a