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

Make more reads of environment variables go through the Config #11754

Merged
merged 2 commits into from
Feb 22, 2023
Merged

Make more reads of environment variables go through the Config #11754

merged 2 commits into from
Feb 22, 2023

Conversation

kylematsuda
Copy link
Contributor

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.

@rustbot
Copy link
Collaborator

rustbot commented Feb 21, 2023

r? @ehuss

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

@rustbot rustbot added A-build-execution Area: anything dealing with executing the compiler A-build-scripts Area: build.rs scripts A-cache-messages Area: caching of compiler messages A-configuration Area: cargo config files and env vars A-dependency-resolution Area: dependency resolution and the resolver A-features2 Area: issues specifically related to the v2 feature resolver A-rebuild-detection Area: rebuild detection and fingerprinting S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2023
@ehuss
Copy link
Contributor

ehuss commented Feb 22, 2023

Thanks! I think I would feel more comfortable leaving util::profile alone. That is a purely internal debugging facility that I don't think we need to abstract away. I'm a little concerned about the scope of changes needed to update it. Since this endeavor is mostly related to potentially consider the interaction of the [env] config table, I don't think that would be needed. Would you mind backing out that particular change?

@kylematsuda
Copy link
Contributor Author

For sure, sounds good! Just pushed a new version without the changes to util::profile.

@ehuss
Copy link
Contributor

ehuss commented Feb 22, 2023

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 22, 2023

📌 Commit 5035cb2 has been approved by ehuss

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 Feb 22, 2023
@bors
Copy link
Contributor

bors commented Feb 22, 2023

⌛ Testing commit 5035cb2 with merge 239ff78...

@bors
Copy link
Contributor

bors commented Feb 22, 2023

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 239ff78 to master...

@bors bors merged commit 239ff78 into rust-lang:master Feb 22, 2023
weihanglo added a commit to weihanglo/rust that referenced this pull request Feb 23, 2023
@weihanglo
Update cargo
80bea6c 
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)
@weihanglo weihanglo mentioned this pull request Feb 23, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2023
@bors
Auto merge of rust-lang#108330 - weihanglo:update-cargo, r=weihanglo
8b1dbf7 
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)
@ehuss ehuss added this to the 1.69.0 milestone Mar 9, 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
No reviews
Assignees

@ehuss ehuss

Labels
A-build-execution Area: anything dealing with executing the compiler A-build-scripts Area: build.rs scripts A-cache-messages Area: caching of compiler messages A-configuration Area: cargo config files and env vars A-dependency-resolution Area: dependency resolution and the resolver A-features2 Area: issues specifically related to the v2 feature resolver A-rebuild-detection Area: rebuild detection and fingerprinting S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.69.0
Development

Successfully merging this pull request may close these issues.
4 participants
@kylematsuda @rustbot @ehuss @bors