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

Update compiler-builtins test to not clear essential env vars #125426

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented May 22, 2024

Noticed in #122580 (comment), the compiler-builtins test failed on Windows for a cargo invocation because necessary env vars TMP and TEMP were cleared by Command::env_clear, causing temp dir eventually used by codegen to fallback to the Windows directory, which will trigger permission errors.

This PR removes the env_clear on the cargo invocation.

r? @saethlin (feel free to reroll, since you authored the test)

try-job: x86_64-msvc
try-job: test-various

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2024
@jieyouxu jieyouxu changed the title Update compiler-builtins test to not fully clear cargo command env vars Update compiler-builtins test to not clear essential env vars May 22, 2024
@saethlin
Copy link
Member

Do you know why we have that env_clear() in the first place? The test passes fine locally without it, maybe CI has an issue? I think based on the issue report, I'd prefer that we do the inverse strategy of what you're doing here; instead of trying to allow through specific environment variables that matter on specific platforms, instead drop the one (if any) specific variables that cause problems.

@jieyouxu
Copy link
Member Author

Do you know why we have that env_clear() in the first place? The test passes fine locally without it, maybe CI has an issue? I think based on the issue report, I'd prefer that we do the inverse strategy of what you're doing here; instead of trying to allow through specific environment variables that matter on specific platforms, instead drop the one (if any) specific variables that cause problems.

Yeah, I should dig more into why we have issues with that test in the first place. Removing problematic env vars make sense to me.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 23, 2024
@bors
Copy link
Contributor

bors commented May 27, 2024

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

@rust-cloud-vms rust-cloud-vms bot force-pushed the rmake-support-env-reset branch from 60312ed to f31f59e Compare June 3, 2024 01:57
@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 3, 2024

@bors try

@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 3, 2024

@bors ping are you home?

@bors
Copy link
Contributor

bors commented Jun 3, 2024

😪 I'm awake I'm awake

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 3, 2024
…<try>

Update `compiler-builtins` test to not clear essential env vars

Noticed in rust-lang#122580 (comment), the `compiler-builtins` test failed on Windows for a `cargo` invocation because necessary env vars `TMP` and `TEMP` were cleared by `Command::env_clear`, causing temp dir eventually used by codegen to fallback to the Windows directory, which will trigger permission errors.

This PR adds a `clear_non_essential_env_vars` helper, which is a more conservative `Command::env_clear` that does not clear `TMP` or `TEMP` on Windows, and does not clear `TMPDIR` on non-Windows platforms.

cc `@ChrisDenton` do you happen to know if there are any more "essential" env vars that we should not clear (on Windows or other platforms)?

r? `@saethlin` (feel free to reroll, since you authored the test)

try-job: x86_64-msvc
try-job: test-various
@bors
Copy link
Contributor

bors commented Jun 3, 2024

⌛ Trying commit f31f59e with merge 932d86e...

@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 4, 2024

I'm guessing bors wasn't trying very hard?

@bors try

@bors
Copy link
Contributor

bors commented Jun 4, 2024

⌛ Trying commit f31f59e with merge d12928a...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2024
…<try>

Update `compiler-builtins` test to not clear essential env vars

Noticed in rust-lang#122580 (comment), the `compiler-builtins` test failed on Windows for a `cargo` invocation because necessary env vars `TMP` and `TEMP` were cleared by `Command::env_clear`, causing temp dir eventually used by codegen to fallback to the Windows directory, which will trigger permission errors.

This PR adds a `clear_non_essential_env_vars` helper, which is a more conservative `Command::env_clear` that does not clear `TMP` or `TEMP` on Windows, and does not clear `TMPDIR` on non-Windows platforms.

cc `@ChrisDenton` do you happen to know if there are any more "essential" env vars that we should not clear (on Windows or other platforms)?

r? `@saethlin` (feel free to reroll, since you authored the test)

try-job: x86_64-msvc
try-job: test-various
@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 4, 2024

Do you know why we have that env_clear() in the first place? The test passes fine locally without it, maybe CI has an issue? I think based on the issue report, I'd prefer that we do the inverse strategy of what you're doing here; instead of trying to allow through specific environment variables that matter on specific platforms, instead drop the one (if any) specific variables that cause problems.

@saethlin do you happen to remember in which CI job that compiler-builtins was failing in?

@bors
Copy link
Contributor

bors commented Jun 4, 2024

☀️ Try build successful - checks-actions
Build commit: d12928a (d12928a426823eb77a709328f70fb7c4e828e390)

@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 4, 2024

Test successful :ferrisClueless:

@rust-cloud-vms rust-cloud-vms bot force-pushed the rmake-support-env-reset branch from f31f59e to 2949195 Compare June 4, 2024 08:04
@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 4, 2024

We could try to see if this still fails in full build CI?
@bors rollup=iffy (expected to probably fail)

@saethlin
Copy link
Member

saethlin commented Jun 4, 2024

Seems to pass locally too. Would be neat if this is the only change required.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 4, 2024

📌 Commit 2949195 has been approved by saethlin

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 4, 2024
@bors
Copy link
Contributor

bors commented Jun 4, 2024

⌛ Testing commit 2949195 with merge 23e040a...

@bors
Copy link
Contributor

bors commented Jun 4, 2024

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing 23e040a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 4, 2024
@bors bors merged commit 23e040a into rust-lang:master Jun 4, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Jun 4, 2024
@saethlin
Copy link
Member

saethlin commented Jun 4, 2024

Nice

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (23e040a): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-4.7% [-4.7%, -4.7%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 1.8%, secondary 4.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.8% [1.5%, 2.1%] 2
Regressions ❌
(secondary)
4.1% [4.1%, 4.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.8% [1.5%, 2.1%] 2

Cycles

Results (secondary -5.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-5.5% [-8.1%, -2.1%] 7
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 668.226s -> 667.724s (-0.08%)
Artifact size: 319.05 MiB -> 318.98 MiB (-0.02%)

@saethlin
Copy link
Member

saethlin commented Jun 5, 2024

Lol, improvements

@jieyouxu jieyouxu deleted the rmake-support-env-reset branch June 5, 2024 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants