Make setup-env.sh compatible with dash

[tgamblin]

On the latest Ubuntu and Debian, and on Arch, the default system shell is dash, and there are systems out there that don't have bash. See Arch and Ubuntu's docs on this.

If you run as Spack in those environments, you get this:

+ export SPACK_ROOT=/spack
+ . /spack/share/spack/setup-env.sh
+ function spack {
/bin/sh: 59: /spack/share/spack/setup-env.sh: function: not found
ERROR: Aborting with RETVAL=255

This PR makes setup-env.sh dash-compatible 😄 . And less readable 😞 . I think it is worthwhile to be sh-compatible so that we can be used easily in these more and more popular minimal environments.

• Replaced use of [[ with [
• Replaced function foo { .. } with foo() { .. }
• Wrapped some long lines
• Add magic so that we can find the sourced file even in dash (whoa!)
• Only do the complicated shell checks in one place; test $_sp_shell elsewhere.
• Tests

[adamjstewart]

I agree with making the script more POSIX compatible. I might as well close #3736 then, as it introduces a lot of Bashisms. This checkbashisms script might be useful for testing purposes. We can also add a package for dash for testing.

[trws]

Generally looks like an improvement. Any reason not to swap ${BASH_SOURCE[0]} for the equivalent ${BASH_SOURCE} to get around the evaluation issue?

[mathstuf]

I'd recommend using ShellCheck to check for other bad patterns.

[adamjstewart]

ShellCheck looks useful, but we may also want to include unit tests to make sure PRs don't break dash support. We should also consider making this change to the compiler wrappers as well. This could create a serious speed up of installation times on systems where /bin/sh links to dash.

Accidental change was removed so I'm happy. Haven't yet tested this with dash though.

[alalazo]

This would be a nice one to have for 0.12.0

[trontrytel]

Hi! I'm trying to use spack to install blitz on Ubuntu 18.04. After doing this:

git clone https://github.com/spack/spack.git
. spack/share/spack/setup-env.sh

I get this error:
function spack {
/bin/sh: 42: spack/share/spack/setup-env.sh: function: not found

Would this pull request be a solution to my problem? Is there a chance for it to be merged? Or is there any workaround for me?

Thanks!

[tgamblin]

@trontrytel: I rebased this PR on develop and updated it quite a bit -- see if it works for you now.

[tgamblin]

@citibeth: yes, it's still relevant.

[tgamblin]

Ok, this PR is updated and seems to actually work. In particular:

• removed all bash-isms from setup-env.sh
• I made it so that you do not have to set SPACK_ROOT even in dash.

The second one was rather surprising to me, but after googling around some, it turns out dash keeps the file descriptor of the sourced script open, and you can hack around with lsof to figure out where the script lives. lsof does not come standard in ubuntu, but I used the same trick to find the script in the /proc filesystem, and that seems to work.

The combination of the two techniques seems like it'll work on either stock macos or on most stock Linux containers, if you happen to be running dash in either of those environments. I mostly care about dash working on macos for testing -- the Linux one is where I would like Spack to work seamlessly out of the box.

We probably need to add some tests that just source this file in containers and make sure everything is set up properly.

[trontrytel]

@trontrytel: I rebased this PR on develop and updated it quite a bit -- see if it works for you now.

Hi @tgamblin. Thanks for looking into this. It works now (tested in vagrant on ubuntu 18.04)

For some reason when doing the same in vagrant while building a singularity image I got a lot of prints from spack (see below). But it seems to be working.

+ git clone https://github.com/spack/spack
Cloning into 'spack'...
remote: Enumerating objects: 49, done.
remote: Counting objects: 100% (49/49), done.
remote: Compressing objects: 100% (40/40), done.
remote: Total 157672 (delta 22), reused 28 (delta 3), pack-reused 157623
Receiving objects: 100% (157672/157672), 54.83 MiB | 10.89 MiB/s, done.
Resolving deltas: 100% (72043/72043), done.
+ cd spack
+ git checkout features/sh-commpatible-setup-env
Branch 'features/sh-commpatible-setup-env' set up to track remote branch 'features/sh-commpatible-setup-env' from 'origin'.
Switched to a new branch 'features/sh-commpatible-setup-env'
+ . share/spack/setup-env.sh
+ _spack_determine_shell
+ [ -n  ]
+ [ -n  ]
+ xargs basename
+ sed s/^-//
+ awk {print $4}
+ tail -n 1
+ PS_FORMAT= ps -p 28789
+ export SPACK_SHELL=sh
+ [ sh = bash ]
+ [ sh = bash ]
+ [ sh = zsh ]
+ tail -1
+ sort -n
+ ls /proc/28789/fd
+ _sp_source_file_fd=10
+ readlink /proc/28789/fd/10
+ _sp_source_file=/spack/share/spack/setup-env.sh
+ [ ! -f /spack/share/spack/setup-env.sh ]
+ dirname /spack/share/spack/setup-env.sh
+ cd /spack/share/spack
+ pwd
+ _sp_share_dir=/spack/share/spack
+ dirname /spack/share/spack
+ dirname /spack/share
+ cd /spack
+ pwd
+ _sp_prefix=/spack
+ [ -x /spack/bin/spack ]
+ export SPACK_ROOT=/spack
+ _spack_pathadd PATH /spack/bin
+ _pa_varname=PATH
+ _pa_new_path=PATH
+ [ -n /spack/bin ]
+ _pa_varname=PATH
+ _pa_new_path=/spack/bin
+ eval _pa_oldvalue=${PATH:-}
+ _pa_oldvalue=/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin
+ _pa_canonical=:/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin:
+ [ -d /spack/bin ]
+ [ :/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin: != *:/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin: ]
+ [ -n /bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin ]
+ eval export PATH="/spack/bin:/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin"
+ export PATH=/spack/bin:/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/sbin
+ need_module=no
+ _spack_fn_exists use
+ grep -q function
+ LANG= type use
+ _spack_fn_exists module
+ grep -q function
+ LANG= type module
+ need_module=yes
+ [ yes = yes ]
+ spack --print-shell-vars sh,modules
+ [ -n  ]
+ _sp_flags=
+ [ -print-shell-vars != --print-shell-vars ]
+ _sp_flags= --print-shell-vars
+ shift
+ [ sh,modules != sh,modules ]
+ [ -n  --print-shell-vars ]
+ [ ell-vars !=  --print-shell-vars ]
+ command spack --print-shell-vars sh,modules
+ return
+ eval _sp_sys_type='linux-ubuntu18.04-x86_64' _sp_tcl_roots='/spack/share/spack/modules' _sp_lmod_roots='/spack/share/spack/lmod' _sp_dotkit_roots='/spack/share/spack/dotkit' _sp_module_prefix='not_installed'
+ _sp_sys_type=linux-ubuntu18.04-x86_64 _sp_tcl_roots=/spack/share/spack/modules _sp_lmod_roots=/spack/share/spack/lmod _sp_dotkit_roots=/spack/share/spack/dotkit _sp_module_prefix=not_installed
+ [ not_installed != not_installed ]
+ _sp_multi_pathadd MODULEPATH /spack/share/spack/modules
+ local IFS=:
+ [ sh = zsh ]
+ _spack_pathadd MODULEPATH /spack/share/spack/modules/linux-ubuntu18.04-x86_64
+ _pa_varname=PATH
+ _pa_new_path=MODULEPATH
+ [ -n /spack/share/spack/modules/linux-ubuntu18.04-x86_64 ]
+ _pa_varname=MODULEPATH
+ _pa_new_path=/spack/share/spack/modules/linux-ubuntu18.04-x86_64
+ eval _pa_oldvalue=${MODULEPATH:-}
+ _pa_oldvalue=
+ _pa_canonical=::
+ [ -d /spack/share/spack/modules/linux-ubuntu18.04-x86_64 ]
+ _sp_multi_pathadd DK_NODE /spack/share/spack/dotkit
+ local IFS=:
+ [ sh = zsh ]
+ _spack_pathadd DK_NODE /spack/share/spack/dotkit/linux-ubuntu18.04-x86_64
+ _pa_varname=PATH
+ _pa_new_path=DK_NODE
+ [ -n /spack/share/spack/dotkit/linux-ubuntu18.04-x86_64 ]
+ _pa_varname=DK_NODE
+ _pa_new_path=/spack/share/spack/dotkit/linux-ubuntu18.04-x86_64
+ eval _pa_oldvalue=${DK_NODE:-}
+ _pa_oldvalue=
+ _pa_canonical=::
+ [ -d /spack/share/spack/dotkit/linux-ubuntu18.04-x86_64 ]
+ [ sh = bash ]
+ cd ../
+ spack install blitz
+ [ -n  ]
+ _sp_flags=
+ [ install != install ]
+ [ -n  ]
+ [  !=  ]
+ _sp_subcommand=
+ [ -n install ]
+ _sp_subcommand=install
+ shift
+ command spack install blitz
==> Installing blitz
==> Searching for binary cache of blitz
==> Warning: No Spack mirrors are currently configured
==> No binary for blitz found: installing from source
==> Fetching https://github.com/blitzpp/blitz/archive/1.0.1.tar.gz
     -#O=-#   #     #                                                         
==> Staging archive: /spack/var/spack/stage/blitz-1.0.1-ygzugsujglfbuugxrmyhewbuv6ythbbz/1.0.1.tar.gz
==> Created stage in /spack/var/spack/stage/blitz-1.0.1-ygzugsujglfbuugxrmyhewbuv6ythbbz
==> No patches needed for blitz
==> Building blitz [AutotoolsPackage]
==> Executing phase: 'autoreconf'
==> Executing phase: 'configure'
==> Executing phase: 'build'
==> Executing phase: 'install'
==> Successfully installed blitz

[tgamblin]

@trontrytel: Awesome!

It looks like vagrant might set -x in the shell before sourcing -- does it? Or does your vagrant script have a shebang line like #!/bin/sh -x? If so then that would explain the output. It looks like it is printing, e.g., cd spack and git checkout features/sh-commpatible-setup-env even before the setup script is sourced.

[scheibelp]

All minor requests:

• 2 requests about comments
• 1 typo

checkbashisms doesn't report any bash-specific syntax (that is not guarded by a bash-shell check). Running the tests with sh would be ideal except that it apparently usually links to bash (or in my Ubuntu docker instance to dash). dash is arguably bare-bones enough that running the tests with it is sufficient to establish sh-compatibility.

[adamjstewart]

I thought $(...) was also a Bash-ism, but I see a lot of that in our scripts. Shouldn't we use `...`?

[scheibelp]

check-bashisms from #4048 (review) did not complain about it at least. One of the reasons I decided to do that is because it can be difficult to determine what is actually bash-specific. I started doing searches for various syntax instances which looked bash-specific and it was difficult to figure out (e.g. something supported on sh but the bash scripting guide would be the top hit).

IMO the best way to proceed is to change something if someone reports an issue with it when running with sh. As these issues come up it might make sense to run other syntax checkers in addition to check-bashisms.

[trws]

I thought $(...) was also a Bash-ism, but I see a lot of that in our scripts. Shouldn't we use `...`?

That’s a common misconception, that I used to hold as well actually, “$(...)” has been part of shell scripting since posix 1, so short of dealing with pre-posix sh it should be fine.

[tgamblin]

I thought $(...) was also a Bash-ism, but I see a lot of that in our scripts. Shouldn't we use ...?

What @trws said -- it's standard POSIX. dash is the main sh implementation I'm concerned about here, and it implements (rather strictly) the POSIX standard.

IMO the best way to proceed is to change something if someone reports an issue with it when running with sh. As these issues come up it might make sense to run other syntax checkers in addition to check-bashisms.

I think these are good ideas, but note also that the tests introduced here actually run setup-env.sh with dash. I don't think there is a better way to check for POSIX compliance :).

[adamjstewart]

Yeah, I'm glad dash is so strict, makes testing really easy.