TSCBasic: avoid using C library functions for environment #452
Conversation
compnerd
requested review from
abertelrud,
neonichu,
tomerd and
MaxDesiatov
as code owners
|
@swift-ci please test |
|
Would it be possible to keep the old code in addition to the new? Or will that cause interaction issues? Basically just wondering if we can avoid breaking possible assumptions of existing clients. |
|
I don't know what the assumptions are around this, but this brings it inline with Foundation which uses the Win32 copy. I don't think that we could ever guarantee the synchronisation since a user can call either function themselves and we are then out of sync. |
|
I mean, yes, I can add that but not run it. The TSC test suite is not windows friendly and doesn't run ever (not even locally like the driver or SPM). |
|
I understand, but probably easier to add that test now in an anticipation of the test suite being enabled eventually, than to hope we'll remember to cover it later. |
Windows is case insensitive for the environment control block, which we did not honour. This causes issues when Swift is used with programs which are incorrectly cased (e.g. emacs). Introduce an explicit wrapper type for Windows to make the lookup case insensitive, canonicalising the name to lowercase. This allows us to treat Path and PATH identically (along with any other environment variable and case matching) which respects the Windows behaviour. Additionally, migrate away from the POSIX variants which do not handle the case properly to the Windows version which does. The introduced type `ProcessEnvironmentBlock` is just a typealias, allowing access to the dictionary itself which is important to preserve the behaviour for the clients. The `CaseInsensitiveString` is a case insensitive, case preserving string representation that allows us to recreate the environment on Windows as the environment is case insensitive, so it should not be possible to have conflicts when reading the Process Environment Block from the Process Execution Block. This is a partial resolution to environment variable handling as the tools need subsequent changes to adopt the new API. Fixes: swiftlang#446 Co-authored-by: Max Desiatov <[email protected]>
|
Ugh, okay, so this doesn't work as intended because Foundation actually is calling into CoreFoundation which is using |
Prefer to use the Win32 APIs for environment management. Note that this is going to break any user of libc on Windows. Setting environment variables through this function will not be reflected in the C library only the Win32 APIs. Furthermore, we use the unicode variants always as the unicode and ANSI environment may diverge as not all unicode (UTF-16) is translatable to ANSI, which is documented at [1]. [1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/getenv-wgetenv?view=msvc-170&viewFallbackFrom=vs-2019.
|
@MaxDesiatov added a test, but to do this properly, we now need to do the storage change first otherwise we will have to change this again with the that. |
Prefer to use the Win32 APIs for environment management. Note that this is going to break any user of libc on Windows. Setting environment variables through this function will not be reflected in the C library only the Win32 APIs. Furthermore, we use the unicode variants always as the unicode and ANSI environment may diverge as not all unicode (UTF-16) is translatable to ANSI, which is documented at [1].
[1] https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/getenv-wgetenv?view=msvc-170&viewFallbackFrom=vs-2019.