Follow-up / Making Scipio environment-configurable #154
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import struct Basics.AbsolutePath | ||
| @_spi(SwiftPMInternal) import struct Basics.Environment | ||
|
|
||
| public typealias ToolchainEnvironment = [String: String] | ||
|
There was a problem hiding this comment. @giginet I've extracted all the |
||
|
|
||
| extension ToolchainEnvironment { | ||
| var developerDirPath: String? { self["DEVELOPER_DIR"] } | ||
| var toolchainBinPath: Basics.AbsolutePath? { | ||
| if let developerDirPath { | ||
| return try? AbsolutePath(validating: developerDirPath) | ||
| .appending(components: "Toolchains", "XcodeDefault.xctoolchain", "usr", "bin") | ||
| } | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| extension ToolchainEnvironment? { | ||
|
There was a problem hiding this comment. It's confusing to extend to There was a problem hiding this comment. The idea is to shift all the boilerplate from the calling site to the extension. Sure, we can use try UserToolchain(
swiftSDK: try .hostSwiftSDK(
toolchainEnvironment?.toolchainBinPath,
environment: toolchainEnvironment.flatMap { Environment($0) } ?? Environment.current
),
environment: toolchainEnvironment.flatMap { Environment($0) } ?? Environment.current
)versus with the optional extension: try UserToolchain(
swiftSDK: try .hostSwiftSDK(
toolchainEnvironment?.toolchainBinPath,
environment: toolchainEnvironment.asSwiftPMEnvironment
),
environment: toolchainEnvironment.asSwiftPMEnvironment
)Additionally, in the first case, we would also need to use this clunky import: or did you mean something else? There was a problem hiding this comment. Thank you for the comment. I haven't considered I already tweaked my changes, but I'll revert this commit. 260baa5 Sorry for the modification! There was a problem hiding this comment. @giginet I see what you mean! Looks good to me 👍 Update: yeah, it doesn't solve the issue with the import. There was a problem hiding this comment. Oops, I already reverted the commit 😓 If you think it's good, I'll apply the commit again and merge this. Thank you! There was a problem hiding this comment. I was mistaken, the import was not needed in your case. Yeah, feel free to apply it again. As a benefit of your approach - it's more obvious that the variable is optional at the calling site. |
||
| var asSwiftPMEnvironment: Environment { map(Environment.init) ?? Environment.current } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import XCTest | ||
|
There was a problem hiding this comment. You can use There was a problem hiding this comment. I haven't noticed you already used it in the other tests. No problem, I'll do it next time 👍 |
||
| @testable import ScipioKit | ||
| @_spi(SwiftPMInternal) import struct Basics.Environment | ||
|
|
||
| final class ToolchainEnvironmentTests: XCTestCase { | ||
|
|
||
| func testBasics() { | ||
| let environment = [ | ||
| "DEVELOPER_DIR": "/Applications/Xcode.app/Contents/Developer", | ||
| ] | ||
|
|
||
| XCTAssertEqual(environment.developerDirPath, "/Applications/Xcode.app/Contents/Developer") | ||
| XCTAssertEqual( | ||
| environment.toolchainBinPath?.pathString, | ||
| "/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin" | ||
| ) | ||
| } | ||
|
|
||
| } | ||
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.
Sorry for the belated response. I'm back as of today!
We’re almost finished with our Scipio-powered tool, and during final testing, we discovered another environment-related issue (hopefully the last one!).
There’s a bug in SwiftPM where Environment isn’t passed down to the shell-executing function here. Fortunately, it’s possible to work around this by supplying a custom binDir (pointing to the bin dir of the current toolchain in our case) a few lines above.
I’m planning to file another fix for this in SwiftPM soon (I've already merged these two, and have one currently open). Although it’s not a critical issue, resolving it would allow a cleaner setup by removing the need to specify a custom bin path.
Apologies for including this change in the current PR, but I believe it’s closely related to its focus, and it’s purely additive, leaving existing behavior unaffected.
As mentioned, this appears to be the last environment-related enhancement needed to enable Scipio for multi-toolchain use. 🎉