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

jieyouxu · 2024-05-22T23:21:55Z

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

src/tools/run-make-support/src/lib.rs

saethlin · 2024-05-23T00:38:55Z

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 · 2024-05-23T00:56:16Z

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

bors · 2024-05-27T23:07:48Z

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

jieyouxu · 2024-06-03T01:58:14Z

@bors try

jieyouxu · 2024-06-03T02:05:55Z

@bors ping are you home?

bors · 2024-06-03T02:05:57Z

😪 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

bors · 2024-06-03T02:19:36Z

⌛ Trying commit f31f59e with merge 932d86e...

jieyouxu · 2024-06-04T06:28:47Z

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

@bors try

bors · 2024-06-04T06:29:57Z

⌛ Trying commit f31f59e with merge d12928a...

…<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 · 2024-06-04T06:30:53Z

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 · 2024-06-04T08:00:29Z

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

jieyouxu · 2024-06-04T08:01:53Z

Test successful :ferrisClueless:

jieyouxu · 2024-06-04T08:05:22Z

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

saethlin · 2024-06-04T18:49:21Z

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

@bors r+

bors · 2024-06-04T18:49:24Z

📌 Commit 2949195 has been approved by saethlin

It is now in the queue for this repository.

bors · 2024-06-04T19:14:31Z

⌛ Testing commit 2949195 with merge 23e040a...

bors · 2024-06-04T21:20:11Z

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

saethlin · 2024-06-04T21:45:25Z

Nice

rust-timer · 2024-06-05T00:39:56Z

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 · 2024-06-05T01:05:23Z

Lol, improvements

rustbot assigned saethlin May 22, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2024

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

jieyouxu commented May 22, 2024

View reviewed changes

src/tools/run-make-support/src/lib.rs Outdated Show resolved Hide resolved

jieyouxu mentioned this pull request May 22, 2024

"Handle" calls to upstream monomorphizations in compiler_builtins #122580

Merged

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

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

compiler-builtins: don't clear env vars for cargo invocation

2949195

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

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 added the merged-by-bors This PR was explicitly merged by bors. label Jun 4, 2024

bors merged commit 23e040a into rust-lang:master Jun 4, 2024

rustbot added this to the 1.80.0 milestone Jun 4, 2024

jieyouxu deleted the rmake-support-env-reset branch June 5, 2024 02:31

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

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

Uh oh!

Conversation

jieyouxu commented May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

saethlin commented May 23, 2024

Uh oh!

jieyouxu commented May 23, 2024

Uh oh!

bors commented May 27, 2024

Uh oh!

jieyouxu commented Jun 3, 2024

Uh oh!

jieyouxu commented Jun 3, 2024

Uh oh!

bors commented Jun 3, 2024

Uh oh!

bors commented Jun 3, 2024

Uh oh!

jieyouxu commented Jun 4, 2024

Uh oh!

bors commented Jun 4, 2024

Uh oh!

jieyouxu commented Jun 4, 2024

Uh oh!

bors commented Jun 4, 2024

Uh oh!

jieyouxu commented Jun 4, 2024

Uh oh!

jieyouxu commented Jun 4, 2024

Uh oh!

saethlin commented Jun 4, 2024

Uh oh!

bors commented Jun 4, 2024

Uh oh!

bors commented Jun 4, 2024

Uh oh!

bors commented Jun 4, 2024

Uh oh!

saethlin commented Jun 4, 2024

Uh oh!

rust-timer commented Jun 5, 2024

Overall result: ✅ improvements - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

saethlin commented Jun 5, 2024

Uh oh!

Uh oh!

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

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

jieyouxu commented May 22, 2024 •

edited

Loading