Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cc: convert compiler wrappers to POSIX sh #25557

Closed
wants to merge 1 commit into from

Conversation

tgamblin
Copy link
Member

@tgamblin tgamblin commented Aug 23, 2021

Fixes #8703.
Closes #25380.

Putting up an initial PR to start the process of converting cc to POSIX shell.

Remaining to do:

  • initial shellcheck-based fixes
  • replace arrays with posix sh tricks
  • read
  • heredocs (is this needed?)
  • additional fixes

@tgamblin
Copy link
Member Author

@haampie: what is the issue with heredocs? They're supported by Bourne shell so not sure why they're listed as needed.

@tgamblin
Copy link
Member Author

Looks like the initial changes weren't entirely sound -- tests are failing.

Copy link
Contributor

@christoph-conrads christoph-conrads left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

POSIX shells do not support arrays, cf. StackOverflow: Arrays in unix shell? Error message on Alpine Linux with Ash:

/home/john/spack/lib/spack/env/gcc/gcc: line 26: syntax error: unexpected "("

@@ -78,10 +78,10 @@ IFS=' ' read -ra SPACK_LDFLAGS <<< "$SPACK_LDFLAGS"
IFS=' ' read -ra SPACK_LDLIBS <<< "$SPACK_LDLIBS"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Here strings (<<<) are a POSIX feature
  • read -a creates an array; POSIX shells do not feature arrays; dash complains with dash: 1: read: Illegal option -a

Copy link
Contributor

@christoph-conrads christoph-conrads left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

POSIX shells do not support arrays, cf. StackOverflow: Arrays in unix shell? Error message on Alpine Linux with Ash:

/home/john/spack/lib/spack/env/gcc/gcc: line 26: syntax error: unexpected "("

@christoph-conrads
Copy link
Contributor

christoph-conrads commented Aug 23, 2021

Putting up an initial PR to start the process of converting cc to POSIX shell.

Why not Python?

  • There must be a Python interpreter available; you cannot run Spack without it
  • A lot of features:
    • assertions
    • data structures like arrays, maps, and so on
    • optional static typing
    • exceptions and/or proper return values
  • Absence of error sources
    • confusion between quoted/unquoted parameter substitution, parameters with/without newlines
    • the interaction between set -e and using the exit status as a branching condition

@tgamblin
Copy link
Member Author

POSIX shells do not support arrays

Yep, this is about removing arrays and emulating them -- see http://www.etalabs.net/sh_tricks.html.

@tgamblin
Copy link
Member Author

tgamblin commented Aug 23, 2021

@christoph-conrads:

Why not Python?

It'd be great if we could. In fact, we started this way. If you go back to 0f3b80c (in 2014), you'll see that we replaced the then-python version with bash. This was done because python startup overhead is really high and was making builds extremely slow. We frequently saw slowdowns of ~6x for builds, just from the wrappers.

@christoph-conrads
Copy link
Contributor

Yep, this is about removing arrays and emulating them -- see http://www.etalabs.net/sh_tricks.html.

I see.

Unlike “enhanced” Bourne shells such as Bash, the POSIX shell does not have array types. However, with a bit of inefficiency, you can get array-like semantics in a pinch using pure POSIX sh. The trick is that you do have one (and only one) array — the positional parameters “$1”, “$2”, etc. — and you can swap things in and out of this array.

😬

Have you considered having the shell code generated?

@christoph-conrads
Copy link
Contributor

@christoph-conrads:

Why not Python?

It'd be great if we could. In fact, we started this way. If you go back to 0f3b80c (in 2014), you'll see that we replaced the then-python version with bash. This was done because python startup overhead is really high and was making builds extremely slow. We frequently saw slowdowns of ~6x for builds, just from the wrappers.

Thank you for the link to the commit.

@haampie
Copy link
Member

haampie commented Aug 23, 2021

@christoph-conrads see #25380 (comment) and below

@christoph-conrads
Copy link
Contributor

@christoph-conrads see #25380 (comment) and below

From the discussion:

Generating the compiler wrapper, benchmarks, everything. You've discussed all approaches that came to my mind.

@tgamblin
Copy link
Member Author

closing this as @cosmicexplorer is working on #26048.

@tgamblin tgamblin closed this Sep 19, 2021
@haampie haampie deleted the convert-cc-to-posix-sh branch August 2, 2022 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Spack's compiler wrapper compatible with dash
3 participants