Skip to content

Conversation

giraffate
Copy link
Contributor

Fix #6041

This lint shouldn't be emitted in build.rs as println! and print! are used for the build script.

changelog: none

This lint shouldn't be emitted in `build.rs` as `println!` and `print!` are used for the build script.
@rust-highfive
Copy link

r? @yaahc

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 24, 2020
Copy link
Contributor

@ebroto ebroto left a comment

Choose a reason for hiding this comment

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

I left some comments regarding the detection of build scripts

@ebroto
Copy link
Contributor

ebroto commented Sep 24, 2020

r? @ebroto

@rust-highfive rust-highfive assigned ebroto and unassigned yaahc Sep 24, 2020
@ebroto ebroto added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 25, 2020
@giraffate
Copy link
Contributor Author

@ebroto Thanks for your review! I addressed these.

Copy link
Contributor

@ebroto ebroto left a comment

Choose a reason for hiding this comment

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

Besides the nits, two things:

  • tests/ui/print_stdout_build_script.stderr can be removed, sorry I did not see that in my first review.
  • I was thinking that we should add an integration test also to make sure the lint actually ignores real build scripts. Could you please add a build.rs file under clippy_workspace_tests similar to the print_stdout_build_script.rs that you added (also referencing the issue)? This will be tested by the dogfood tests.

@giraffate
Copy link
Contributor Author

I was thinking that we should add an integration test also to make sure the lint actually ignores real build scripts. Could you please add a build.rs file under clippy_workspace_tests similar to the print_stdout_build_script.rs that you added (also referencing the issue)? This will be tested by the dogfood tests.

Thanks for your review. It's a good suggestion. However, I tried it but the build script under clippy_workspace_tests didn't work in the dogfood tests for some reason.

Copy link
Contributor

@ebroto ebroto left a comment

Choose a reason for hiding this comment

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

I think the only missing thing was the deny attribute. I'll merge this if the tests pass.

Thanks for making Clippy better!

@ebroto
Copy link
Contributor

ebroto commented Sep 26, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Sep 26, 2020

📌 Commit 1214a85 has been approved by ebroto

@bors
Copy link
Contributor

bors commented Sep 26, 2020

⌛ Testing commit 1214a85 with merge b64d21d...

@bors
Copy link
Contributor

bors commented Sep 26, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto
Pushing b64d21d to master...

@bors bors merged commit b64d21d into rust-lang:master Sep 26, 2020
@giraffate giraffate deleted the print_stdout_in_build_rs branch September 28, 2020 13:40
bors added a commit that referenced this pull request Oct 12, 2020
Remove the generated files by `update-references.sh` if they are empty

An empty file may be generated by `update-references.sh` and committed as is when creating a patch like #6101 (comment) and #6079 (review). So, I think it would be helpful to add documentation, and automatically remove the generated file if it's empty.

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

print_stdout should not look in build.rs
5 participants