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.

Sign up for GitHub

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

Jump to bottom

clippy: warn disallowed_methods for std::env::var and friends #11828

Merged
merged 3 commits into from
Mar 17, 2023
Merged

clippy: warn disallowed_methods for std::env::var and friends #11828

merged 3 commits into from
Mar 17, 2023

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented Mar 10, 2023 •
edited
Loading

What does this PR try to resolve?

In #11588 we want to avoid reading from environment variables, #11727 did that well. I wonder if we could leverage tools to help with this. Thankfully, clippy has a disallowed_methods lint, helping us enforce the rule.

I am trying to dogfood tools from rust-lang org :)

How should we test and review this PR?

Before staring a review, let's make decisions on these topics.

We need to scrutinize each usage of std::env::var and std::env::var_os. Please review comments above each usage of those.

Additional information

Some variables are read from snapshot starting from #11727 might need double checks. See #11588 (comment).

@rustbot
Copy link
Collaborator

rustbot commented Mar 10, 2023

r? @ehuss

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

@rustbot rustbot added A-build-scripts Area: build.rs scripts A-console-output Area: Terminal output, colors, progress bar, etc. A-dependency-resolution Area: dependency resolution and the resolver A-rebuild-detection Area: rebuild detection and fingerprinting A-unstable Area: nightly unstable support Command-fix S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 10, 2023
@weihanglo weihanglo force-pushed the clippy-disallowed-methods branch 2 times, most recently from a00a551 to 290c0dd Compare March 10, 2023 13:06
@rustbot rustbot added the A-configuration Area: cargo config files and env vars label Mar 10, 2023
epage
epage reviewed Mar 10, 2023
View reviewed changes
src/cargo/ops/fix.rs Outdated Show resolved Hide resolved
src/cargo/util/profile.rs Outdated Show resolved Hide resolved
src/cargo/core/features.rs Show resolved Hide resolved
weihanglo
weihanglo commented Mar 10, 2023
View reviewed changes
Copy link
Member Author

@weihanglo weihanglo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think we need to make decisions on these things (in this order):

  • Should we leverage clippy::disallowed_methods in our codebase?
  • Do we want to have a centralized place for exceptions of env var access?

src/cargo/core/compiler/custom_build.rs Show resolved Hide resolved
src/cargo/core/compiler/fingerprint/mod.rs Outdated Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented Mar 10, 2023

Should we leverage clippy::disallowed_methods in our codebase?

I think it can be a useful tool

Do we want to have a centralized place for exceptions of env var access?

I lean towards "no" but am fine if we do end up centralizing. Its also something we can always change later with minimal impact, so its not a big deal either way.

@weihanglo weihanglo added the T-cargo Team: Cargo label Mar 10, 2023
@weihanglo
Copy link
Member Author

Before starting the scrutiny of all environment variable access. Let's see if we agree on this:

@rfcbot poll T-cargo "Should we leverage clippy::disallowed_methods in our codebase?"

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 10, 2023 •
edited by Muscraft
Loading

Team member @weihanglo has asked teams: T-cargo, for consensus on:

"Should we leverage clippy::disallowed_methods in our codebase?"

@bors
Copy link
Contributor

bors commented Mar 17, 2023

