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

Add cargo_warnings config to control the use of the cargo warning instruction #917

Merged
merged 24 commits into from
Jan 24, 2024

Conversation

scootermon
Copy link
Contributor

Closes: #916

I couldn't come up with a clean abstraction that doesn't needlessly bloat the code so I decided to keep it simple. Because of the lack of abstraction I'm somewhat concerned about maintainability though, as it might become harder to keep track of the warning instructions.

I also found some instances of println!s, which looked to me like they're supposed to be using the cargo warning instruction. I adjusted those, but let me know if that's incorrect.

src/lib.rs Outdated
@@ -2382,7 +2405,7 @@ impl Build {
enum ArchSpec {
Device(&'static str),
Simulator(&'static str),
Catalyst(&'static str),
Catalyst(#[allow(dead_code)] &'static str),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is only here because the new nightly versions correctly detect that this field is never used, which causes the CI to fail.

Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

Can you please add a test for which warnings are disabled?

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
cc-test/build.rs Outdated Show resolved Hide resolved
@NobodyXu
Copy link
Collaborator

NobodyXu commented Jan 20, 2024

@scootermon if it's too hard to add testing, I'm ok with only test them on unix (since tests failed on windows)

@scootermon
Copy link
Contributor Author

@scootermon if it's too hard to add testing, I'm ok with only test them on unix (since tests failed on windows)

I added testing in the latest commits and there shouldn't be any problems on Windows because I'm not actually forking the process. The CI is failing because the tests are working :) We're still writing the warning "Compiler version doesn't include clang or GCC" to the console (because that's handled in Tool not in Build).
I took some time off to think about how to deal with it but I would like to hear your input on it:

  1. allow specific warnings to be written in the unit test
  2. pass down the cargo_warnings flag into Tool to suppress it. Not as clean because we can't use the Build::print_warning method.

@NobodyXu
Copy link
Collaborator

NobodyXu commented Jan 20, 2024

I suppose we should pass down the cargo_warnings flag to `Tool.

I suppose we can add a new type:

struct CargoWarning(bool);

impl CargoWarning {
    fn print_warning(&self, msg: &dyn Display) { .. }
}

So that the print_warning can be shared between Build and Tool.

@scootermon
Copy link
Contributor Author

@NobodyXu, apologies for the delay, got caught up with other stuff.

I did end up having to skip the warnings_on test case on MSVC though because cl.exe apparently doesn't write compilation warnings to stderr...

cc-test/tests/output.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
scootermon and others added 3 commits January 24, 2024 11:20
Co-authored-by: Jiahao XU <Jiahao_XU@outlook.com>
Co-authored-by: Jiahao XU <Jiahao_XU@outlook.com>
Co-authored-by: Jiahao XU <Jiahao_XU@outlook.com>
src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Jiahao XU <Jiahao_XU@outlook.com>
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

Can you also resolve the conflict please?

@NobodyXu NobodyXu merged commit 8cf5455 into rust-lang:main Jan 24, 2024
18 checks passed
@NobodyXu
Copy link
Collaborator

Thank you!

@scootermon scootermon deleted the disable-warnings branch January 25, 2024 00:57
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.

Allow suppression of cargo warning instructions in output
2 participants