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

explicitly connect /dev/tty. Allows running via sh #5500

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sabine
Copy link

@sabine sabine commented Apr 3, 2023

It should be super simple and quick to install opam on various Linux distros and macOS. Some of the targeted OSes might not have the bash command needed for the invocation

bash -c "sh <(curl -fsSL https://raw.githubusercontent.com/ocaml/opam/master/shell/install.sh)"

Instead, install.sh should be invokable via

curl -fsSL https://raw.githubusercontent.com/ocaml/opam/master/shell/install.sh | sh

However, a problem with that is that, when piping the downloaded script into sh, /dev/tty is not connected, so we cannot read from stdin.

This patch remedies this issue by changing install.sh to explicitly read from /dev/tty. It also factors out the prompting into a function prompt.

Note: if any of the other commands called by install.sh require user interaction via stdin, they should also be explicitly connected to /dev/tty. I have not checked whether they do!

shell/install.sh Outdated Show resolved Hide resolved
@kit-ty-kate
Copy link
Member

Do we know if anyone is using the install script in an automated way (e.g. (echo /usr/bin/ ; yes) | bash -c "sh <(curl -fsSL https://raw.githubusercontent.com/ocaml/opam/master/shell/install.sh)")? If some do this change will break them as it would make it very hard to pipe any kind of input to this script.

@sabine
Copy link
Author

sabine commented Apr 4, 2023

This is a good question. There is the possibility to extend this patch to conditionally connect /dev/tty based on a new flag, so that breaking automated scripts could be fixed by passing the new flag to this script. At least, that's what the rustup install script does. 🤔

@sabine
Copy link
Author

sabine commented Apr 5, 2023

I found a way to conditionally connect /dev/tty in a backwards-compatible way by using an environment variable that must be set in order to explicitly connect to /dev/tty.

Then, the script can be invoked as:

curl -fsSL https://raw.githubusercontent.com/ocaml/opam/master/shell/install.sh | set TTY=1 sh

This is backwards-compatible for automated scripts, as explicitly connecting to /dev/tty is opt-in instead of opt-out.

shell/install.sh Outdated Show resolved Hide resolved
shell/install.sh Outdated Show resolved Hide resolved
@kit-ty-kate
Copy link
Member

Then, the script can be invoked as:

curl -fsSL https://raw.githubusercontent.com/ocaml/opam/master/shell/install.sh | set TTY=1 sh

set does not do anything but I believe what you want is env instead (which is POSIX-complient)

@sabine sabine force-pushed the connect_stdin_explicitly branch 2 times, most recently from 6799e66 to 8e65706 Compare April 5, 2023 16:42
@kit-ty-kate kit-ty-kate added this to the 2.4.0~alpha1 milestone Sep 25, 2024
@kit-ty-kate kit-ty-kate added the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Dec 1, 2024
@kit-ty-kate
Copy link
Member

This PR is queued on #6313. There will be a couple of things to do to make it correct afterwards too, i'll handle it.
I'll also change the TTY env variable to a --tty argument to make it even more backward compatible and more fitting to the rest of the script. It has also the advantage to guide users on how to pass extra parameters when using the | sh method. E.g.:

curl -fsSL https://opam.ocaml.org/install.sh | sh -s -- --tty --version 2.2.0

and without any extra parameters:

curl -fsSL https://opam.ocaml.org/install.sh | sh -s -- --tty

@kit-ty-kate kit-ty-kate removed the PR: QUEUED Pending pull request, waiting for other work to be merged or closed label Dec 2, 2024
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.

2 participants