Changing PATH is a no-op in non-windows platforms.#10094
Changing PATH is a no-op in non-windows platforms.#10094MrSoup678 wants to merge 2 commits intoFacepunch:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts launcher environment setup so that PATH mutation is only applied on Windows, avoiding ineffective (and potentially confusing) behavior on other platforms.
Changes:
- Gate PATH-prepending logic behind
OperatingSystem.IsWindows()inUpdateNativeDllPath. - Add an extra
using(currently unused) related to interop.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var path = System.Environment.GetEnvironmentVariable( "PATH" ); | ||
| path = $"{nativeDllPath};{path}"; | ||
| System.Environment.SetEnvironmentVariable( "PATH", path ); | ||
| if (OperatingSystem.IsWindows()) |
There was a problem hiding this comment.
Formatting is inconsistent with the surrounding codebase style (spaces inside parentheses): if (OperatingSystem.IsWindows()) vs if ( OperatingSystem.IsWindows() ) used elsewhere in this file. Please align the spacing for consistency (and to match any .editorconfig formatting rules).
| if (OperatingSystem.IsWindows()) | |
| if ( OperatingSystem.IsWindows() ) |
| if (OperatingSystem.IsWindows()) | ||
| { | ||
| var path = System.Environment.GetEnvironmentVariable( "PATH" ); | ||
| path = $"{nativeDllPath};{path}"; | ||
| System.Environment.SetEnvironmentVariable( "PATH", path ); | ||
| } |
There was a problem hiding this comment.
The comment above this block implies we’re adjusting lookup behavior unconditionally, but this now only happens on Windows. Consider updating the comment to explicitly note the Windows-only PATH modification (and rely on NetCore.NativeDllPath for other platforms) to avoid misleading future readers.
| @@ -1,5 +1,6 @@ | |||
| using System; | |||
| using System.Reflection; | |||
| using System.Runtime.InteropServices; | |||
There was a problem hiding this comment.
using System.Runtime.InteropServices; appears unused in this file (no references to RuntimeInformation, NativeLibrary, DllImport, etc.). Please remove it to avoid analyzer warnings and keep the import list minimal.
| using System.Runtime.InteropServices; |
handsomematt
added
the
triaged
|
This PR has been merged upstream. |
Pull Request
Thanks for contributing to s&box ❤️
Please fill out the sections below to help us review your change efficiently.
Summary
Motivation & Context
Fixes:
Implementation Details
Screenshots / Videos (if applicable)
Checklist