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

shouldn't install script be quiet? #3350

Closed
2 tasks done
vic1707 opened this issue May 6, 2023 · 13 comments · Fixed by #3910
Closed
2 tasks done

shouldn't install script be quiet? #3350

vic1707 opened this issue May 6, 2023 · 13 comments · Fixed by #3910

Comments

@vic1707
Copy link
Contributor

vic1707 commented May 6, 2023

Problem you are trying to solve

Hello,
I'm currently working on some installation scripts and I'd like to install rust somewhat silently (or at least restrict the output to only errors).
I found the -q|--quiet option in the script but it doesn't affect the script itself, only rustup as far as I understand.

# curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -q -y       
info: downloading installer
info: profile set to 'default'
info: default host triple is aarch64-unknown-linux-gnu
info: syncing channel updates for 'stable-aarch64-unknown-linux-gnu'
info: latest update on 2023-04-20, rust version 1.69.0 (84c898d65 2023-04-16)
info: downloading component 'cargo'
info: downloading component 'clippy'
info: downloading component 'rust-docs'
info: downloading component 'rust-std'
info: downloading component 'rustc'
info: downloading component 'rustfmt'
info: installing component 'cargo'
info: installing component 'clippy'
info: installing component 'rust-docs'
info: installing component 'rust-std'
info: installing component 'rustc'
info: installing component 'rustfmt'
info: default toolchain set to 'stable-aarch64-unknown-linux-gnu'

  stable-aarch64-unknown-linux-gnu installed - rustc 1.69.0 (84c898d65 2023-04-16)


Rust is installed now. Great!

To get started you may need to restart your current shell.
This would reload your PATH environment variable to include
Cargo's bin directory ($HOME/.cargo/bin).

To configure your current shell, run:
source "$HOME/.cargo/env"

All printf and echo lines in the script are postfixed with 1>&2 which redirect stdout to stderr even though the script has dedicated say and err function.

By doing so we are completely unable to only allow errors to be printed out and the -q|--quiet doesn't do what its name suggests.
We are also unable to bypass this restriction by setting 1>/dev/null ourselves.

Solution you'd like

I think the best solution would be apply the -q|--quiet flag to the install script.
Another solution would be to let stdout be what it is instead of redirecting it to stderr (removing most of the 1>&2 inside the script).
At least I'd like to see some sort of warning/message in the --help section of the script.

Notes

English isn't my primary language, I'm sorry if I made mistakes or if anything sounds bad.

Tasks

@J-ZhengLi
Copy link
Member

Another solution would be to let stdout be what it is instead of redirecting it to stderr (removing most of the 1>&2 inside the script).

Isn't most of those indicates error or some kind of warnings? I'm only against the info: downloading installer being redirected to the stderr, but it looks like it's there to matching rustup's behavior, so I can understand

@djc
Copy link
Contributor

djc commented May 12, 2024

Sounds reasonable to make sure more of the info: output is omitted if -q is used. Want to submit a PR?

@vic1707
Copy link
Contributor Author

vic1707 commented May 12, 2024

would love to, I'll do the PR asap

@rami3l
Copy link
Member

rami3l commented Jun 10, 2024

To clarify the desired outcome of this cleanup:

  • All error/warning/info messages in the script should go thru stderr.
    • Info messages may be omitted on -q.
  • Stdout should be clean when you run the script with -q.

@vic1707
Copy link
Contributor Author

vic1707 commented Jun 20, 2024

All error/warning/info messages in the script should go thru stderr

Not sure I understand that.
I think that, by definition, all infos should be stdout.
I don't see the point of basically ignoring stdout existence (we can always debate on what is or isn't an info tho).

And implementing -q would also feel pretty odd:
Either a say function that early returns if quiet but always prints to stderr or have a ton of
$quiet && say "something"
Doesn't feel right to me 🤔

I'll make some tests in the couple of days (hopefully this weekend) and come back with a diff so we can all agree before submitting the PR.

@rami3l
Copy link
Member

rami3l commented Jun 20, 2024

@vic1707 Thanks for your reply!

Don't get me wrong, the point here is that in rustup the binary all that error/warning/info messages are part of the logging system and they're wired to stderr. We're not ignoring stdout's existence: they're there for prompts and notes, it's just that there aren't many in the script per se.

When I said

Stdout should be clean when you run the script with -q

I meant when the script logic was still running. You'd expect rustup the installer to give some stdout output, even if you pass -q to it.

@vic1707
Copy link
Contributor Author

vic1707 commented Jun 22, 2024

ok I think I get it, feels a bit odd not gonna lie, but makes sense and that's the most important 👍.
I'll hopefully open the PR tomorrow 😁

@vic1707
Copy link
Contributor Author

vic1707 commented Jun 23, 2024

Just had fun implementing it here.

