SPMI: Pass explicit environments to subprocesses instead of mutating os.environ - #134508
Open
jakobbotsch wants to merge 1 commit into
Open
jakobbotsch wants to merge 1 commit into
jakobbotsch wants to merge 1 commit into
Conversation
…os.environ
superpmi.py modified os.environ to pass environment variables to collection
subprocesses, and restored it afterwards with os.environ.clear() followed by
os.environ.update(saved). AsyncSubprocessHelper.run_to_completion also did this
defensively around every run.
That restore is lossy on Windows: assigning an empty string to an os.environ key
calls _wputenv("NAME="), which removes the variable from the process
environment. Environment variables with empty values were therefore silently
dropped for all subsequent child processes. For example, with
GIT_CONFIG_COUNT/GIT_CONFIG_KEY_n/GIT_CONFIG_VALUE_n where one value is empty,
every later git invocation failed with "unable to parse command-line config",
and the asm diffs summary showed "No diffs found?" for all examples.
Instead, pass the environment explicitly via env= to the PMI, crossgen2 and
NativeAOT collection subprocesses (the dicts are already based on a copy of
os.environ), and remove all os.environ mutation.
Also make the example generation in the asm diffs summary report git diff
failures instead of treating them as "No diffs found?".
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ef38ea02-2a4e-45b5-8127-86b751617a68
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Decode Git stderr with replacement handling to prevent reporting failures from aborting on non-UTF-8 output.
Review effort: Lite
Findings: None
What changed in this PR
This PR avoids mutating os.environ for SuperPMI subprocesses and improves Git diff failure reporting.
Changes:
- Passes explicit environment dictionaries to collection subprocesses.
- Removes lossy environment restoration.
- Reports Git diff failures in asm-diff summaries.
| File | Description |
|---|---|
src/coreclr/scripts/superpmi.py |
Updates subprocess environment handling and Git diff diagnostics. |
Member
Author
|
/azp run runtime |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Member
Author
|
PTAL @dotnet/jit-contrib |
adamperlin
approved these changes
Sep 25, 2026
adamperlin
left a comment
Contributor
There was a problem hiding this comment.
LGTM, passing through an env explicitly like this seems less error prone all around I think.
This branch has not been deployed
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
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.
Mutating
os.environin this way is lossy -- when you add an environment variable with an empty string as a value, it is equivalent to deleting that environment variable on Windows. Subprocesses will not see the environment variable set at all.I hit this for superpmi asmdiffs reports generated through copilot. Copilot apparently sets
GIT_CONFIG_VALUE_Nand this environment mutation got rid of some of those environment variables which resulted in git errors in subprocesses.Also make the example generation in the asm diffs summary report git diff failures instead of treating them as "No diffs found?".