Opened 2 years ago

Closed 2 years ago

#30128 closed defect (fixed)

Replace bashism in src/bin/sage-env

Reported by: Michael Orlitzky Owned by:
Priority: major Milestone: sage-9.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:

Status badges

Description

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.

Change History (27)

comment:1 Changed 2 years ago by Matthias Köppe

Which scripts runs sage-env under /bin/sh? It's clearly documented that it's to be run in bash

comment:2 Changed 2 years ago by Michael Orlitzky

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.

comment:3 Changed 2 years ago by Matthias Köppe

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 2 years ago by Matthias Köppe

Replying to mkoeppe:

Ah ok, this is the code for script packages? I want to revise that anyway.

... in #29386

comment:5 Changed 2 years ago by Matthias Köppe

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.

comment:6 Changed 2 years ago by Michael Orlitzky

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.

comment:7 Changed 2 years ago by Matthias Köppe

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?

comment:8 in reply to:  7 ; Changed 2 years ago by Michael Orlitzky

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.

comment:9 in reply to:  8 Changed 2 years ago by Matthias Köppe

Replying to mjo:

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.

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.

comment:10 Changed 2 years ago by Michael Orlitzky

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 Matthias Köppe

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 Matthias Köppe

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.

comment:13 Changed 2 years ago by Michael Orlitzky

Authors: Michael Orlitzky
Branch: u/mjo/ticket/30128
Commit: 624e32674aef4947aa36d8c5d8051a7c6816fd62
Status: newneeds_review

This seems to work:

  1. The build finished with /bin/sh -> /bin/dash.
  2. I can run ~/src/sage.git/local/bin/sage from my home directory afterwards and it works.

New commits:

b8a2dccTrac #30128: always source sage-env-config before sage-env.
624e326Trac #30128: enforce sourcing of sage-env-config before src/bin/sage-env.

comment:14 Changed 2 years ago by Matthias Köppe

Looking great. I'll try it out later today

comment:15 Changed 2 years ago by Matthias Köppe

sage-spkg installs uninstall scripts for some packages in $SAGE_LOCAL/. Are these handled correctly?

comment:16 in reply to:  15 Changed 2 years ago by Michael Orlitzky

Replying to mkoeppe:

sage-spkg 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 uninstall-related in $SAGE_LOCAL that I can see after a "default" install (with many system packages used).

comment:17 Changed 2 years ago by Matthias Köppe

$ 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

comment:18 Changed 2 years ago by Matthias Köppe

Definitely the prerm and postrm scripts are installed in $SAGE_LOCAL/var/lib/sage/scripts.

comment:19 Changed 2 years ago by git

Commit: 624e32674aef4947aa36d8c5d8051a7c6816fd62554282ac3c38667cf0e83488872aa488467de1f2

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2f141c8Trac #30128: always source sage-env-config before sage-env.
554282aTrac #30128: enforce sourcing of sage-env-config before src/bin/sage-env.

comment:20 Changed 2 years ago by Michael Orlitzky

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'

comment:21 Changed 2 years ago by Matthias Köppe

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 2 years ago by Matthias Köppe

Ah OK - I see that you have taken care of my concern already.

Last edited 2 years ago by Matthias Köppe (previous) (diff)

comment:23 Changed 2 years ago by Matthias Köppe

Cc: John Palmieri added
Reviewers: Matthias Koeppe

comment:24 Changed 2 years ago by Matthias Köppe

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.

comment:25 Changed 2 years ago by Matthias Köppe

Status: needs_reviewpositive_review

comment:26 in reply to:  24 Changed 2 years ago by Michael Orlitzky

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/sage and 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.

comment:27 Changed 2 years ago by Volker Braun

Branch: u/mjo/ticket/30128554282ac3c38667cf0e83488872aa488467de1f2
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.