TSCBasic: support case insensitivity for environment #447
Conversation
compnerd
requested review from
abertelrud,
neonichu,
tomerd and
MaxDesiatov
as code owners
|
@swift-ci please test |
compnerd
force-pushed
the
insensitive
branch
from
07b0569 to
010a72f
Compare
|
@swift-ci please test |
compnerd
force-pushed
the
insensitive
branch
from
010a72f to
9e8d570
Compare
|
@swift-ci please test |
|
@swift-ci please test |
compnerd
force-pushed
the
insensitive
branch
from
9e7ac95 to
416729d
Compare
|
@swift-ci test |
|
This will need a complementary change in swift-driver, I'll whip that up in a bit. |
|
It may break SwiftPM and/or SourceKit-LSP too, that's why deprecation of the old API and introducing a new one may be preferable. |
|
Sure, the deprecation is something to consider, but this is a problem for Windows and needs to propagate. |
|
@swift-ci please test |
|
@swift-ci please test Linux platform |
compnerd
force-pushed
the
insensitive
branch
2 times, most recently
from
e78f87f to
fdfb719
Compare
|
@MaxDesiatov I'm no longer convinced that trying to do this is valuable and we should just expose this in all its nuance to all users. We will end up having to rebuild the full |
compnerd
force-pushed
the
insensitive
branch
from
fdfb719 to
1fbfb9f
Compare
|
One more request here: this needs test cases to specify and verify all of the new API surface, since the original bug report doesn't provide self-contained reproducible test cases. Hence my confusion about desired behaviour in a different version of the change. |
|
I don't know about tests that would be useful here - this is providing the API surface that is necessary to build the dependencies - I don't think that this makes sense to provide tests for unless we just give up on the type representation and just force people to spell out the dictionary type. |
|
We have to verify that the new API surface works well in the first place. Tests codify a specification for the behavior and the API shape. For example, if you want to preserve string case sensitivity internally while allowing case insensitive lookup at the same time, as you mentioned in the other PR, tests should verify that this property holds, and will also guarantee that it doesn't regress in the future. |
compnerd
force-pushed
the
insensitive
branch
from
fa6aae5 to
b2ddcca
Compare
|
|
||
| /// Invalidate the cached env. | ||
| public static func invalidateEnv() { | ||
| _vars = ProcessInfo.processInfo.environment | ||
| _vars = ProcessEnvironmentBlock(uniqueKeysWithValues: ProcessInfo.processInfo.environment.map { (CaseInsensitiveString($0.key), $0.value) }) |
There was a problem hiding this comment.
This has to be _block to avoid the deprecation, doesn't it? Also the line is too long, so needs formatting adjustments
| _vars = ProcessEnvironmentBlock(uniqueKeysWithValues: ProcessInfo.processInfo.environment.map { (CaseInsensitiveString($0.key), $0.value) }) | |
| _block = ProcessEnvironmentBlock( | |
| uniqueKeysWithValues: ProcessInfo.processInfo.environment.map { | |
| (CaseInsensitiveString($0.key), $0.value) | |
| } | |
| ) |
There was a problem hiding this comment.
I don't think it does, I don't see any deprecation warnings. Note that I left the backing store named _vars. I can change it to _block if you prefer.
| } | ||
| } | ||
| #else | ||
| public typealias ProcessEnvironmentBlock = [String:String] |
There was a problem hiding this comment.
I'm unsure if this is a good name, since AFAIU this is specific to Windows NT. Is there something more generic we could use instead?
There was a problem hiding this comment.
We already have a mixture of env and environment, I think that this is a reasonable alternative.
compnerd
force-pushed
the
insensitive
branch
3 times, most recently
from
1978ba3 to
f57674f
Compare
compnerd
force-pushed
the
insensitive
branch
from
f57674f to
c23ba35
Compare
|
@swift-ci please test |
compnerd
force-pushed
the
insensitive
branch
2 times, most recently
from
d988b94 to
90fd114
Compare
|
@swift-ci please test |
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]>
compnerd
force-pushed
the
insensitive
branch
from
90fd114 to
8ded310
Compare
|
@swift-ci please test |
|
@swift-ci please test Windows platform |
1 similar comment
|
@swift-ci please test Windows platform |
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
PathandPATHidentically (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.Fixes: #446