[SR-12912] Fix crash in test targets when accessing Bundle.module #2817

Steven0351 · 2020-07-17T08:24:03Z

Currently, running swift test fails when a test target accesses Bundle.module during a test (https://forums.swift.org/t/swift-5-3-spm-resources-in-tests-uses-wrong-bundle-path/37051).
This patch uses the bundlePath that already exists instead of crafting the path from Bundle.main. I've tested this in both library, executable, and test targets on macOS and it seems to do the trick.

Setting the path for Bundle.module fails in the case where the bundle is accessed during tests. This uses the already derived bundlePath instead.

abertelrud · 2020-07-17T18:27:11Z

Sources/Build/BuildPlan.swift

        let stream = BufferedOutputByteStream()
        stream <<< """
        import class Foundation.Bundle

        extension Foundation.Bundle {
            static var module: Bundle = {
-                let bundlePath = Bundle.main.bundlePath + "/" + "\(bundleBasename)"
+                let bundlePath = "\(bundlePath)"


Wouldn't this hardcode the absolute path into each artifact (also non-tests)?

I'm not familiar with how bundlePath is derived so that is possible. I wasn't able to confirm this on an iOS device itself as Xcode won't let me install it on my test device with a custom toolchain. ~~Is it possible to check if a given BuildPlan is compiling a test target?~~ I saw that there is an isTestTarget member variable on BuildPlan and changed the logic to only use bundlePath when that is true.

Be aware that the Xcode integration of packages does not use SwiftPM from the toolchain, you will only see an effect of your changes in commandline SwiftPM.

Right, but in this case, even when just building with swift build, wouldn't this also change the path that's built into a package target that's linked into an executable, meaning that if the executable and its associated resource bundles are later moved, the bundles can't be found?

Right, but in this case, even when just building with swift build, wouldn't this also change the path that's built into a package target that's linked into an executable, meaning that if the executable and its associated resource bundles are later moved, the bundles can't be found?

That seems likely, when I tested I did move the executable but not the resource bundle. ~~Would it make sense to conditionally check if the current Bundle.main path is located in an Xcode path and fallback to the hard coded path?~~ (see latest commit)

@neonichu I'm guessing that Xcode has its own method for computing Bundle.module? I had gone down a rabbit hole of trying to run my tests with xcodebuild on my package, but had other issues altogether there.

Steven0351 · 2020-07-17T20:11:00Z

Sources/Build/BuildPlan.swift

@@ -577,14 +577,16 @@ public final class SwiftTargetBuildDescription {
    private func generateResourceAccessor() throws {
        // Do nothing if we're not generating a bundle.
        guard let bundlePath = self.bundlePath else { return }
-
+
+        let pathToModule = isTestTarget ? bundlePath : "Bundle.main.bundlePath/\(bundlePath.basename)"


I just realized this is not actually going to work, I've made some changes locally and am building a new toolchain before pushing the changes, but tentatively this has been re-written to

let pathToModule = isTestTarget ? bundlePath : "Bundle.main.bundlePath + \"/\" + \(bundlePath.basename)"

…xpected, however, when the target under test accesses Bundle.module, it fails for the same reason as before

Steven0351 · 2020-07-17T23:13:34Z

I've converted this to a Draft PR because this is going to take a bit of consideration. To address the concern raised by @abertelrud, I updated the generation of Bundle.module to this:

let pathToModule = isTestTarget ? #""\#(bundlePath.pathString)""# : #"Bundle.main.bundlePath + "/" + "\#(bundlePath.basename)""#
      
 let stream = BufferedOutputByteStream()
 stream <<< """
 import class Foundation.Bundle

 extension Foundation.Bundle {
     static var module: Bundle = {
         let bundlePath = \(pathToModule)
         guard let bundle = Bundle(path: bundlePath) else {
             fatalError("could not load resource bundle: \\(bundlePath)")
         }
         return bundle
    }()
}
"""

However, when a target that is being tested calls Bundle.module to access a resource during a test, the test fails with the same error:
Fatal error: could not load resource bundle: ${path to xcode developer tools}/usr/bin/Foo_Foo.bundle: file ${path to derived sources}/resource_bundle_accessor.swift, line 7

…the build output directory path.

Steven0351 · 2020-07-18T04:33:58Z

I think I've come up with a reasonable solution: we first check if the resource bundle is accessible by appending to the bundle name to Bundle.main, if that fails we fall back to the absolute path to the resource bundle in the build directory, and if that fails we bail out with a fatal error.

Jeehut · 2020-08-15T13:52:23Z

@Steven0351 Did you already find time to implement your "reasonable solution" so we can get this merged before final release of Xcode 12? Feel free to ping me if I can help here as the bug is really blocking a proper CI configuration for any open source repo using resources right now ...

Steven0351 · 2020-08-15T14:24:35Z

@Jeehut I had committed what I thought was a "reasonable" solution, but there has been no feedback on those commits after I marked them ready for review. Bundle.module is generated at build time, so my workaround has the original bundle, but falls back to the build path in the event that the preferred bundle is not available (which should only be the case when a target is under test). I've been using my fork to run tests for macOS Swift packages without issue, but I'm only really able to run those locally because I haven't published that for use in any of my CI anywhere.

MaxDesiatov · 2020-08-15T15:31:02Z

I can trigger CI and/or request reviews from relevant people, is this ready for CI or review at all?

Steven0351 · 2020-08-15T15:38:29Z

@MaxDesiatov It's ready for review. I've run the tests locally when I originally patched and validated that my macOS Swift PM projects with bundle resources were able to both be tested and run (along with being moved around the file system without breaking non-test behavior).

MaxDesiatov · 2020-08-15T15:59:54Z

Would you be able to add corresponding automated tests to the SwiftPM test suite, so that it could be tested on CI?

Steven0351 · 2020-08-15T16:49:23Z

@MaxDesiatov I could give it a go but I'll have to dig into the test suite to see how this part of the codebase is tested. I could probably write something that validates the values that are generated, but I didn't notice anything in the current test suite that would enable validating the properties at runtime. Do you have a suggested approach there?

MaxDesiatov · 2020-08-15T21:09:13Z

Did you check the integration test suite with test cases such as this one?

Added passing integration test into TestToolTests.swift Added failing integration test into BasicTests.swift

Steven0351 · 2020-08-20T07:10:01Z

@MaxDesiatov I had never noticed the IntegrationsTest project since it's not visible when opening the root package in Xcode (thanks for the tip!).

I did run into an issue there: it uses the existing swift toolchain by default. This will fail if CI runs without setting the SWIFTPM_BIN_DIR environment variable, which I have no way of knowing because those configs aren't publicly available. This can be made to pass by invoking SWIFTPM_BIN_DIR=/path/to/swift-package-manager/.build/debug swift test in the IntegrationsTest directory after performing a build.

@neonichu Would you be able to review/throw in your opinion here as well?

abertelrud · 2020-08-20T22:14:37Z

@swift-ci please smoke test

abertelrud

Thanks for revising the PR (and apologies about the delay in taking another look at it). Looks good, though I'd suggest moving the lookup of the fallback bundle to a place where it will only be run if the first one isn't found. There can be a performance cost to instantiating a bundle.

abertelrud · 2020-08-20T22:18:26Z

Sources/Build/BuildPlan.swift

+                let buildPath = "\(bundlePath.pathString)"
+
+                let preferredBundle = Bundle(path: mainPath)
+                let fallBackBundle = Bundle(path: buildPath)


Since this could be a little expensive, it might be worth instantiating this second Bundle only if the first one can't be found, especially since at deployment time for the non-test products, the first one should always be found.

That's fair; I've inlined the instantiation of this in the ternery expression per this feedback

abertelrud · 2020-08-21T01:44:29Z

Thanks for making the change!

abertelrud · 2020-08-21T01:44:40Z

@swift-ci please test

Steven0351 · 2020-08-21T17:08:19Z

Looks like that Linux build failure was related to an issue compiling core-libs-foundation?

MaxDesiatov · 2020-08-21T18:53:23Z

@swift-ci please smoke test

abertelrud · 2020-08-21T22:51:44Z

The Linux failure on the full test does indeed look unrelated. The smoke tests did pass. I would like to get one more review on this but as far as I am concerned this looks good.

neonichu · 2020-08-24T18:24:17Z

Looks like the only failure is from an accidental full test run, so this can be merged.

…iftlang#2817) * SR-12912 Setting the path for Bundle.module fails in the case where the bundle is accessed during tests. This uses the already derived bundlePath instead. * Only use bundlePath when building a testTarget * Updating this for discussion in PR. This hardcodes the test path as expected, however, when the target under test accesses Bundle.module, it fails for the same reason as before * This prefers Bundle.main, but if it is inaccessible it falls back to the build output directory path. * Cleaned up changes in BuildPlan.swift Added passing integration test into TestToolTests.swift Added failing integration test into BasicTests.swift * Remove integration test from TestToolTests.swift * Only instantiate Bundle for buildPath if main bundle is not found

ffried · 2020-09-03T08:40:45Z

Any chance for this to make it into Swift 5.3?

…iftlang#2817) * SR-12912 Setting the path for Bundle.module fails in the case where the bundle is accessed during tests. This uses the already derived bundlePath instead. * Only use bundlePath when building a testTarget * Updating this for discussion in PR. This hardcodes the test path as expected, however, when the target under test accesses Bundle.module, it fails for the same reason as before * This prefers Bundle.main, but if it is inaccessible it falls back to the build output directory path. * Cleaned up changes in BuildPlan.swift Added passing integration test into TestToolTests.swift Added failing integration test into BasicTests.swift * Remove integration test from TestToolTests.swift * Only instantiate Bundle for buildPath if main bundle is not found

MaxDesiatov · 2020-09-03T09:07:22Z

I've created #2905 for that.

ffried · 2020-09-03T09:08:33Z

@MaxDesiatov Thank you!

) (#2905) * SR-12912 Setting the path for Bundle.module fails in the case where the bundle is accessed during tests. This uses the already derived bundlePath instead. * Only use bundlePath when building a testTarget * Updating this for discussion in PR. This hardcodes the test path as expected, however, when the target under test accesses Bundle.module, it fails for the same reason as before * This prefers Bundle.main, but if it is inaccessible it falls back to the build output directory path. * Cleaned up changes in BuildPlan.swift Added passing integration test into TestToolTests.swift Added failing integration test into BasicTests.swift * Remove integration test from TestToolTests.swift * Only instantiate Bundle for buildPath if main bundle is not found Co-authored-by: Steven Sherry <[email protected]>

odrobnik · 2020-10-09T18:56:29Z

Forgive my ignorance, how can one tell in which Swift version this fix will be included? Swift 5.3, as of Xcode 12 still has the original implementation as evidenced with the shorter error message.

MaxDesiatov · 2020-10-09T19:00:59Z

I saw some folks mentioned it's in Xcode 12.2 Beta 2, but I haven't verified it myself

Steven0351 · 2020-10-09T19:16:36Z

I can verify it is available in Xcode 12.2 Beta 2.

djbe · 2020-10-13T23:32:22Z

Unfortunately it's not in Xcode 12.1 though 😢 So we'll have to wait until 12.2 is released.

SR-12912

99a4dad

Setting the path for Bundle.module fails in the case where the bundle is accessed during tests. This uses the already derived bundlePath instead.

Steven0351 requested review from abertelrud, aciidgh, friedbunny and neonichu as code owners July 17, 2020 08:24

Steven0351 changed the title ~~SR-12912 - Fixes crash in test targets that contain resouces when accessing Bundle.module~~ SR-12912 - Fixes crash in test targets when accessing Bundle.module Jul 17, 2020

abertelrud reviewed Jul 17, 2020

View reviewed changes

Only use bundlePath when building a testTarget

5aafa7f

Steven0351 requested a review from abertelrud July 17, 2020 19:41

Steven0351 commented Jul 17, 2020

View reviewed changes

Updating this for discussion in PR. This hardcodes the test path as e…

d0168fb

…xpected, however, when the target under test accesses Bundle.module, it fails for the same reason as before

Steven0351 marked this pull request as draft July 17, 2020 23:07

This prefers Bundle.main, but if it is inaccessible it falls back to …

4c46da4

…the build output directory path.

Steven0351 marked this pull request as ready for review July 18, 2020 04:33

MaxDesiatov changed the title ~~SR-12912 - Fixes crash in test targets when accessing Bundle.module~~ SR-12912 Fix crash in test targets when accessing Bundle.module Aug 15, 2020

MaxDesiatov changed the title ~~SR-12912 Fix crash in test targets when accessing Bundle.module~~ [SR-12912] Fix crash in test targets when accessing Bundle.module Aug 17, 2020

tomerd assigned neonichu Aug 17, 2020

Cleaned up changes in BuildPlan.swift

a7366bc

Added passing integration test into TestToolTests.swift Added failing integration test into BasicTests.swift

Remove integration test from TestToolTests.swift

41ead0d

abertelrud reviewed Aug 20, 2020

View reviewed changes

Only instantiate Bundle for buildPath if main bundle is not found

074f23e

Steven0351 requested a review from abertelrud August 21, 2020 00:20

abertelrud approved these changes Aug 21, 2020

View reviewed changes

neonichu approved these changes Aug 21, 2020

View reviewed changes

neonichu merged commit cdcf0e0 into swiftlang:master Aug 24, 2020

MaxDesiatov mentioned this pull request Sep 3, 2020

[SR-12912] Fix crash in test targets with Bundle.module in 5.3 #2905

Merged

djbe mentioned this pull request Oct 13, 2020

Convert project to SPM SwiftGen/SwiftGen#793

Merged

nixberg mentioned this pull request Oct 29, 2020

Tests broken nixberg/shake-swift#1

Closed

richardpiazza mentioned this pull request Nov 4, 2020

Issue Additions richardpiazza/XcodeServer#14

Merged

This was referenced Nov 10, 2020

Support Swift Package Manager as development environment mapbox/MapboxStatic.swift#109

Merged

Mocked tests don’t run under Swift Package Manager mapbox/mapbox-directions-swift#494

Closed

swift-ci mentioned this pull request Jul 17, 2020

[SR-12912] SPM: Resources do not work in test targets #4541

Closed

[SR-12912] Fix crash in test targets when accessing Bundle.module #2817

[SR-12912] Fix crash in test targets when accessing Bundle.module #2817

Uh oh!

Conversation

Steven0351 commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abertelrud Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

Steven0351 Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neonichu Jul 17, 2020

Choose a reason for hiding this comment

Uh oh!

abertelrud Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Steven0351 Jul 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Steven0351 Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Steven0351 commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Steven0351 commented Jul 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jeehut commented Aug 15, 2020

Uh oh!

Steven0351 commented Aug 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxDesiatov commented Aug 15, 2020

Uh oh!

Steven0351 commented Aug 15, 2020

Uh oh!

MaxDesiatov commented Aug 15, 2020

Uh oh!

Steven0351 commented Aug 15, 2020

Uh oh!

MaxDesiatov commented Aug 15, 2020

Uh oh!

Steven0351 commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abertelrud commented Aug 20, 2020

Uh oh!

abertelrud left a comment

Choose a reason for hiding this comment

Uh oh!

abertelrud Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

Steven0351 Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

abertelrud commented Aug 21, 2020

Uh oh!

abertelrud commented Aug 21, 2020

Uh oh!

Steven0351 commented Aug 21, 2020

Uh oh!

MaxDesiatov commented Aug 21, 2020

Uh oh!

abertelrud commented Aug 21, 2020

Uh oh!

neonichu commented Aug 24, 2020

Uh oh!

ffried commented Sep 3, 2020

Uh oh!

MaxDesiatov commented Sep 3, 2020

Uh oh!

Steven0351 commented Jul 17, 2020 •

edited

Loading

Steven0351 Jul 17, 2020 •

edited

Loading

abertelrud Jul 17, 2020 •

edited

Loading

Steven0351 Jul 18, 2020 •

edited

Loading

Steven0351 Jul 17, 2020 •

edited

Loading

Steven0351 commented Jul 17, 2020 •

edited

Loading

Steven0351 commented Jul 18, 2020 •

edited

Loading

Steven0351 commented Aug 15, 2020 •

edited

Loading

Steven0351 commented Aug 20, 2020 •

edited

Loading