-
Notifications
You must be signed in to change notification settings - Fork 5k
Use set
on Windows, not export
#116745
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
base: main
Are you sure you want to change the base?
Use set
on Windows, not export
#116745
Conversation
set
on Windows, not `export'set
on Windows, not export
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses an issue from runtime logs by correcting the command used for setting environment variables on Windows.
- Replace condition from using "$(TargetOS)" to using "$(OS)" for proper Windows detection.
- Update both Windows and non-Windows branches to ensure the appropriate command ("set" for Windows, "export" for others) is used.
Comments suppressed due to low confidence (2)
eng/testing/xunit/xunit.console.targets:16
- Ensure that the $(OS) variable is consistently set in all expected environments; if there is any risk it might be missing in some CI configurations, consider adding a fallback or additional documentation.
<SetScriptCommands Condition="'$(OS)' == 'Windows_NT'" Include="set XUNIT_HIDE_PASSING_OUTPUT_DIAGNOSTICS=1" />
eng/testing/xunit/xunit.console.targets:17
- Verify that using the $(OS) condition for non-Windows environments reliably covers all target platforms, and document any assumptions if necessary.
<SetScriptCommands Condition="'$(OS)' != 'Windows_NT'" Include="export XUNIT_HIDE_PASSING_OUTPUT_DIAGNOSTICS=1" />
This keeps getting flipped back and forth https://github.com/dotnet/runtime/pull/107349/files |
We can wait for @pavelsavara but looking in the binlog, the check for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have non trivial amount placed where we set envvars like this. We have to always duplicate the lines. Could we do better?
Wasm.Build.Tests use |
For this particular case (envvar setting) we could have
not yet sure where to set the 1st part, to be early enough for all the test cases. Edit: runtime/eng/testing/tests.props Line 30 in 3c5f74a
for WindowsShell in helix-related actions and for TestWrapperTargetsWindows in helix&arcade.
|
Found in library logs: #116695 (comment).
log
TargetOS
isbrowser
and command form should not depend on it. The change removes the error from logs.