But I sincerely thought/hoped that way less thing would get printed out, in fact it doesn't change anything apart from

info: downloading installer

being silenced (on the happy path).

So, imo, while the implementation of the -q flag in the bash script is a nice to have (and could be still be turned into a PR).
It mostly showed me that I misunderstood the source of my complain which is in fact coming from the rustup binary if I understood correctly ?
If that's the case, is there a reason as to why rustup (bin) outputs everything to stderr ? Is that a technical limitation of some sort ?
It implements the -q and -v flags but afaik the -q doesn't seem to silence anything (but I can be wrong).

My issue was opened as I hoped in the end the happy path of rust's installation would output nothing at all when -q is passed (apart from recoverable errors & warnings).

@rami3l
Copy link
Member

rami3l commented Jun 24, 2024

Just had fun implementing it here.

@vic1707 Thanks! I'd still love to see a PR from this so that we can unify the output style across the script.

But I sincerely thought/hoped that way less thing would get printed out, in fact it doesn't change anything apart from

info: downloading installer

being silenced (on the happy path).

That's exactly what I'd expect from this change.

It mostly showed me that I misunderstood the source of my complain which is in fact coming from the rustup binary if I understood correctly ? If that's the case, is there a reason as to why rustup (bin) outputs everything to stderr ? Is that a technical limitation of some sort ?

Putting the logs into stderr sounds like standard practice. Quoting https://en.cppreference.com/w/c/io/std_streams:

#define stderr

Associated with the standard error stream, used for writing diagnostic output.

It implements the -q and -v flags but afaik the -q doesn't seem to silence anything (but I can be wrong).

Currently it's just preventing the progress bar from appearing, otherwise it could be potentially problematic in CI logs for example.

My issue was opened as I hoped in the end the happy path of rust's installation would output nothing at all when -q is passed (apart from recoverable errors & warnings).

This has become possible after #3875 by setting both -q and RUSTUP_LOG=rustup=WARN at the same time (we wish to ship it in Rustup v1.28). Does that sound okay to you?

@rami3l
Copy link
Member

rami3l commented Jun 24, 2024

Sounds reasonable to make sure more of the info: output is omitted if -q is used. Want to submit a PR?

@djc I think there could potentially be a coherence issue here, as currently -q is clearly for disabling the progress report only:

> rustup-init --help
rustup-init 1.27.1+291 (6eb71e956 2024-06-22)

The installer for rustup

Usage: rustup-init[EXE] [OPTIONS]

Options:
  -v, --verbose                                Enable verbose output
  -q, --quiet                                  Disable progress output
[..]

... so that a functionality like -q is valuable in the script, but I'm not sure if it should be reusing -q at all (the current point of -q is to make the output easier to harness, like the case of CI logs, or for other tools to consume). How about disabling info from the script when RUSTUP_LOG is set to anything? We're not using the legacy output format in this case anyway, so info: downloading installer seems off already.

@djc
Copy link
Contributor

djc commented Jun 24, 2024

I think it would be fine to make -q more general and use a LevelFilter::Warn if it's enabled.

@vic1707
Copy link
Contributor Author

vic1707 commented Jun 25, 2024

Thanks! I'd still love to see a PR from this so that we can unify the output style across the script.

Done ✅ , don't know if I should have put more than a link to this issue tho.

Putting the logs into stderr sounds like standard practice. Quoting https://en.cppreference.com/w/c/io/std_streams:

Ok didn't know that, thx for the informations ^^

This has become possible after #3875 by setting both -q and RUSTUP_LOG=rustup=WARN at the same time (we wish to ship it in Rustup v1.28). Does that sound okay to you?

just made some tests with a master build of rustup-init and it gives the following output

root@0f2c76fdab86:/rustup# RUSTUP_LOG=rustup=WARN ./target/debug/rustup-init -q -y
2024-06-25T18:13:17.261510Z  WARN rustup::cli::self_update: Updating existing toolchain, profile choice will be ignored

  stable-aarch64-unknown-linux-gnu unchanged - rustc 1.79.0 (129f3b996 2024-06-10)


Rust is installed now. Great!

To get started you may need to restart your current shell.
This would reload your PATH environment variable to include
Cargo's bin directory ($HOME/.cargo/bin).

To configure your current shell, you need to source
the corresponding env file under $HOME/.cargo.

This is usually done by running one of the following (note the leading DOT):
. "$HOME/.cargo/env"            # For sh/bash/zsh/ash/dash/pdksh
source "$HOME/.cargo/env.fish"  # For fish

Which still looks like a lot of logs for a -q but it's better than before ^^

@rami3l
Copy link
Member

rami3l commented Jun 26, 2024

@vic1707 Thanks for your PR! I guess I'll evaluate the potential cleanup as mentioned in #3350 (comment) (split into #3900).

@rami3l rami3l linked a pull request Jun 30, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants