Follow-up / Making Scipio environment-configurable #154

dmhts · 2024-10-30T11:13:47Z

This PR is a follow-up built on #140, where Runner became more environment-aware. This follow-up addresses two additional issues:

Issue 1:

It fixes an issue where the environment dictionary was not properly passed through to UserToolchain.

Issue 2:

It introduces a non-caching implementation of the SwiftSDK.sdkPlatformFrameworkPaths(environment:) function (which basically copies the original function without the path-caching part). The original function, although environment-aware, caches the resolved platform path unconditionally on the first call in a private static property, with no way to reset the cache later. For Scipio to be fully environment-configurable, especially when the environment may change multiple times within a single run (e.g., when Scipio is used with multiple toolchains), each environment (associated with a specific toolchain) requires a unique platform path, e.g.:

- /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform
- /Applications/Xcode-15.4.app/Contents/Developer/Platforms/MacOSX.platform
- /Applications/Xcode-16.1.app/Contents/Developer/Platforms/MacOSX.platform"

Therefore, to enable greater flexibility, a non-caching implementation of the original function has been introduced.

@giginet As always, looking forward to your comments and questions.

…dkPlatformFrameworkPaths`

dmhts · 2024-10-30T11:20:02Z

Sources/ScipioKit/Producer/PIF/ToolchainGenerator.swift

+
+    /// A non-caching environment-aware implementation of `SwiftSDK.sdkPlatformFrameworkPaths`
+    /// Returns `macosx` sdk platform framework path.
+    func nonCachingSDKPlatformFrameworkPaths() async throws -> (fwk: AbsolutePath, lib: AbsolutePath) {


As I mentioned in the PR description, this is just a copy of the existing function without the path-caching part.

Could you note this context in the comment?

Please avoid abbreviations frameworkPath, libPath

I prefer to add a verb to the name. This method executes external commands, so it's not better to name it like a getter.

How about renaming like resolveSDKPlatformFrameworkPaths?

Please avoid abbreviations frameworkPath, libPath

Sorry, I just copied Apple's implementation one-to-one. Of course, it makes sense. Done.

How about renaming like resolveSDKPlatformFrameworkPaths?

Yes, I also like it. Done.

Could you note this context in the comment?

/// A non-caching environment-aware implementation of SwiftSDK.sdkPlatformFrameworkPaths

I believe it's already in the comment above, or did you mean something else?

giginet

Its concept looks good. However, it's better to tweak the interface of adding the method even if you just moved the existing implementation.

giginet · 2024-11-06T10:07:50Z

Sources/ScipioKit/Producer/PIF/ToolchainGenerator.swift

+
+    /// A non-caching environment-aware implementation of `SwiftSDK.sdkPlatformFrameworkPaths`
+    /// Returns `macosx` sdk platform framework path.
+    func nonCachingSDKPlatformFrameworkPaths() async throws -> (fwk: AbsolutePath, lib: AbsolutePath) {


Could you note this context in the comment?

giginet · 2024-11-06T10:08:28Z

Sources/ScipioKit/Producer/PIF/ToolchainGenerator.swift

+
+    /// A non-caching environment-aware implementation of `SwiftSDK.sdkPlatformFrameworkPaths`
+    /// Returns `macosx` sdk platform framework path.
+    func nonCachingSDKPlatformFrameworkPaths() async throws -> (fwk: AbsolutePath, lib: AbsolutePath) {


Please avoid abbreviations frameworkPath, libPath

giginet · 2024-11-06T10:12:37Z

Sources/ScipioKit/Producer/PIF/ToolchainGenerator.swift

+
+    /// A non-caching environment-aware implementation of `SwiftSDK.sdkPlatformFrameworkPaths`
+    /// Returns `macosx` sdk platform framework path.
+    func nonCachingSDKPlatformFrameworkPaths() async throws -> (fwk: AbsolutePath, lib: AbsolutePath) {


I prefer to add a verb to the name. This method executes external commands, so it's not better to name it like a getter.

How about renaming like resolveSDKPlatformFrameworkPaths?

…ent-fixes

dmhts · 2024-11-13T13:48:43Z

Sources/ScipioKit/ToolchainEnvironment.swift

+import struct Basics.AbsolutePath
+@_spi(SwiftPMInternal) import struct Basics.Environment
+
+public typealias ToolchainEnvironment = [String: String]


@giginet I've extracted all the ToolchainEnvironment-related logic into a separate file and introduced a type alias for improved semantics. Does that work for you?

dmhts · 2024-11-13T14:06:54Z

Sources/ScipioKit/DescriptionPackage.swift

+        self.toolchain = try UserToolchain(
+            swiftSDK: try .hostSwiftSDK(
+                toolchainEnvironment?.toolchainBinPath,
+                environment: toolchainEnvironment.asSwiftPMEnvironment
+            ),
+            environment: toolchainEnvironment.asSwiftPMEnvironment
+        )


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. 🎉

dmhts · 2024-11-13T14:08:38Z

@giginet is it the CI issue?

ikesyo · 2024-11-18T02:17:10Z

The CI failure is related to actions/runner-images#10703

giginet

Almost great. I'll modify the small points on this PR and I'll merge this

giginet · 2024-11-18T04:36:10Z

Tests/ScipioKitTests/ToolchainEnvironmentTests.swift

@@ -0,0 +1,19 @@
+import XCTest


You can use @Testing for new tests 😄

I haven't noticed you already used it in the other tests. No problem, I'll do it next time 👍

Sources/ScipioKit/Producer/PIF/ToolchainGenerator.swift

giginet · 2024-11-18T08:00:10Z

Sources/ScipioKit/ToolchainEnvironment.swift

+    }
+}
+
+extension ToolchainEnvironment? {


It's confusing to extend to Optional.
.current is injected on the callee side, but I prefer to inject on the caller side explicitly.

The idea is to shift all the boilerplate from the calling site to the extension. Sure, we can use map or flatMap at the calling site, but then it would be like this:

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: @_spi(SwiftPMInternal) import struct Basics.Environment every time Environment is referenced.

or did you mean something else?

Thank you for the comment. I haven't considered _spi.

I already tweaked my changes, but I'll revert this commit. 260baa5

Sorry for the modification!

@giginet I see what you mean! Looks good to me 👍

Update: yeah, it doesn't solve the issue with the import.

Oops, I already reverted the commit 😓

If you think it's good, I'll apply the commit again and merge this. Thank you!

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.

giginet · 2024-11-18T08:09:00Z

Sorry, I tweaked your patch. I'll merge this 🚀

This reverts commit 260baa5.

This reverts commit 0664260.

Sources/ScipioKit/Producer/PIF/ToolchainGenerator.swift

Co-authored-by: IKEDA Sho <suicaicoca@gmail.com>

dmhts added 2 commits October 10, 2024 15:22

Pass toolchainEnvironment to ToolchainGenerator

d52529b

Introduce non-caching environment-aware implementation of `SwiftSDK.s…

