Read environment variables through Config instead of std::env::var(_os)#11727
Merged
bors merged 5 commits intorust-lang:masterfrom Feb 21, 2023
Merged
Read environment variables through Config instead of std::env::var(_os)#11727bors merged 5 commits intorust-lang:masterfrom
Config instead of std::env::var(_os)#11727bors merged 5 commits intorust-lang:masterfrom
Conversation
Collaborator
|
r? @ehuss (rustbot has picked a reviewer for you, use r? to override) |
…get_env and get_env_os
…e config is available
e34dbe5 to
a148cd4
Compare
ehuss
reviewed
Feb 17, 2023
Contributor
ehuss
left a comment
There was a problem hiding this comment.
Thanks for getting this started!
Comment on lines
+781
to
+786
| /// Get the value of environment variable `key` through the `Config` snapshot. | ||
| /// | ||
| /// This can be used similarly to `std::env::var_os`. | ||
| pub fn get_env_os(&self, key: impl AsRef<str>) -> Option<String> { | ||
| self.env.get(key.as_ref()).cloned() | ||
| } |
Contributor
There was a problem hiding this comment.
This might be a little confusing since it is not returning an OsString, which is what the _os suffix implies.
The lack of OsString support also seems like a regression in the cases where var_os is used. Cargo has fairly poor support for non-UTF-8 paths and environment, but I think it would be good to try to avoid making it worse.
How hard would it be to store OsString in the map instead of String?
Contributor
Author
There was a problem hiding this comment.
Oops yes good point! I'll investigate what else needs to change if Config.env is a HashMap<OsString, OsString> instead.
Contributor
Author
There was a problem hiding this comment.
Ok, this turned out to be not too bad to implement (I think). Please see the most recent commit. I mainly had to add/edit a few utility methods on Config to handle invalid UTF-8 entries in self.env.
Contributor
|
Thanks! I'm pretty sure (though not certain) that there shouldn't be any behavioral changes here. 🤞 @bors r+ |
Contributor
Contributor
Contributor
|
☀️ Test successful - checks-actions |
This was referenced Feb 21, 2023
bors
added a commit
that referenced
this pull request
Feb 22, 2023
Make more reads of environment variables go through the `Config` As discussed in #11588, it's better to make reads of environment variables go through the `Config` instead of calling `std::env::var` or `env::var_os` everywhere. Most of the "easy" places to change this in `src/` were handled in #11727. This PR fixes a few remaining call sites that were missed in #11727, namely: - `LocalFingerprint::find_stale_item` - `util::profile::start` and `Profiler` - `util::rustc::rustc_fingerprint` After doing this, there are a few remaining calls to `env::var` in `src/` but those probably require more discussion to fix.
weihanglo
added a commit
to weihanglo/rust
that referenced
this pull request
Feb 23, 2023
15 commits in 17b3d0de0897e1c6b8ca347bd39f850bb0a5b9f6..9d5b32f503fc099c4064298465add14d4bce11e6 2023-02-17 19:45:09 +0000 to 2023-02-22 23:04:16 +0000 - refactor(job_queue): docs and move types around (rust-lang/cargo#11758) - Scrub more of the test environment (rust-lang/cargo#11757) - Make more reads of environment variables go through the `Config` (rust-lang/cargo#11754) - Revert "Update curl-sys to use libcurl 7.88.1" (rust-lang/cargo#11755) - use consistent case (rust-lang/cargo#11748) - Switch some tests from `build` to `check` (rust-lang/cargo#11725) - Fix typo in sparse-registry warning message (rust-lang/cargo#11753) - reuse url encoding from `url` crate, don't use separate `percent-encoding` (rust-lang/cargo#11750) - Read environment variables through `Config` instead of `std::env::var(_os)` (rust-lang/cargo#11727) - Update curl-sys to use libcurl 7.88.1 (rust-lang/cargo#11749) - mdman: update pretty_assertions to reduce deps (rust-lang/cargo#11747) - Cleanup tests (rust-lang/cargo#11745) - Enhance help texts of position args (rust-lang/cargo#11740) - Fix typo (rust-lang/cargo#11741) - Update comment about cargo-ok (rust-lang/cargo#11724)
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 23, 2023
Update cargo 15 commits in 17b3d0de0897e1c6b8ca347bd39f850bb0a5b9f6..9d5b32f503fc099c4064298465add14d4bce11e6 2023-02-17 19:45:09 +0000 to 2023-02-22 23:04:16 +0000 - refactor(job_queue): docs and move types around (rust-lang/cargo#11758) - Scrub more of the test environment (rust-lang/cargo#11757) - Make more reads of environment variables go through the `Config` (rust-lang/cargo#11754) - Revert "Update curl-sys to use libcurl 7.88.1" (rust-lang/cargo#11755) - use consistent case (rust-lang/cargo#11748) - Switch some tests from `build` to `check` (rust-lang/cargo#11725) - Fix typo in sparse-registry warning message (rust-lang/cargo#11753) - reuse url encoding from `url` crate, don't use separate `percent-encoding` (rust-lang/cargo#11750) - Read environment variables through `Config` instead of `std::env::var(_os)` (rust-lang/cargo#11727) - Update curl-sys to use libcurl 7.88.1 (rust-lang/cargo#11749) - mdman: update pretty_assertions to reduce deps (rust-lang/cargo#11747) - Cleanup tests (rust-lang/cargo#11745) - Enhance help texts of position args (rust-lang/cargo#11740) - Fix typo (rust-lang/cargo#11741) - Update comment about cargo-ok (rust-lang/cargo#11724)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR try to resolve?
Adds two new methods
get_envandget_env_osfor reading environment variables from theConfigand uses them to replace the many of the calls tostd::env::varandvar_os(see the discussion in #11588).Closes #11588