-
Notifications
You must be signed in to change notification settings - Fork 13
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
Runtime envdef handles case-insensitive environment variable names on Windows. #3553
Conversation
mitchell-as
commented
Oct 22, 2024
•
edited by github-actions
bot
Loading
edited by github-actions
bot
DX-3030 Handle/merge PATH case insensitively on Windows |
296f6f7
to
7f14bdd
Compare
Test failures are known issues and unrelated to this PR. |
We read the case-insensitive version as needed, but replace it with our case-sensitive version (e.g. "Path" -> "PATH").
7f14bdd
to
7653324
Compare
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.
In addition to the comments, can we make sure there is a test case covering this mechanic (specifically: PATH and Path on Windows).
if pev.Name != osName { | ||
// On Windows, delete the case-insensitive version. Our case-sensitive version has already | ||
// processed the value of the case-insensitive version. | ||
delete(res, osName) | ||
} |
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.
if pev.Name != osName { | |
// On Windows, delete the case-insensitive version. Our case-sensitive version has already | |
// processed the value of the case-insensitive version. | |
delete(res, osName) | |
} | |
if pev.Name != osName { | |
// Delete the redundant value that ours could conflict with | |
// Eg. this would delete "Path" while preserving our "PATH" | |
delete(res, osName) | |
} |
26bd059
to
fee33f5
Compare
fee33f5
to
501cb62
Compare