☔ The latest upstream changes (presumably #11824) made this pull request unmergeable. Please resolve the merge conflicts.

@weihanglo
clippy: warn disallowed_methods for std::env::var and friends
9e3bb85 
In 11588 we want to avoid reading from environment variables,
11727 did that well. I wonder if we could leverage tools to help
with this. Thankfully, clippy has a `disallowed_methods`[1] lint,
helping us enforce the rule.

[1]: https://rust-lang.github.io/rust-clippy/stable/index.html#disallowed_methods
@rustbot rustbot added the A-cli Area: Command-line interface, option parsing, etc. label Mar 17, 2023
@weihanglo weihanglo marked this pull request as ready for review March 17, 2023 12:58
weihanglo
weihanglo commented Mar 17, 2023
View reviewed changes
src/cargo/core/compiler/fingerprint/mod.rs
fn from_env<K: AsRef<str>>(key: K) -> LocalFingerprint {
let key = key.as_ref();
let var = key.to_owned();
let val = env::var(key).ok();
Copy link
Member Author

Choose a reason for hiding this comment

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

Confirmed that on the current master cargo, when changing variable inside [env] config table, build script won't rerun.

@weihanglo weihanglo changed the title clippy: warn on disallowed_methods std::env::{var,var_os} clippy: warn disallowed_methods for std::env::var and friends Mar 17, 2023
@weihanglo
Copy link
Member Author

This is ready to review. The examination of all usages of Config::get_env{_os} will come as a follow-up PR.

epage
epage reviewed Mar 17, 2023
View reviewed changes
build.rs Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented Mar 17, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Mar 17, 2023

📌 Commit 1071cce has been approved by epage

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 17, 2023
@bors
Copy link
Contributor

bors commented Mar 17, 2023

⌛ Testing commit 1071cce with merge 99ad42d...

@bors
Copy link
Contributor

bors commented Mar 17, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing 99ad42d to master...

@bors bors merged commit 99ad42d into rust-lang:master Mar 17, 2023
@weihanglo weihanglo deleted the clippy-disallowed-methods branch March 18, 2023 06:42
@Rustin170506 Rustin170506 mentioned this pull request Mar 18, 2023
Closed
@weihanglo weihanglo mentioned this pull request Mar 21, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2023
@bors
Auto merge of rust-lang#109463 - weihanglo:update-cargo, r=weihanglo
6dc3999 
Update cargo

11 commits in 4a3c588b1f0a8e2dc8dd8789dbf3b6a71b02ed49..15d090969743630bff549a1b068bcaa8174e5ee3
2023-03-14 14:05:36 +0000 to 2023-03-21 17:54:28 +0000
- docs(contrib): Move higher level resolver docs into doc comments (rust-lang/cargo#11870)
- docs(contrib): Pull impl info out of architecture (rust-lang/cargo#11869)
- Update curl-sys (rust-lang/cargo#11871)
- Poll loop fixes (rust-lang/cargo#11624)
- clippy: warn `disallowed_methods` for `std::env::var` and friends (rust-lang/cargo#11828)
- Add --ignore-rust-version flag to cargo install (rust-lang/cargo#11859)
- Handle case mismatches when looking up env vars in the Config snapshot (rust-lang/cargo#11824)
- align semantics of generated vcs ignore files (rust-lang/cargo#11855)
- Add more information to wait-for-publish (rust-lang/cargo#11713)
- docs: Address warnings (rust-lang/cargo#11856)
- docs(contrib): Create a file overview in the nightly docs (rust-lang/cargo#11850)
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Mar 22, 2023
@bors
Auto merge of #109463 - weihanglo:update-cargo, r=weihanglo
0ed98a9 
Update cargo

11 commits in 4a3c588b1f0a8e2dc8dd8789dbf3b6a71b02ed49..15d090969743630bff549a1b068bcaa8174e5ee3
2023-03-14 14:05:36 +0000 to 2023-03-21 17:54:28 +0000
- docs(contrib): Move higher level resolver docs into doc comments (rust-lang/cargo#11870)
- docs(contrib): Pull impl info out of architecture (rust-lang/cargo#11869)
- Update curl-sys (rust-lang/cargo#11871)
- Poll loop fixes (rust-lang/cargo#11624)
- clippy: warn `disallowed_methods` for `std::env::var` and friends (rust-lang/cargo#11828)
- Add --ignore-rust-version flag to cargo install (rust-lang/cargo#11859)
- Handle case mismatches when looking up env vars in the Config snapshot (rust-lang/cargo#11824)
- align semantics of generated vcs ignore files (rust-lang/cargo#11855)
- Add more information to wait-for-publish (rust-lang/cargo#11713)
- docs: Address warnings (rust-lang/cargo#11856)
- docs(contrib): Create a file overview in the nightly docs (rust-lang/cargo#11850)
@ehuss ehuss added this to the 1.70.0 milestone Mar 26, 2023
@weihanglo weihanglo mentioned this pull request Apr 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage left review comments

Assignees

@ehuss ehuss

Labels
A-build-scripts Area: build.rs scripts A-cli Area: Command-line interface, option parsing, etc. A-configuration Area: cargo config files and env vars A-console-output Area: Terminal output, colors, progress bar, etc. A-dependency-resolution Area: dependency resolution and the resolver A-rebuild-detection Area: rebuild detection and fingerprinting A-unstable Area: nightly unstable support Command-fix S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-cargo Team: Cargo
Projects
None yet
Milestone
1.70.0
Development

Successfully merging this pull request may close these issues.
6 participants
@weihanglo @rustbot @epage @rfcbot @bors @ehuss