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

rustup.sh uses ANSI color codes without checking if stdout is wired to a terminal #721

Closed
raphaelcohn opened this issue Sep 13, 2016 · 3 comments

Comments

@raphaelcohn
Copy link
Contributor

This is a minor nit. This line:-

printf "\33[1minfo:\33[0m downloading installer\n"

in https://github.com/rust-lang-nursery/rustup.rs/blob/master/rustup-init.sh uses ANSI color codes. Before outputting the line, we ought to check if stdout is connected to a terminal, eg using test / [, so that users running without a terminal don't get rubbish in their logs if redirecting. Additionally, to be truly robust, we ought to then output the ANSI sequences using tput. However, tput isn't common on embedded systems built around Busybox and the like, so one needs to be quite defensive.

A strategy I've used in the past is lines here 70 through to 250. Obviously that's extensive, but it works. In addition, we seem to check for the presence of printf (need_cmd printf). That seems a little odd, as all POSIX compliant shells that I know of support it. I can't vouch for zsh or ksh93.

As an extra special super-pedantic nit, the printf output should be going to stderr as it's progress information. I'll get my coat...

@raphaelcohn
Copy link
Contributor Author

See PR.

@gifnksm
Copy link

gifnksm commented Oct 23, 2016

#725 merged. Can this issue be closed?

@raphaelcohn
Copy link
Contributor Author

Yes

On 23 Oct 2016 10:36 a.m., "NAKASHIMA, Makoto" [email protected]
wrote:

#725 #725 merged.
Can this issue be closed?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#721 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPx2RQ__JQPVfOEH5j7geJZp2Va_fKRks5q2yqlgaJpZM4J7iAq
.

@Diggsey Diggsey closed this as completed Oct 25, 2016
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

No branches or pull requests

3 participants