[SR-12912] Fix crash in test targets with Bundle.module in 5.3 #2905
Conversation
…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
requested review from
abertelrud,
aciidgh,
friedbunny,
neonichu and
rballard
as code owners
|
@swift-ci please test |
MaxDesiatov
changed the title
|
Looks good to me, though I don't know where we are with being to get things into the SwiftPM 5.3 branch right now. |
|
Should I ping Rick Ballard to clarify this? (He's the release manager to make the decision on whether this can be included, isn't he?) |
|
I saw the PR! I'm discussing schedules with some other Swift folks and will get back to you soon. |
|
While this is an important fix, at this point in the schedule we're going to hold it; but let's keep the PR open and take it at the first post-5.3 opportunity. |
|
Thanks everyone! Please correct me if I'm wrong, but this basically renders SPM resources unusable for the following use cases (in Swift 5.3.0 that is):
The former is less critical IMHO. I also found a workaround for that and posted it in the forums. I find it rather difficult, however, to find a workaround for the latter case, since I'd like to avoid having to differentiate how resources are loaded in release vs. debug builds, just so that unit tests don't crash. Also, this would mean that the production code of loading resources is never actually tested. |
|
If we missed the mark for Xcode 12 GM, I would still really like to see this merged and available for an upcoming Xcode 12 point release, because otherwise it means we can't release SPM support for a popular library that includes resources (Core Data model definitions) and unit test coverage that involves those models: robbiehanson/XMPPFramework#1177 What I'm not quite sure about is if this is merged into a Swift 5.3.1 release, our |
|
Please ship it with 5.3 |
|
At this point in the schedule we need to hold this fix, but I expect us to take it ASAP for the first dot release post-5.3. This is a good question @chrisballinger re: whether we need to define a new package version for compatibility here; I'll chat with @neonichu about that. |
|
Will this fix SR-13560 ? |
|
We've shipped 5.3 and should get this into the next patch release; approved to merge now. (I see @airspeedswift just beat me to it). I chatted with @neonichu and we agreed that no new format version was needed here, as clients of a package don't run that package's tests, so clients on stock 5.3 shouldn't break when packages they use adopt resources. |
Same as #2817, resolves SR-12912 in the
release/5.3branch.Setting the path for Bundle.module fails in the case where the bundle is accessed during tests.
This uses the already derived bundlePath instead.