bff1ca6

…dkPlatformFrameworkPaths`

dmhts changed the title ~~Follow-up / Making environment configurable~~ Follow-up / Making Scipio environment-configurable Oct 30, 2024

dmhts commented Oct 30, 2024

View reviewed changes

giginet requested review from giginet and ikesyo November 6, 2024 09:44

giginet reviewed Nov 6, 2024

View reviewed changes

dmhts added 3 commits November 13, 2024 14:34

Pass optional bin path to SwiftSDK

a0edddb

Address comments

9dceec3

Merge remote-tracking branch 'upstream/main' into bugfix/sdk-environm…

489b6bb

…ent-fixes

dmhts commented Nov 13, 2024

View reviewed changes

giginet approved these changes Nov 18, 2024

View reviewed changes

giginet reviewed Nov 18, 2024

View reviewed changes

Sources/ScipioKit/Producer/PIF/ToolchainGenerator.swift Outdated Show resolved Hide resolved

Note original code

0b24489

giginet reviewed Nov 18, 2024

View reviewed changes

Do not extend optional value

260baa5

Revert "Do not extend optional value"

0664260

This reverts commit 260baa5.

giginet approved these changes Nov 18, 2024

View reviewed changes

Reapply "Do not extend optional value"

12dfdf7

This reverts commit 0664260.

ikesyo requested changes Nov 18, 2024

View reviewed changes

Sources/ScipioKit/Producer/PIF/ToolchainGenerator.swift Outdated Show resolved Hide resolved

Update Sources/ScipioKit/Producer/PIF/ToolchainGenerator.swift

4614bdd

Co-authored-by: IKEDA Sho <suicaicoca@gmail.com>

giginet approved these changes Nov 18, 2024

View reviewed changes

ikesyo approved these changes Nov 18, 2024

View reviewed changes

giginet merged commit 8bab62a into giginet:main Nov 18, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow-up / Making Scipio environment-configurable #154

Follow-up / Making Scipio environment-configurable #154

dmhts commented Oct 30, 2024 •

edited

Loading

dmhts Oct 30, 2024 •

edited

Loading

giginet Nov 6, 2024

giginet Nov 6, 2024

giginet Nov 6, 2024

dmhts Nov 13, 2024

dmhts Nov 13, 2024

dmhts Nov 13, 2024

giginet left a comment

giginet Nov 6, 2024

giginet Nov 6, 2024

giginet Nov 6, 2024

dmhts Nov 13, 2024 •

edited

Loading

dmhts Nov 13, 2024 •

edited

Loading

dmhts commented Nov 13, 2024

ikesyo commented Nov 18, 2024

giginet left a comment

giginet Nov 18, 2024

dmhts Nov 18, 2024

giginet Nov 18, 2024 •

edited

Loading

dmhts Nov 18, 2024 •

edited

Loading

giginet Nov 18, 2024

dmhts Nov 18, 2024 •

edited

Loading

giginet Nov 18, 2024

dmhts Nov 18, 2024

giginet commented Nov 18, 2024

Follow-up / Making Scipio environment-configurable #154

Follow-up / Making Scipio environment-configurable #154

Conversation

dmhts commented Oct 30, 2024 • edited Loading

Issue 1:

Issue 2:

dmhts Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giginet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmhts Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

dmhts Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

dmhts commented Nov 13, 2024

ikesyo commented Nov 18, 2024

giginet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giginet Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

dmhts Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmhts Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giginet commented Nov 18, 2024

dmhts commented Oct 30, 2024 •

edited

Loading

dmhts Oct 30, 2024 •

edited

Loading

dmhts Nov 13, 2024 •

edited

Loading

dmhts Nov 13, 2024 •

edited

Loading

giginet Nov 18, 2024 •

edited

Loading

dmhts Nov 18, 2024 •

edited

Loading

dmhts Nov 18, 2024 •

edited

Loading