/bin/(ba|c)?sh -> /usr/bin/env $1sh #25380
Conversation
spackbot-app
bot
added
commands
documentation
sbang
shell-support
tests
haampie
force-pushed
the
fix/usr/bin/env
branch
from
8687597
to
641ffe8
Compare
haampie
force-pushed
the
fix/usr/bin/env
branch
from
d8e951e
to
0281ff7
Compare
| @@ -1,4 +1,4 @@ | |||
| #!/bin/bash | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
This might be the only place I hesitate to use /usr/bin/env. How much overhead does this result in? Build scripts call cc thousands of times, so if we have to search for bash in the PATH every time that could be slow.
There was a problem hiding this comment.
maybe we should bootstrap a static binary executable to replace the bash script ^^. i was meaning to check what the overhead of the shell wrapper actually is
There was a problem hiding this comment.
This might actually help us on windows -- where there is no bash. If you get numbers on this I'd be interested. But I also worry b/c ultimately not every Spack build will actually require a compiler, so we may not have one handy (unlikely, I know). But sure we could use the bootstrap logic and also provide binaries for something this simple.
On a similar note: we have talked about (for a while) generating compiler wrappers for each build, to avoid the symlink farm currently in lib/spack/env and the lengthy switch statement in lib/spack/env/cc. Then the names of the compilers would be completely up to the classes in lib/spack/spack/compilers, or, ultimately, to the compiler packages (when compilers become normal dependencies).
There was a problem hiding this comment.
So, I kind of like the spirit of this, but what problem does it solve?
This reminds me of how we originally had bin/spack running with #!/usr/bin/env python, then finding a python[23]? from your environment, and then we finally introduced SPACK_PYTHON to nail things down so that broken environments don't break Spack.
IMO this introduces a bunch of potential sources of error -- for little gain. What's the benefit? Why change this now?
| @@ -1,4 +1,4 @@ | |||
| #!/bin/sh | |||
| #!/usr/bin/env sh | |||
There was a problem hiding this comment.
sbang is vendored and while I don't see a reason why this won't work in general -- why do this? Is it just so that we can use a shell built by Spack?
Spack can't bootstrap bash on Alpine. Optimally I'd like to be able to just download spack on any platform, run spack install xyz, and it just works, without having to worry about the system package manager. |
Do you guys need Alpine? (not saying that's bad just curious - are you using it for lots of containers?)
My inclination for this one would be to port Alternately if we do generate a |
It's not super popular, but I've been using it with some success to build small static executables (so, building in a container, but the static executables can be used outside of it too).
Yeah, I think this is sensible, we have to improve |
|
With the attached diff the "only" things to fix are arrays, read, heredocs, and some trivial stuff we can work around: https://github.com/koalaman/shellcheck/wiki/SC3011 Instead of relying on arrays & environment variables & heredocs, we could generate the shell script from a template once ahead of time per package installation? A just-in-time compiler wrapper 😄 that generates explicit diff --git a/lib/spack/env/cc b/lib/spack/env/cc
index 5157a5f381..160c301cc7 100755
--- a/lib/spack/env/cc
+++ b/lib/spack/env/cc
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/bin/sh
#
# Copyright 2013-2021 Lawrence Livermore National Security, LLC and other
# Spack Project Developers. See the top-level COPYRIGHT file for details.
@@ -60,7 +60,7 @@ parameters=(
# die()
# Prints a message and exits with error 1.
-function die {
+die() {
echo "$@"
exit 1
}
@@ -78,10 +78,10 @@ IFS=' ' read -ra SPACK_LDFLAGS <<< "$SPACK_LDFLAGS"
IFS=' ' read -ra SPACK_LDLIBS <<< "$SPACK_LDLIBS"
# test whether a path is a system directory
-function system_dir {
+system_dir() {
path="$1"
for sd in "${SPACK_SYSTEM_DIRS[@]}"; do
- if [ "${path}" == "${sd}" ] || [ "${path}" == "${sd}/" ]; then
+ if [ "${path}" = "${sd}" ] || [ "${path}" = "${sd}/" ]; then
# success if path starts with a system prefix
return 0
fi
@@ -90,27 +90,27 @@ function system_dir {
}
for param in "${parameters[@]}"; do
- if [[ -z ${!param+x} ]]; then
+ if [ -z ${!param+x} ]; then
die "Spack compiler must be run from Spack! Input '$param' is missing."
fi
done
# Check if optional parameters are defined
# If we aren't asking for debug flags, don't add them
-if [[ -z ${SPACK_ADD_DEBUG_FLAGS+x} ]]; then
+if [ -z ${SPACK_ADD_DEBUG_FLAGS+x} ]; then
SPACK_ADD_DEBUG_FLAGS="false"
fi
# SPACK_ADD_DEBUG_FLAGS must be true/false/custom
is_valid="false"
for param in "true" "false" "custom"; do
- if [ "$param" == "$SPACK_ADD_DEBUG_FLAGS" ]; then
+ if [ "$param" = "$SPACK_ADD_DEBUG_FLAGS" ]; then
is_valid="true"
fi
done
# Exit with error if we are given an incorrect value
-if [ "$is_valid" == "false" ]; then
+if [ "$is_valid" = "false" ]; then
die "SPACK_ADD_DEBUG_FLAGS, if defined, must be one of 'true' 'false' or 'custom'"
fi
@@ -174,9 +174,9 @@ esac
# If any of the arguments below are present, then the mode is vcheck.
# In vcheck mode, nothing is added in terms of extra search paths or
# libraries.
-if [[ -z $mode ]] || [[ $mode == ld ]]; then
+if [ -z "$mode" ] || [ "$mode" = ld ]; then
for arg in "$@"; do
- case $arg in
+ case "$arg" in
-v|-V|--version|-dumpversion)
mode=vcheck
break
@@ -186,16 +186,16 @@ if [[ -z $mode ]] || [[ $mode == ld ]]; then
fi
# Finish setting up the mode.
-if [[ -z $mode ]]; then
+if [ -z "$mode" ]; then
mode=ccld
for arg in "$@"; do
- if [[ $arg == -E ]]; then
+ if [ "$arg" = -E ]; then
mode=cpp
break
- elif [[ $arg == -S ]]; then
+ elif [ "$arg" = -S ]; then
mode=as
break
- elif [[ $arg == -c ]]; then
+ elif [ "$arg" = -c ]; then
mode=cc
break
fi
@@ -225,14 +225,14 @@ linker_arg="${SPACK_LINKER_ARG}"
eval rpath=\$SPACK_${comp}_RPATH_ARG
# Dump the mode and exit if the command is dump-mode.
-if [[ $SPACK_TEST_COMMAND == dump-mode ]]; then
+if [ "$SPACK_TEST_COMMAND" = dump-mode ]; then
echo "$mode"
exit
fi
# Check that at least one of the real commands was actually selected,
# otherwise we don't know what to execute.
-if [[ -z $command ]]; then
+if [ -z "$command" ]; then
die "ERROR: Compiler '$SPACK_COMPILER_SPEC' does not support compiling $language programs."
fi
@@ -247,7 +247,7 @@ export PATH=""
for dir in "${env_path[@]}"; do
addpath=true
for env_dir in "${spack_env_dirs[@]}"; do
- if [[ "$dir" == "$env_dir" ]]; then
+ if [ "$dir" = "$env_dir" ]; then
addpath=false
break
fi
@@ -257,7 +257,7 @@ for dir in "${env_path[@]}"; do
fi
done
-if [[ $mode == vcheck ]]; then
+if [ "$mode" = vcheck ]; then
exec "${command}" "$@"
fi
@@ -265,16 +265,20 @@ fi
# It doesn't work with -rpath.
# This variable controls whether they are added.
add_rpaths=true
-if [[ ($mode == ld || $mode == ccld) && "$SPACK_SHORT_SPEC" =~ "darwin" ]];
+if [ "$mode" = ld ] || [ "$mode" = ccld ];
then
- for arg in "$@"; do
- if [[ ($arg == -r && $mode == ld) ||
- ($arg == -r && $mode == ccld) ||
- ($arg == -Wl,-r && $mode == ccld) ]]; then
- add_rpaths=false
- break
- fi
- done
+ case "$SPACK_SHORT_SPEC" in
+ *darwin*)
+ for arg in "$@"; do
+ if { [ "$arg" = -r ] && [ "$mode" = ld ]; } ||
+ { [ "$arg" = -r ] && [ "$mode" = ccld ]; } ||
+ { [ "$arg" = -Wl,-r ] && [ "$mode" = ccld ]; }; then
+ add_rpaths=false
+ break
+ fi
+ done
+ ;;
+ esac
fi
# Save original command for debug logging
@@ -368,58 +372,71 @@ while [ $# -ne 0 ]; do
-Wl,*)
arg="${1#-Wl,}"
if [ -z "$arg" ]; then shift; arg="$1"; fi
- if [[ "$arg" = -rpath=* ]]; then
- rp="${arg#-rpath=}"
- elif [[ "$arg" = --rpath=* ]]; then
- rp="${arg#--rpath=}"
- elif [[ "$arg" = -rpath,* ]]; then
- rp="${arg#-rpath,}"
- elif [[ "$arg" = --rpath,* ]]; then
- rp="${arg#--rpath,}"
- elif [[ "$arg" =~ ^-?-rpath$ ]]; then
- shift; arg="$1"
- if [[ "$arg" != -Wl,* ]]; then
- die "-Wl,-rpath was not followed by -Wl,*"
- fi
- rp="${arg#-Wl,}"
- elif [[ "$arg" = "$dtags_to_strip" ]] ; then
- : # We want to remove explicitly this flag
- else
- other_args+=("-Wl,$arg")
- fi
+ case "$arg" in
+ -rpath=*)
+ rp="${arg#-rpath=}"
+ ;;
+ --rpath=*)
+ rp="${arg#--rpath=}"
+ ;;
+ -rpath,*)
+ rp="${arg#-rpath,}"
+ ;;
+ --rpath,*)
+ rp="${arg#--rpath,}"
+ ;;
+ ^-?-rpath$)
+ shift; arg="$1"
+ case "$arg" in
+ -Wl,*) die "-Wl,-rpath was not followed by -Wl,*" ;;
+ esac
+ rp="${arg#-Wl,}"
+ ;;
+ "$dtags_to_strip")
+ : # We want to remove explicitly this flag
+ ;;
+ *)
+ other_args+=("-Wl,$arg")
+ ;;
+ esac
;;
-Xlinker,*)
arg="${1#-Xlinker,}"
if [ -z "$arg" ]; then shift; arg="$1"; fi
- if [[ "$arg" = -rpath=* ]]; then
- rp="${arg#-rpath=}"
- elif [[ "$arg" = --rpath=* ]]; then
- rp="${arg#--rpath=}"
- elif [[ "$arg" = -rpath ]] || [[ "$arg" = --rpath ]]; then
- shift; arg="$1"
- if [[ "$arg" != -Xlinker,* ]]; then
- die "-Xlinker,-rpath was not followed by -Xlinker,*"
- fi
- rp="${arg#-Xlinker,}"
- else
- other_args+=("-Xlinker,$arg")
- fi
+ case "$arg" in
+ -rpath=*)
+ rp="${arg#-rpath=}"
+ ;;
+ --rpath=*)
+ rp="${arg#--rpath=}"
+ ;;
+ -rpath|--rpath)
+ shift; arg="$1"
+ case "$arg" in
+ -Xlinker,*) die "-Xlinker,-rpath was not followed by -Xlinker,*" ;;
+ esac
+ rp="${arg#-Xlinker,}"
+ ;;
+ *)
+ other_args+=("-Xlinker,$arg")
+ ;;
+ esac
;;
-Xlinker)
- if [[ "$2" == "-rpath" ]]; then
- if [[ "$3" != "-Xlinker" ]]; then
+ if [ "$2" = "-rpath" ]; then
+ if [ "$3" != "-Xlinker" ]; then
die "-Xlinker,-rpath was not followed by -Xlinker,*"
fi
shift 3;
rp="$1"
- elif [[ "$2" = "$dtags_to_strip" ]] ; then
+ elif [ "$2" = "$dtags_to_strip" ] ; then
shift # We want to remove explicitly this flag
else
other_args+=("$1")
fi
;;
*)
- if [[ "$1" = "$dtags_to_strip" ]] ; then
+ if [ "$1" = "$dtags_to_strip" ] ; then
: # We want to remove explicitly this flag
else
other_args+=("$1")
@@ -448,11 +465,11 @@ done
flags=()
# Add debug flags
-if [ "${SPACK_ADD_DEBUG_FLAGS}" == "true" ]; then
+if [ "${SPACK_ADD_DEBUG_FLAGS}" = "true" ]; then
flags=("${flags[@]}" "${debug_flags}")
# If a custom flag is requested, derive from environment
-elif [ "$SPACK_ADD_DEBUG_FLAGS" == "custom" ]; then
+elif [ "$SPACK_ADD_DEBUG_FLAGS" = "custom" ]; then
IFS=' ' read -ra SPACK_DEBUG_FLAGS <<< "$SPACK_DEBUG_FLAGS"
flags=("${flags[@]}" "${SPACK_DEBUG_FLAGS[@]}")
fi
@@ -494,20 +511,24 @@ case "$mode" in
esac
# On macOS insert headerpad_max_install_names linker flag
-if [[ ($mode == ld || $mode == ccld) && "$SPACK_SHORT_SPEC" =~ "darwin" ]];
+if [ "$mode" = ld ] || [ "$mode" = ccld ];
then
- case "$mode" in
- ld)
- flags=("${flags[@]}" -headerpad_max_install_names) ;;
- ccld)
- flags=("${flags[@]}" "-Wl,-headerpad_max_install_names") ;;
+ case "$SPACK_SHORT_SPEC" in
+ *darwin*)
+ case "$mode" in
+ ld)
+ flags=("${flags[@]}" -headerpad_max_install_names) ;;
+ ccld)
+ flags=("${flags[@]}" "-Wl,-headerpad_max_install_names") ;;
+ esac
+ ;;
esac
fi
IFS=':' read -ra rpath_dirs <<< "$SPACK_RPATH_DIRS"
-if [[ $mode == ccld || $mode == ld ]]; then
+if [ "$mode" = ccld ] || [ "$mode" = ld ]; then
- if [[ "$add_rpaths" != "false" ]] ; then
+ if [ "$add_rpaths" != "false" ] ; then
# Append RPATH directories. Note that in the case of the
# top-level package these directories may not exist yet. For dependencies
# it is assumed that paths have already been confirmed.
@@ -517,7 +538,7 @@ if [[ $mode == ccld || $mode == ld ]]; then
fi
IFS=':' read -ra link_dirs <<< "$SPACK_LINK_DIRS"
-if [[ $mode == ccld || $mode == ld ]]; then
+if [ "$mode" = ccld ] || [ "$mode" = ld ]; then
libdirs=("${libdirs[@]}" "${link_dirs[@]}")
fi
@@ -527,13 +548,13 @@ case "$mode" in
# Set extra RPATHs
IFS=':' read -ra extra_rpaths <<< "$SPACK_COMPILER_EXTRA_RPATHS"
libdirs+=("${extra_rpaths[@]}")
- if [[ "$add_rpaths" != "false" ]] ; then
+ if [ "$add_rpaths" != "false" ] ; then
rpaths+=("${extra_rpaths[@]}")
fi
# Set implicit RPATHs
IFS=':' read -ra implicit_rpaths <<< "$SPACK_COMPILER_IMPLICIT_RPATHS"
- if [[ "$add_rpaths" != "false" ]] ; then
+ if [ "$add_rpaths" != "false" ] ; then
rpaths+=("${implicit_rpaths[@]}")
fi
@@ -560,8 +581,8 @@ for dir in "${includes[@]}"; do args+=("-I$dir"); done
for dir in "${isystem_includes[@]}"; do args+=("-isystem" "$dir"); done
IFS=':' read -ra spack_include_dirs <<< "$SPACK_INCLUDE_DIRS"
-if [[ $mode == cpp || $mode == cc || $mode == as || $mode == ccld ]]; then
- if [[ "$isystem_was_used" == "true" ]] ; then
+if [ "$mode" = cpp ] || [ "$mode" = cc ] || [ "$mode" = as ] || [ "$mode" = ccld ]; then
+ if [ "$isystem_was_used" = "true" ] ; then
for dir in "${spack_include_dirs[@]}"; do args+=("-isystem" "$dir"); done
else
for dir in "${spack_include_dirs[@]}"; do args+=("-I$dir"); done
@@ -612,18 +633,18 @@ if [ -n "$SPACK_CCACHE_BINARY" ]; then
fi
# dump the full command if the caller supplies SPACK_TEST_COMMAND=dump-args
-if [[ $SPACK_TEST_COMMAND == dump-args ]]; then
+if [ "$SPACK_TEST_COMMAND" = dump-args ]; then
IFS="
" && echo "${full_command[*]}"
exit
-elif [[ -n $SPACK_TEST_COMMAND ]]; then
+elif [ -n "$SPACK_TEST_COMMAND" ]; then
die "ERROR: Unknown test command"
fi
#
# Write the input and output commands to debug logs if it's asked for.
#
-if [[ $SPACK_DEBUG == TRUE ]]; then
+if [ "$SPACK_DEBUG" = TRUE ]; then
input_log="$SPACK_DEBUG_LOG_DIR/spack-cc-$SPACK_DEBUG_LOG_ID.in.log"
output_log="$SPACK_DEBUG_LOG_DIR/spack-cc-$SPACK_DEBUG_LOG_ID.out.log"
echo "[$mode] $command $input_command" >> "$input_log" |
|
About the overhead of the compiler wrapper, I think an interesting benchmark is the configure script of Using the following diff diff --git a/lib/spack/spack/build_systems/autotools.py b/lib/spack/spack/build_systems/autotools.py
index 03923e3e51..209a58295c 100644
--- a/lib/spack/spack/build_systems/autotools.py
+++ b/lib/spack/spack/build_systems/autotools.py
@@ -339,7 +339,11 @@ def configure(self, spec, prefix):
options += self.configure_args()
with working_dir(self.build_directory, create=True):
+ import time
+ start = time.time()
inspect.getmodule(self).configure(*options)
+ end = time.time()
+ print("Time = ", end - start)
def build(self, spec, prefix):
"""Makes the build targets specified byand the following environment: spack:
config:
ccache: false
install_tree:
root: ./here
specs:
- gettext
view: falseWhen I do Then when I remove the compiler wrapper: $ cd ~/spack/lib/spack/env/
$ rm -rf *
$ mkdir gcc
$ ln -s /usr/bin/gcc-10 gcc/gcc
$ ln -s /usr/bin/g++-10 gcc/g++And rerun the So, the overhead of the compiler wrapper is not particularly negligible; 16% more time spent on the gettext configure phase. I got similar results for |
|
@haampie can you put up a PR for the initial diff you've got there? We can start with that and work on the rest of the posix conversion from there. We've been talking about how to do the compiler wrappers for the windows port -- so we've given it some thought and settled on still doing shell instead of compiling... the rationale was, basically:
So, it just seems simpler esp. if we need bash for git and have to bootstrap that anyway. And bootstrapping bash or posix sh (via, say, busybox or some spack build) seems simpler in any scenario than bootstrapping the wrappers themselves for every build. The fact that git comes with bash, and azure VMs ship with git, git bash, and WSL sort of makes me think that the Unix dev environment is pretty much the only real CLI dev environment these days, so we should work on making spack self-host that instead of working really hard to be pure windows, since windows itself seems to be going the other way (towards Linuxness). Thoughts? |
|
Based on some numbers from @adamjstewart, I think we could reclaim 40% or so of that overhead with dash, though I wonder how much is startup overhead and how much is just the string processing. |
|
See #8703 for those numbers. |
|
I put up an initial draft PR here: #25557. |
This one is also good for compiler as dependencies, since in this way each package providing a language will be responsible to create the wrappers for that language in the build directory of the dependent - and all compiled languages will be treated equally. |
My team has Alpine Linux containers for testing Melissa. The three main reasons for Alpine Linux (containers) are
Disadvantages:
|
|
Closing this as we decided to convert the compiler wrappers to posix sh. See #26048. |
|
it's touching more than the compiler wrapper, but sure :p |
how else can you use a shell built by spack