-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Downgrade linker-warnings
to allow-by-default
#136098
Conversation
@bors r+ p=1 |
This comment has been minimized.
This comment has been minimized.
@bors r- |
This PR modifies cc @jieyouxu |
@@ -12,7 +12,7 @@ use run_make_support::rustc; | |||
fn main() { | |||
// A regular compilation should not use rust-lld by default. We'll check that by asking the | |||
// linker to display its version number with a link-arg. | |||
let output = rustc().link_arg("-Wl,-v").input("main.rs").run(); | |||
let output = rustc().arg("-Wlinker-messages").link_arg("-Wl,-v").input("main.rs").run(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @rust-lang/release it continues to make me very uncomfortable to be editing tests that i have no way of running. if this fails in 6 weeks i do not know if i will have time to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locally, you can config [rust] channel = "beta"
(or stable). For CI, you could temporarily change src/ci/channel
for a @bors try
attempt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test runs in every merge via the x86_64-gnu-stable job:
https://github.com/rust-lang-ci/rust/actions/runs/12975734650/job/36187153380
2025-01-26T16:46:32.1849227Z test [run-make] tests/run-make/rust-lld-by-default-beta-stable ... ok
Nightly recently introduced the `linker-messages` lint which prints any messages from linkers. These tests were triggering that lint because they were passing missing directories to the linker. This fixes it by creating empty directories to pass to the linker. Note that this lint will be downgraded soon via rust-lang/rust#136098, but it seemed worthwhile to fix the underlying problem. This also fixes a problem where build_script_needed_for_host_and_target was not testing for the correct command-line flags. These got lost in rust-lang#14132.
03ef802
to
e6e619e
Compare
@bors r+ |
💔 Test failed - checks-actions |
This needs more time to bake before we turn it on. Turning it on early risks people silencing the warning indefinitely, before we have the chance to make it less noisy.
e6e619e
to
97311a8
Compare
@bors r+ |
Downgrade `linker-warnings` to allow-by-default This needs more time to bake before we turn it on. Turning it on early risks people silencing the warning indefinitely, before we have the chance to make it less noisy. cc rust-lang#136096 fixes rust-lang#136086 (comment) r? `@saethlin` cc `@Noratrieb` `@bjorn3` `@rustbot` label A-linkage L-linker_messages
Rollup of 6 pull requests Successful merges: - rust-lang#135876 (fix doc for std::sync::mpmc) - rust-lang#135961 (Fix 2/4 tests skipped by opt-dist) - rust-lang#136037 (Mark all NuttX targets as tier 3 target and support the standard library) - rust-lang#136098 (Downgrade `linker-warnings` to allow-by-default) - rust-lang#136110 (Miri subtree update) - rust-lang#136117 (Subtree update of `rust-analyzer`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 6 pull requests Successful merges: - rust-lang#135876 (fix doc for std::sync::mpmc) - rust-lang#135961 (Fix 2/4 tests skipped by opt-dist) - rust-lang#136037 (Mark all NuttX targets as tier 3 target and support the standard library) - rust-lang#136098 (Downgrade `linker-warnings` to allow-by-default) - rust-lang#136110 (Miri subtree update) - rust-lang#136117 (Subtree update of `rust-analyzer`) r? `@ghost` `@rustbot` modify labels: rollup
@bors p=10 |
@bors p=10 Now go to sleep >:( |
☀️ Test successful - checks-actions |
Finished benchmarking commit (0cffe5c): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (secondary 3.7%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 773.18s -> 772.206s (-0.13%) |
Nightly recently introduced the `linker-messages` lint which prints any messages from linkers. These tests were triggering that lint because they were passing missing directories to the linker. This fixes it by creating empty directories to pass to the linker. Note that this lint will be downgraded soon via rust-lang/rust#136098, but it seemed worthwhile to fix the underlying problem. This also fixes a problem where build_script_needed_for_host_and_target was not testing for the correct command-line flags. These got lost in rust-lang#14132.
This needs more time to bake before we turn it on. Turning it on early risks people silencing the warning indefinitely, before we have the chance to make it less noisy.
cc #136096
fixes #136086 (comment)
r? @saethlin cc @Noratrieb @bjorn3
@rustbot label A-linkage L-linker_messages