-
Notifications
You must be signed in to change notification settings - Fork 1.2k
SR-6968: ProcessInfo.processInfo.environment only gets once #1444
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
Conversation
- Reparse the environment variables each time ProcessInfo.processinfo.enviroment is read.
Foundation/ProcessInfo.swift
Outdated
| let equalSign = Int32(UInt8(ascii: "=")) | ||
| var env: [String : String] = [:] | ||
| var idx = 0 | ||
| let envp = environ |
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.
Should this use _CFEnviron() instead of environ?
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.
Yes I think you are correct, it should also work correctly on Windows then as well. I will update it in a bit.
|
@swift-ci please test |
Foundation/ProcessInfo.swift
Outdated
| guard keyLen > 0 else { | ||
| continue | ||
| } | ||
| if let key = NSString(bytes: entry, length: keyLen, encoding: String.Encoding.utf8.rawValue) { |
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.
Is it guaranteed that all platforms use a UTF8 encoding?
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.
@parkera Probably not, I had looked at __CFGetEnvironment() and saw the decoding of UTF-8 but not the fallback to CFStringGetSystemEncoding(). Does the following look more portable?
open var environment: [String : String] {
let equalSign = Character("=")
var env: [String : String] = [:]
var idx = 0
let envp = _CFEnviron()
let strEncoding = String.Encoding(rawValue: CFStringConvertEncodingToNSStringEncoding(CFStringGetSystemEncoding()))
while let entry = envp.advanced(by: idx).pointee {
if let entry = String(cString: entry, encoding: strEncoding), let i = entry.index(of: equalSign) {
let key = String(entry.prefix(upTo: i))
let value = String(entry.suffix(from: i).dropFirst())
env[key] = value
}
idx += 1
}
return env
}
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.
That looks fine too.
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.
-let strEncoding = String.Encoding(rawValue: CFStringConvertEncodingToNSStringEncoding(CFStringGetSystemEncoding()))
+let strEncoding = String.defaultCStringEncoding
parkera
left a comment
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.
Looks good to me.
|
@swift-ci please test |
2 similar comments
|
@swift-ci please test |
|
@swift-ci please test |
|
The test failure seems unrelated. |
|
@swift-ci please test |
|
@swift-ci please clean test |
|
This is failing in upstream tests: |
|
Once we land this, I'd like to propose we backport it to the |
|
No problem, I have a few bug fixes I want to back port once CI is working again |
|
@swift-ci please test |
|
@swift-ci please test and merge |
1 similar comment
|
@swift-ci please test and merge |
|
@shahmishal do you know why we're seeing these errors in the test build?
|
|
@swift-ci please clean test |
|
@swift-ci please test and merge |
ProcessInfo.processinfo.enviroment is read.