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

Use sudo instead #94

Merged
merged 5 commits into from
Nov 28, 2022
Merged

Use sudo instead #94

merged 5 commits into from
Nov 28, 2022

Conversation

jcbhmr
Copy link
Contributor

@jcbhmr jcbhmr commented Nov 27, 2022

Closes #85 with @jcbhmr's changes.

Jacob Hummer added 3 commits November 25, 2022 19:22
This works because sudo executes as another user, and the -i makes it a login
shell which sets the proper $HOME variables.

$_REMOTE_USER variable is fromm a proposal to the
devcontainers spec
https://github.com/devcontainers/spec/blob/main/proposals/features-user-env-variables.md
Shebang is (below), so we use that
```sh
#!/bin/sh
```
@jcbhmr jcbhmr marked this pull request as ready for review November 27, 2022 21:28
@jcbhmr
Copy link
Contributor Author

jcbhmr commented Nov 27, 2022

This is a discussion tangentially related to this PR

@danielbraun89 What are your thoughts?
@DaneWeber I'd like a third opinion on this. Sorry to ping you. If you don't want to be involved beyond your #74 fix, that's OK.

What do you think about piping to sh like curl ... | sh vs piping to /bin/sh vs downloading it and using the hash-bang line?

# Basically do whatever the installer instructions say
curl ... | sh
curl ... | bash
curl ... | python3
# Programmer pre-inspects the file and see what the #!/usr/bin... header is
curl ... | /bin/sh
curl ... | /usr/bin/env bash
# Use the hash-bang by downloading the file and 'chmod +x'-ing it
curl -o INSTALLFILE https://...
chmod +x INSTALLFILE
./INSTALLFILE

I, personally, am all for just following the instructions given by whatever website. For this instance (Haskell), I think it should be

# Install instructions from https://www.haskell.org/ghcup/#
curl --proto '=https' --tlsv1.2 -sSf https://get-ghcup.haskell.org | sh

@danielbraun89
Copy link
Member

danielbraun89 commented Nov 28, 2022

probably following the instructions given by whatever website is safest? as it probably knew which shell is required for the specific syntax

If it had a shebang in it, I guess it would be respected and replaced to it regardless of the shell you launched it with, so also good

@danielbraun89 danielbraun89 merged commit 93c3255 into devcontainers-contrib:main Nov 28, 2022
@jcbhmr jcbhmr deleted the use-sudo-instead branch November 28, 2022 19:07
@DaneWeber
Copy link
Contributor

I'm a little torn because I see some trade-offs. Piping to a shell (... | sh) is presumably in the official install instructions primarily to make installation easier: just copy-paste one line instead of following multiple instructions. Since we're writing scripts/programs here, however, fitting everything on a single line is generally harder to read and understand.

That said, my sense is that the priority in this repo is to optimize for simplicity and ease of maintenance since we want contributors and will likely only discover that a given Feature needs an update when an upstream dependency breaks the build. As such, I really ❤️ the link to the source of the install instructions and following it verbatim. If this Feature fails to install in the future, it will be very easy to check whether the GHCup folks have changed the install method.

@DaneWeber
Copy link
Contributor

Hey @jcbhmr, I'm having a terrible time right now with getting different versions to install via the settings specified in .devcontainer.json. I'm 99% sure that the various env vars set in the script are not making it into the sudo command. I've been trying to pass them in, and thought I had done so (confirming with echoing them into a text file), but I can't get the GHCup installer to respect them.

My question to you: did you verify that if you set a value of 9.2.4 for ghcVersion etc., that version is what is actually installed? I may just have something messed up with my local situation.

@jcbhmr
Copy link
Contributor Author

jcbhmr commented Nov 29, 2022

Hey @jcbhmr, I'm having a terrible time right now with getting different versions to install via the settings specified in .devcontainer.json. I'm 99% sure that the various env vars set in the script are not making it into the sudo command. I've been trying to pass them in, and thought I had done so (confirming with echoing them into a text file), but I can't get the GHCup installer to respect them.

My question to you: did you verify that if you set a value of 9.2.4 for ghcVersion etc., that version is what is actually installed? I may just have something messed up with my local situation.

@DaneWeber

Ah! No I did not verify that the $GHC_VERSION was passed through. It's not 😞. BUT I have good news! It's only because I didn't know that it needed to. In another instance very similar, I used some env vars and commented why some are the way they are.

# We run as the non-root user so to fix https://github.com/devcontainers-contrib/features/issues/80
# Note that we substitute SOME variables before evaluation, and some are
# substituted inside the $_REMOTE_USER shell. Particularily $HOME which needs to
# be from the $_REMOTE_USER, and $VERSION which needs to come from this script.

https://github.com/devcontainers-contrib/features/blob/main/src/pulumi/install.sh#L30

Basically, it's because ENV vars don't pass through the sudo boundary since it's like a whole different user executing. Thus, if you want any to go through, (like I should have done 😢) you must:

sudo -iu "$_REMOTE_USER" <<EOF
  export GHC_VERSION="$GHC_VERSION"
  export CABAL_VERSION="$CABAL_VERSION"
  export INCLUDE_STACK="$INCLUDE_STACK"
  export ADJUST_BASHRC="$ADJUST_BASHRC"
  export INSTALL_STACK_GHCUP_HOOK="$INSTALL_STACK_GHCUP_HOOK"

  # Note that I can use a \$ to avoid substituting NOW and leave that the other shell pass to do
  export STRING_WITH_DOLLAR_SIGN_IN_IT='\$GHC_VERSION'
EOF

Think of it like JavaScript or Python's eval()

I can open a PR & issue to fix this. I should have caught this earlier 😡. Oh well!

@DaneWeber
Copy link
Contributor

DaneWeber commented Nov 29, 2022

@jcbhmr, I was trying to pass them in with the --preserve-env= flag on sudo, but your approach looks more reliable (since my approach was unwieldy and failing).

Note that you don't need the env vars at the top, but the ones set in

export BOOTSTRAP_HASKELL_NONINTERACTIVE=1
export GHCUP_USE_XDG_DIRS=1
export BOOTSTRAP_HASKELL_GHC_VERSION="${GHC_VERSION}"
export BOOTSTRAP_HASKELL_CABAL_VERSION="${CABAL_VERSION}"
export BOOTSTRAP_HASKELL_DOWNLOADER="curl"
if [[ "${INCLUDE_STACK}" = "false" ]]; then
export BOOTSTRAP_HASKELL_INSTALL_NO_STACK="true"
fi
if [[ "${ADJUST_BASH}" = "true" ]]; then
export BOOTSTRAP_HASKELL_ADJUST_BASHRC="true"
fi
if [[ "${INSTALL_STACK_GHCUP_HOOK}" = "false" ]]; then
export BOOTSTRAP_HASKELL_INSTALL_NO_STACK_HOOK="true"
fi

One gotcha I can test is that the so-called boolean vars are true if set, so we'll need to make sure they don't get set to true simply by being included in the list.

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.

Haskell available only for root
3 participants