-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix resource bundle lookup #3463
Conversation
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.
Thank you for fixing this! Just one question inline about a path separator.
Sources/Build/BuildPlan.swift
Outdated
stream <<< """ | ||
import class Foundation.Bundle | ||
|
||
extension Foundation.Bundle { | ||
static var module: Bundle = { | ||
let mainPath = "\(mainPath.asSwiftStringLiteralConstant)" | ||
let buildPath = "\(bundlePath.asSwiftStringLiteralConstant)" | ||
let mainPath = Bundle.main.bundlePath + "\(bundlePath.basename.asSwiftStringLiteralConstant)" |
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 the bundlePath
guaranteed to end with a path separator on all platforms, or might one need to be conditionally inserted here?
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.
@abertelrud Good point, thanks! The old code also appended a slash. I've added this back!
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.
@abertelrud Actually, I'm now using Bundle.main.bundleURL.appendingPathComponent
. This moves the responsibility for path separators to Foundation.URL
. Using a forward-slash might break Windows, since the path separator is a backslash there.
Hope this is ok?
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.
Thanks @ffried, I think that's even better, especially for Windows compatibility.
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.
Thanks, this looks good to me. @neonichu, WDYT?
This would be good to nominate for 5.5 once it's in main, I think.
+1 cherry-picking to 5.5 make sense |
@swift-ci please smoke test |
are there any tests we need to add here to make sure this does regress in the future? |
@tomerd I added a test assertion that checks for Bundle.main being in the generated code. That's not bulletproof but still better than nothing I guess. |
That's a good point about a regression test. One way to test the functionality here would be to have a test package that builds an executable that relies on the presence of the resource bundle, and then a functional test that builds it, copies the executable and its resource bundle somewhere else, and then removes the original built resource bundle (to make sure hardcoded paths break), and then runs the executable. It's a bit involved but that would be a useful test to add. I don't think we should prevent nominating for 5.5 for that test but we should really have one of those to detect any functional breakage in the future. |
lets record in a radar? |
Definitely, if that test isn't part of this PR, then we should absolutely have a JIRA on it, and get to it soon. I can create it, or @ffried would you want to create a JIRA to track adding such a test (or you might have other ideas for how best to test it)? |
@abertelrud What you're describing sounds exactly like what we're doing in our docker deployments (two images, one for building, one for running). So that would be an awesome test case. |
Sounds good — happy to create the JIRA. I’ll post back here when I’ve written it up. To be clear, the JIRA initially should spell out what needs to happen, but can leave some details of exactly how to implement it for later. We should be able to slot that into the functional tests without too much problem. |
@ffried Thanks for this PR, could you also do a cherry-pick for the |
* Fix resource bundle lookup * Add test check that `Bundle.main` is executed in the compiled binary * Ensure trailing slash for bundle path * Rely on Foundation.URL for path separators
Created https://bugs.swift.org/browse/SR-14592 for the functional test. I used a package similar to that for manual testing of #3467 though the actual work is in writing the functional test that causes all of that to happen. |
Thanks again for this fix! |
@abertelrud Couldn't let go of this... 🙃 I've taken a shot at adding this test in #3472. Maybe it's not perfect yet, but I've tested it on the release/5.5 branch where the test was green (tested on macOS only, though). |
Fix the preferred lookup location for resource bundles.
Motivation:
The preferred lookup location for resource bundles is next to the binary that is executed. This was previously achieved by searching next to
Bundle.main.bundlePath
. This change moved the execution ofBundle.main
outside of the generated bundle accessor code - and thus usesBundle.main
of the compilation process.Modifications:
Move the
Bundle.main
call back to the generated code.Result:
The preferred resource bundle lookup will again be the location of the compiled binary.
Fixes https://bugs.swift.org/browse/SR-14555.