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

Output hygiene: write to stderr by default #5049

Closed
wants to merge 2 commits into from
Closed

Output hygiene: write to stderr by default #5049

wants to merge 2 commits into from

Conversation

sanmai-NL
Copy link

Only proper machine readable output should be written to stdout. This would be JSON for Cargo and rustc. This is only one slight patch toward this requirement, there are still other instances of writing various stuff to stdout in rustc.

Only proper machine readable output should be written to stdout. This would be
JSON for Cargo and `rustc`. This is only one slight patch toward this
requirement, there are still other instances of writing various stuff to stdout
in `rustc`.
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@@ -49,7 +49,7 @@ pub fn package(ws: &Workspace,
}).collect();
list.sort();
for file in list.iter() {
println!("{}", file.display());
eprintln!("{}", file.display());
Copy link
Member

Choose a reason for hiding this comment

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

This is machine-consumable, and is the only way to get the list of files that would be packaged AFAIK.

Copy link
Author

@sanmai-NL sanmai-NL Feb 17, 2018

Choose a reason for hiding this comment

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

I concur. However, this PR isn’t so much about distinguishing machine-readable output from human-readable output across Cargo. It is rather about distinguishing standardized, exclusively machine-readable output from ‘other’ output. So can’t we make principled decision about what ‘machine-readable’ means within Cargo? If that means making all such output JSON, then let’s do that rather than making local exceptions left and right.

Copy link
Member

Choose a reason for hiding this comment

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

The change as-is will break code that calls this command and expects to see something on stdout.

Also reduce overspecificity of tests.
@sanmai-NL
Copy link
Author

sanmai-NL commented Feb 17, 2018

The cross-compilation test (tests/cross-compile.rs) still fails, since I can’t run it locally. I find the diff output not clear enough to fix it. Can anyone suggest the expected output?

@ehuss
Copy link
Contributor

ehuss commented Feb 17, 2018

This would undo the fix for #4884 which was just added a few weeks ago.

@sanmai-NL
Copy link
Author

sanmai-NL commented Feb 17, 2018

@ehuss
@tromey

In response to #4878 (comment):

Currently, cargo help test output goes to stderr. This makes it unnecessarily difficult to pipe the output through a pager.

That’s only true if you use a shell in which that’s difficult. It’s easy with e.g. fish:

stderr_command ^| less

Furthermore, this isn't an error situation -- the command exists to print the help, so it seems to me that it ought to go to stdout. (If it's any use, this is a longstanding GNU convention for tools as well; so many if not most core Linux utilities work this way.)

The same discussion has come up repeatedly (e.g. #1473). In this day and age, it is important to be able parse output flowing through a pipeline according to some standardized formal grammar. While it was convenient in the past to have CLI tools in which at least some crude distinction was made between ‘things you’ll want to see’ and ‘extra stuff’, there is no formal criterion to distinguish ‘output’ from ‘diagnostics’ or ‘errors’. One thing is sure, formal and natural language output don’t go well together.

@tromey
Copy link
Contributor

tromey commented Feb 18, 2018

IIUC this bug arises because some program invokes cargo with --help and expects to get an empty stdout.

Could this program just not use --help?

Alternatively, if very strict separation is needed, it seems that cargo could have some way to say "please be strict about stdout use" -- an environment variable or some such.

@matklad
Copy link
Member

matklad commented Feb 19, 2018

Hm, so it indeed looks like we are printing some stuff to stdout which should probably go to stderr. However, I think we'll need to be more nuanced than just replacing all println with eprintln :-)

In particular, I believe

  1. println!("Hello, world") should still use stdout: it is the expected behavior, and hello world is not intended for consumption by robots, so it's perfectly fine to use stdout for humans!

  2. version stuff should got to stdout: it is de-facto stable machine readable output, and tools do relay on it.

  3. same goes for pkgid

  4. --help definitely should go to stdout: it is not intended for consumption by robots, and --help | less is a very common workflow which we should not break (well, ideally we should try to keep the help small so that you don't need a pager for it, but this doesn't seem like a realistically achievable goal in the long run :)

@bors
Copy link
Contributor

bors commented Feb 19, 2018

☔ The latest upstream changes (presumably #5041) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

r? @matklad

@sanmai-NL
Copy link
Author

sanmai-NL commented Feb 26, 2018

@matklad:

Hm, so it indeed looks like we are printing some stuff to stdout which should probably go to stderr.
However, I think we'll need to be more nuanced than just replacing all println with eprintln :-)

In particular, I believe

println!("Hello, world") should still use stdout: it is the expected behavior, and hello world is not intended for consumption by robots, so it's perfectly fine to use stdout for humans!

Strongly disagree. It should not be arbitrary where to write output to. Where did you get from that people would expect ‘hello world’-like text to appear on stdout? It makes no visual difference.

version stuff should got to stdout: it is de-facto stable machine readable output, and tools do relay on it.
same goes for pkgid

Disagree, machine readable does not mean ‘could be beaten into parseability’, but ‘comes in a standardized syntax’. Are you providing a Command Line Interface or not? By going on this way, enormous breakage will occur whenever the version output changes a bit and people rely on parsing it.

--help definitely should go to stdout: it is not intended for consumption by robots, and --help | less is > a very common workflow which we should not break (well, ideally we should try to keep the help small > so that you don't need a pager for it, but this doesn't seem like a realistically achievable goal in the long > run :)

‘it is not intended for consumption by robots’ -> so it should be on stderr, not stdout.

Piping will not be broken, as I explained in an earlier post.

@matklad
Copy link
Member

matklad commented Feb 27, 2018

Where did you get from that people would expect ‘hello world’-like text to appear on stdout?

Overwhelming majority of hello-world programs I've seen printed the message to stdout. For example, the one at https://www.rust-lang.org does this :-) It can be argued that all hello-worlds in the world are wrong, but this is a pretty strong claim. If we want to change this, we'll need to update all the learning materials as well, so this would probably need to go through the RFC process.

By going on this way, enormous breakage will occur whenever the version output changes a bit and people rely on parsing it.

People already rely on it, so, if we are going to change it, it must be a backwards-compatible change. For example, adding something like cargo version --semver which is specified to print a semver string might be a good idea (cc #3046).

Piping will not be broken, as I explained in an earlier post.

The reasoning here is pretty similar to the hello-world case: most of over tools use stdout for help. It may be argued that all the tools are wrong, and that we should switch to stderr here. However, this should be done simultaneously across all of the Rust tools (rustc/rustup/cargo), and this is also a pretty drastic change which probably wants an RFC :-)

To make it clear: I do want Cargo's output to be easier to parse, and I've even broken every other test for it :-) However there are things which we just can't change without breaking tons of code, or becoming incompatible with virtually every other tool. This doesn't mean that we can't change anything, we are not quite at the XKCD1172 level of backwards compatibility, but I think it is a good approach to separate low-risk changes from high-risk changes, and to go an extra mile to provide backward-compatibility opt-ins.

@sanmai-NL
Copy link
Author

sanmai-NL commented Feb 28, 2018

@matklad: I agree, I disagree with the old habit as I expressed, but it would be better to file an RFC mandating the various ideals I argued for. Note that I don’t have a plan (yet) for actually doing it (alone).

@sanmai-NL sanmai-NL closed this Feb 28, 2018
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.

8 participants