-
Notifications
You must be signed in to change notification settings - Fork 14k
bootstrap: Use cargo's build.warnings=deny rather than -Dwarnings
#148332
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
base: main
Are you sure you want to change the base?
Conversation
Another advantage is you get clear signal what is only a warning (at least for me that is nice) |
bootstrap: Use cargo's `build.warnings=deny` rather than -Dwarnings
This has two major advantages. First, it makes us less dependent on the rustc shim, which is nice but not super important. More importantly, it gives us much nicer caching properties.
Previously, `x build --warnings warn && x build --warnings deny` would rebuild all of bootstrap, and if there were any warnings emitted on the last build of the compiler, they would *not* fail the build, because cargo would cache the output rather than rerunning the shim.
After this change, bootstrap rebuilds instantly, and cargo knows that it should fail the build but *without* invalidating the cache.
<details><summary>An example of rebuilding bootstrap after a switch from warn->deny:</summary>
```
INFO: Downloading and building bootstrap before processing --help command.
See src/bootstrap/README.md for help with common commands.
Building bootstrap
Compiling bootstrap v0.0.0 (/Users/jyn/src/ferrocene3/src/bootstrap)
warning: unused variable: `x`
--> src/bootstrap/src/core/builder/mod.rs:1792:13
|
1792 | let x: ();
| ^
|
= note: `#[warn(unused_variables)]` (part of `#[warn(unused)]`) on by default
help: if this is intentional, prefix it with an underscore
|
1792 | let _x: ();
| +
help: you might have meant to pattern match on the similarly named constant `_`
|
1792 - let x: ();
1792 + let utils::render_tests::_: ();
|
warning: `bootstrap` (lib) generated 1 warning (run `cargo fix --lib -p bootstrap` to apply 1 suggestion)
Finished `dev` profile [unoptimized] target(s) in 5.14s
error: warnings are denied by `build.warnings` configuration
failed to run: /Users/jyn/src/ferrocene3/build/aarch64-apple-darwin/stage0/bin/cargo build --jobs=default --manifest-path /Users/jyn/src/ferrocene3/src/bootstrap/Cargo.toml -Zroot-dir=/Users/jyn/src/ferrocene3 -Zwarnings
```
</details>
building the compiler from scratch with `deny`: https://gist.github.com/jyn514/493ed26c2aa6f61bf47c21e75efa2175
<details><summary>and rebuilding the compiler after switching from deny->warn (note that it reuses the whole cache, there are no invalidations):</summary>
```
$ x c compiler
Building bootstrap
Finished `dev` profile [unoptimized] target(s) in 0.15s
Checking stage1 compiler artifacts{rustc-main, rustc_abi, rustc_arena, rustc_ast, rustc_ast_ir, rustc_ast_lowering, rustc_ast_passes, rustc_ast_pretty, rustc_attr_parsing, rustc_baked_icu_data, rustc_borrowck, rustc_builtin_macros, rustc_codegen_llvm, rustc_codegen_ssa, rustc_const_eval, rustc_data_structures, rustc_driver, rustc_driver_impl, rustc_error_codes, rustc_error_messages, rustc_errors, rustc_expand, rustc_feature, rustc_fluent_macro, rustc_fs_util, rustc_graphviz, rustc_hashes, rustc_hir, rustc_hir_analysis, rustc_hir_id, rustc_hir_pretty, rustc_hir_typeck, rustc_incremental, rustc_index, rustc_index_macros, rustc_infer, rustc_interface, rustc_lexer, rustc_lint, rustc_lint_defs, rustc_llvm, rustc_log, rustc_macros, rustc_metadata, rustc_middle, rustc_mir_build, rustc_mir_dataflow, rustc_mir_transform, rustc_monomorphize, rustc_next_trait_solver, rustc_parse, rustc_parse_format, rustc_passes, rustc_pattern_analysis, rustc_privacy, rustc_proc_macro, rustc_public, rustc_public_bridge, rustc_query_impl, rustc_query_system, rustc_resolve, rustc_sanitizers, rustc_serialize, rustc_session, rustc_span, rustc_symbol_mangling, rustc_target, rustc_thread_pool, rustc_trait_selection, rustc_traits, rustc_transmute, rustc_ty_utils, rustc_type_ir, rustc_type_ir_macros, rustc_windows_rc} (stage0 -> stage1, aarch64-apple-darwin)
warning: function `foo` is never used
--> compiler/rustc_hashes/src/lib.rs:16:4
|
16 | fn foo() {}
| ^^^
|
= note: `#[warn(dead_code)]` (part of `#[warn(unused)]`) on by default
warning: `rustc_hashes` (lib) generated 1 warning
Finished `release` profile [optimized + debuginfo] target(s) in 0.53s
Build completed successfully in 0:00:08
```
</details>
thanks to `@epage` for the help finding this!
r? bootstrap
|
Possibly failed in rollup in @bors r- |
|
@bors try jobs=test-various |
This comment has been minimized.
This comment has been minimized.
bootstrap: Use cargo's `build.warnings=deny` rather than -Dwarnings try-job: test-various
This comment has been minimized.
This comment has been minimized.
|
💔 Test for 9a6eed8 failed: CI. Failed jobs:
|
|
oh ah um hm. apparently cargo’s version of this applies to hard warnings, not just to lints. that seems not ideal? sometimes hard warnings are there because we don’t want them to break code, and because they’re not lints they can’t be allowed…. |
|
frankly I don’t like the idea of hard warnings in the first place, but given that they exist, I would not expect this behavior from cargo. |
Feel free to bring this up on rust-lang/cargo#14802. Would be good to have an example of a hard warning for use in a test and an explanation of their role and why they shouldn't be subject to the flag. |
Motivation for this: rustc is trying to adopt this feature but hard warnings are getting in the way, see rust-lang/rust#148332 Reasons to do this: - Less flexibility in how hard warnings are resolved - Matches Cargo not erroring for Cargo hard warnings Reasons not do this: - A user may see a hard warning from rustc and get confused - Cargo's hard warnings are not blocking in part because we would need to audit them to see if they should actually be hard warnings See also the discussion at rust-lang#14802 (comment)
Motivation for this: rustc is trying to adopt this feature but hard warnings are getting in the way, see rust-lang/rust#148332 Reasons to do this: - Less flexibility in how hard warnings are resolved - Matches Cargo not erroring for Cargo hard warnings Reasons not do this: - A user may see a hard warning from rustc and get confused - Cargo's hard warnings are not blocking in part because we would need to audit them to see if they should actually be hard warnings We do have some flexibility on warnings evolving over time, so this is likely a two-way door (on an unstable feature). See also the discussion at rust-lang#14802 (comment)
Motivation for this: rustc is trying to adopt this feature but hard warnings are getting in the way, see rust-lang/rust#148332 Reasons to do this: - Less flexibility in how hard warnings are resolved - Matches Cargo not erroring for Cargo hard warnings Reasons not do this: - A user may see a hard warning from rustc and get confused - Cargo's hard warnings are not blocking in part because we would need to audit them to see if they should actually be hard warnings We do have some flexibility on warnings evolving over time, so this is likely a two-way door (on an unstable feature). See also the discussion at rust-lang#14802 (comment)
Motivation for this: rustc is trying to adopt this feature but hard warnings are getting in the way, see rust-lang/rust#148332 Reasons to do this: - Less flexibility in how hard warnings are resolved - Matches Cargo not erroring for Cargo hard warnings Reasons not do this: - A user may see a hard warning from rustc and get confused - Cargo's hard warnings are not blocking in part because we would need to audit them to see if they should actually be hard warnings We do have some flexibility on warnings evolving over time, so this is likely a two-way door (on an unstable feature). See also the discussion at rust-lang#14802 (comment)
### What does this PR try to resolve? Motivation for this: rustc is trying to adopt this feature but hard warnings are getting in the way, see rust-lang/rust#148332 This is a part of #14802. ### How to test and review this PR? Reasons to do this: - Less flexibility in how hard warnings are resolved - Matches Cargo not erroring for Cargo hard warnings - This matches `-Dwarnings` behavior which this feature is meant to replace Reasons not do this: - A user may see a hard warning from rustc and get confused - Cargo's hard warnings are not blocking in part because we would need to audit them to see if they should actually be hard warnings We do have some flexibility on warnings evolving over time, so this is likely a two-way door (on an unstable feature). See also the discussion at #14802 (comment) Ideally, we'd also test for duplicate hard warnings but unsure how to trigger that.
This has two major advantages. First, it makes us less dependent on the rustc shim, which is nice but not super important. More importantly, it gives us much nicer caching properties. Previously, `x build --warnings warn && x build --warnings deny` would rebuild all of bootstrap, and if there were any warnings emitted on the last build of the compiler, they would *not* fail the build, because cargo would cache the output rather than rerunning the shim. After this change, bootstrap rebuilds instantly, and cargo knows that it should fail the build but *without* invalidating the cache.
e1e8913 to
93df0f2
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors try jobs=test-various |
|
@jyn514: 🔑 Insufficient privileges: not in try users |
|
@bors try jobs=test-various @bors2 delegate=try You can now run try builds on this PR :) |
|
✌️ @jyn514, you can now perform try builds on this pull request! You can now post |
bootstrap: Use cargo's `build.warnings=deny` rather than -Dwarnings try-job: test-various
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
oh, this is still using bootstrap cargo :/ time to wait 6 weeks i guess next release is December 11 |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test for 8f18f3f failed: CI. Failed jobs:
|
|
@rustbot label -S-waiting-on-author +S-blocked |
This has two major advantages. First, it makes us less dependent on the rustc shim, which is nice but not super important. More importantly, it gives us much nicer caching properties.
Previously,
x build --warnings warn && x build --warnings denywould rebuild all of bootstrap, and if there were any warnings emitted on the last build of the compiler, they would not fail the build, because cargo would cache the output rather than rerunning the shim.After this change, bootstrap rebuilds instantly, and cargo knows that it should fail the build but without invalidating the cache.
An example of rebuilding bootstrap after a switch from warn->deny:
building the compiler from scratch with
deny: https://gist.github.com/jyn514/493ed26c2aa6f61bf47c21e75efa2175and rebuilding the compiler after switching from deny->warn (note that it reuses the whole cache, there are no invalidations):
thanks to @epage for the help finding this!
r? bootstrap