Replace bashism in src/bin/sage-env #30128

orlitzky · 2020-07-13T04:11:27Z

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

The text was updated successfully, but these errors were encountered:

mkoeppe · 2020-07-13T04:13:30Z

comment:1

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

orlitzky · 2020-07-13T04:19:32Z

comment:2

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.

mkoeppe · 2020-07-13T04:23:28Z

comment:3

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

Is there another place?

mkoeppe · 2020-07-13T04:35:46Z

comment:4

Replying to @mkoeppe:

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

... in #29386

mkoeppe · 2020-07-13T04:56:00Z

comment:5

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.

orlitzky · 2020-07-13T12:44:44Z

comment:6

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.

mkoeppe · 2020-07-13T14:11:21Z

comment:7

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?

orlitzky · 2020-07-13T16:54:59Z

comment:8

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.

mkoeppe · 2020-07-13T17:10:14Z

comment:9

Replying to @orlitzky:

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.

orlitzky · 2020-07-13T17:51:14Z

comment:10

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.

mkoeppe · 2020-07-13T18:40:25Z

comment:11

I would prefer sourcing from src/bin/, not build/pkgs/sage_conf/..., but it makes no factual difference.

mkoeppe · 2020-07-13T18:41:54Z

comment:12

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.

orlitzky · 2020-07-13T21:28:48Z

Branch: u/mjo/ticket/30128

orlitzky · 2020-07-13T21:28:48Z

Commit: 624e326

orlitzky · 2020-07-13T21:28:48Z

comment:13

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 sage-env-config before sage-env.`
`624e326`	`Trac #30128: enforce sourcing of sage-env-config before src/bin/sage-env.`

orlitzky · 2020-07-13T21:28:48Z

Author: Michael Orlitzky

mkoeppe · 2020-07-13T21:45:09Z

comment:14

Looking great. I'll try it out later today

mkoeppe · 2020-07-13T21:47:31Z

comment:15

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

orlitzky · 2020-07-13T22:12:21Z

comment:16

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).

mkoeppe · 2020-07-13T22:41:25Z

comment:17

$ 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

mkoeppe · 2020-07-13T22:42:30Z

comment:18

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

sagetrac-git · 2020-07-14T00:47:12Z

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

`2f141c8`	`Trac #30128: always source sage-env-config before sage-env.`
`554282a`	`Trac #30128: enforce sourcing of sage-env-config before src/bin/sage-env.`

sagetrac-git · 2020-07-14T00:47:12Z

Changed commit from 624e326 to 554282a

orlitzky · 2020-07-14T00:49:33Z

comment:20

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'

mkoeppe · 2020-07-14T01:43:07Z

comment:21

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.

mkoeppe · 2020-07-14T01:53:23Z

comment:22

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

mkoeppe · 2020-07-14T01:53:46Z

Reviewer: Matthias Koeppe

mkoeppe · 2020-07-14T01:57:24Z

comment:24

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.

orlitzky · 2020-07-15T14:15:02Z

comment:26

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.

vbraun · 2020-07-25T22:50:55Z

Changed branch from u/mjo/ticket/30128 to 554282a

orlitzky added this to the sage-9.2 milestone Jul 13, 2020

orlitzky added c: build labels Jul 13, 2020

orlitzky added the s: needs review label Jul 13, 2020

mkoeppe added s: positive review and removed s: needs review labels Jul 15, 2020

vbraun removed the s: positive review label Jul 25, 2020

vbraun closed this as completed in 0794394 Jul 25, 2020

This was referenced Jul 28, 2020

Use /bin/sh where possible for sage-* scripts #30135

Closed

Add the downward monotonic cone to the cone catalogue #30200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace bashism in src/bin/sage-env #30128

Replace bashism in src/bin/sage-env #30128

orlitzky commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

orlitzky commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

orlitzky commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

orlitzky commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

orlitzky commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

orlitzky commented Jul 13, 2020

orlitzky commented Jul 13, 2020

orlitzky commented Jul 13, 2020

orlitzky commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

orlitzky commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

sagetrac-git mannequin commented Jul 14, 2020

sagetrac-git mannequin commented Jul 14, 2020

orlitzky commented Jul 14, 2020

mkoeppe commented Jul 14, 2020

mkoeppe commented Jul 14, 2020

mkoeppe commented Jul 14, 2020

mkoeppe commented Jul 14, 2020

orlitzky commented Jul 15, 2020

vbraun commented Jul 25, 2020

Replace bashism in src/bin/sage-env #30128

Replace bashism in src/bin/sage-env #30128

Comments

orlitzky commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

orlitzky commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

orlitzky commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

orlitzky commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

orlitzky commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

orlitzky commented Jul 13, 2020

orlitzky commented Jul 13, 2020

orlitzky commented Jul 13, 2020

orlitzky commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

orlitzky commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

mkoeppe commented Jul 13, 2020

sagetrac-git mannequin commented Jul 14, 2020

sagetrac-git mannequin commented Jul 14, 2020

orlitzky commented Jul 14, 2020

mkoeppe commented Jul 14, 2020

mkoeppe commented Jul 14, 2020

mkoeppe commented Jul 14, 2020

mkoeppe commented Jul 14, 2020

orlitzky commented Jul 15, 2020

vbraun commented Jul 25, 2020