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

Mechanism to unconditionally emit warnings from build scripts #3777

Closed
SergioBenitez opened this issue Feb 28, 2017 · 15 comments · Fixed by #3847
Closed

Mechanism to unconditionally emit warnings from build scripts #3777

SergioBenitez opened this issue Feb 28, 2017 · 15 comments · Fixed by #3847

Comments

@SergioBenitez
Copy link
Contributor

SergioBenitez commented Feb 28, 2017

At present, I can use println!("cargo:warning={}", message) in a build script to ask cargo to emit a warning message during compilation. Unfortunately, this warning message cannot be seen from end-users of a library unless they pass -vv to their cargo invocation. Of course, this is not something that users do, and as a result, these warnings are never seen by end-users.

This inability to unconditionally emit a warning message during compilation leads to usability issues in libraries that utilize build scripts. As an example, consider Rocket's build script. The script attempts to detect the version of the user's rustc so that it can decline to compile with known incompatible versions. When the rustc version cannot be determined, the build script attempts to emit a warning. Unfortunately this warning is hidden by cargo by default leading to users being confused about why Rocket is failing to build; this is the exact thing the script is trying to prevent.

I would like a mechanism by which build scripts can log to the console, regardless of the verbosity level set by the user. I can think of several such mechanisms:

  1. Use the same cargo:warning={} trick, but always emit the warnings.
  2. Emit everything written to stderr from build scripts.
  3. Add levels to warnings, and always emit errors above some threshold. Something like: cargo:warning(k) where k is some integer defining the level.
  4. Add something like cargo:critical={} which unconditionally emits critical messages.

My personal preference is for 2. This is what I would expect when writing a build script, but I'd be content with any solution.

@aturon
Copy link
Member

aturon commented Mar 11, 2017

I agree that #2 seems like a simple, intuitive approach. @alexcrichton, do you see any downsides? If not, would you be willing to mentor @SergioBenitez on making a PR for the change?

@alexcrichton
Copy link
Member

Heh, yes I see a number of downsides! We have very intentionally not allowed build scripts to print to a main build by default historically.

Typically when you check out a C project and run ./configure && make you're sprayed with megabytes of output that you don't really know how to parse anyway. Cargo strives to avoid this by having the output be meaningful to the user (any user) at the time and not have a bunch of superfluous information.

The failure mode is that if we arbitrarily ship stderr to the output on the console you're highly likely to get a bunch of warnings, log messages, etc, from build scripts you know nothing about and have absolutely no clue how to fix. This ends up being a pretty poor user experience to have by default.

The cargo:warning was an attempt to balance these concerns with other features like warnings form C compilers and such. The warnings are emitted in a controlled fashion, though, and you're in theory never sprayed with warnings you know nothing about.

@aturon
Copy link
Member

aturon commented Mar 13, 2017

@alexcrichton

Do you think it's out of the question to add a separate cargo:error path? Is the concern that it would be abused and create a huge mess of output by default?

It occurs to me that, even if we didn't print the output by default, we could at least print a message saying "XX lines of errors were produced; run with -vv to see them" or something.

@alexcrichton
Copy link
Member

I think it's reasonable to add support for something like cargo:error which completely halts the build with a nice error message (today build script failures aren't always the most readable). The crucial part to me is that it always unconditionally halts the build no matter what.

My main issue with warnings is that the one person's definition of "warnings that must be seen" is inevitably someone else's "warnings they don't care about and want shut up"

@aturon
Copy link
Member

aturon commented Mar 13, 2017

@alexcrichton I agree, that seems like a very plausible constraint.

@SergioBenitez From what I understand of your use case, unconditionally halting the build would be fine, since it's going to fail to compile anyway.

If that sounds good to you, would you be interested in working toward a PR, perhaps with some guidance?

@SergioBenitez
Copy link
Contributor Author

The central issue here is that I'd like to issue a warning unconditionally. In my case, the warning being issued may or may not be followed by a failed build later. As such, failing the build in this circumstance is not an option.

Wouldn't I simply use panic! if I wanted to fail the build immediately? That's what I do now, and I don't see what cargo:error would add to this.

@aturon
Copy link
Member

aturon commented Mar 13, 2017

@SergioBenitez Ah ok, looking back at your original case I see that you want to issue the warning whenever you couldn't detect the version number, which might not indicate that the build will fail.

Hopefully you understand the concerns @alexcrichton is raising from an ecosystem perspective. It's a typical situation in Cargo, where opening a door like this that seems fine in one isolated case can cause problems if used at scale.

@alexcrichton What do you think about some way for Cargo to alert you that there were some warnings? (I assume it doesn't do this now?)

@SergioBenitez
Copy link
Contributor Author

What if warnings emitted by a build script are buffered and later emitted only if the build for that crate fails? Cargo could output something like:

The following warnings were emitted during compilation:

{{ buffered warnings }}

@aturon
Copy link
Member

aturon commented Mar 13, 2017

@SergioBenitez That sounds like a pretty reasonable compromise to me.

@alexcrichton
Copy link
Member

Yeah that sounds reasonable to me as well!

@SergioBenitez
Copy link
Contributor Author

SergioBenitez commented Mar 14, 2017

Great! I'm happy to implement this.

But, I wonder: should we only display the warnings by the crate that failed to compile, or should we display all buffered warnings? I worry that some crate B will fail to compile because of something that went wrong with crate A but the user won't see crate A's warnings even though it was the root cause of the issue. Maybe we should display warnings from all dependencies of a failing crate?

@alexcrichton
Copy link
Member

I'd prefer to start conservatively and only emit for the crate at hand, otherwise I feel like it may tip too far in the other direction of spraying too many warnings.

A possible failure mode here is that the actual error could get drowned out in warnings if dependencies print too many warnings.

SergioBenitez added a commit to SergioBenitez/cargo that referenced this issue Apr 9, 2017
bors added a commit that referenced this issue Apr 12, 2017
Always emit build script warnings for crates that fail to build

Resolves #3777.

r? @alexcrichton
@Boddlnagg
Copy link
Contributor

After this has been closed, I'm not sure if this means that the documentation on http://doc.crates.io/build-script.html is now outdated, which says:

warning=MESSAGE is a message that will be printed to the main console after a build script has finished running. Warnings are only shown for path dependencies (that is, those you're working on locally), so for example warnings printed out in crates.io crates are not emitted by default.

Will the warning be printed for crates.io crates or not?

@alexcrichton
Copy link
Member

@Boddlnagg no, crates on crates.io with build scripts will not have their warnings emitted by default, only on -vv I believe.

@SergioBenitez
Copy link
Contributor Author

@Boddlnagg @alexcrichton Warnings are emitted for all crates if compilation fails. If compilation doesn't fail, warnings are only emitted for local crates.

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 a pull request may close this issue.

4 participants