-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Conversation
compiler-builtins
test to not fully clear cargo command env varscompiler-builtins
test to not clear essential env vars
Do you know why we have that |
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 |
☔ The latest upstream changes (presumably #125628) made this pull request unmergeable. Please resolve the merge conflicts. |
60312ed
to
f31f59e
Compare
@bors try |
@bors ping are you home? |
😪 I'm awake I'm awake |
…<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
I'm guessing bors wasn't trying very hard? @bors try |
…<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
@saethlin do you happen to remember in which CI job that |
☀️ Try build successful - checks-actions |
Test successful :ferrisClueless: |
f31f59e
to
2949195
Compare
We could try to see if this still fails in full build CI? |
Seems to pass locally too. Would be neat if this is the only change required. @bors r+ |
☀️ Test successful - checks-actions |
Nice |
Finished benchmarking commit (23e040a): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
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.
CyclesResults (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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 668.226s -> 667.724s (-0.08%) |
Lol, improvements |
Noticed in #122580 (comment), the
compiler-builtins
test failed on Windows for acargo
invocation because necessary env varsTMP
andTEMP
were cleared byCommand::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