Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Expect Info.plist in correct location #77

Merged
merged 4 commits into from
Sep 1, 2019
Merged

Expect Info.plist in correct location #77

merged 4 commits into from
Sep 1, 2019

Conversation

tcurdt
Copy link
Contributor

@tcurdt tcurdt commented Aug 24, 2019

According to https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/FrameworkAnatomy.html the correct location for the Info.plist is
FRAMEWORK.framework/Resources/Info.plist. Yet the verification code was expecting it at FRAMEWORK.framework/Info.plist and failed the integration into the Xcode project.

The PR adjusts the location and swift test passes. It also works in my test project. This should fix #73 and #76

@tcurdt
Expect Info.plist in correct location
84b9cb7 
According to https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPFrameworks/Concepts/FrameworkAnatomy.html the correct location for the `Info.plist` is
`FRAMEWORK.framework/Resources/Info.plist`. Yet the verification code was expecting it at `FRAMEWORK.framework/Info.plist` and failed the integration into the Xcode project.

The PR adjusts the location and `swift test` passes. It also works in my test project. This should fix #73 and #76
Jeehut
Jeehut reviewed Aug 24, 2019
View reviewed changes
Copy link
Contributor

@Jeehut Jeehut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, looks good overall. Just one thing: Are you sure that the path is correct for each target OS? Could it be, that it's Resources/Info.plist for macOS targets and Info.plist for others?

If so, we should update the code to reflect that change. We should definitely verifiy if this code also works with iOS target for example.

Sources/AccioKit/Services/XcodeProjectIntegrationService.swift Outdated
@@ -244,7 +244,7 @@ final class XcodeProjectIntegrationService {
}

private func verifyBundleVersion(of product: FrameworkProduct) throws {
let plistURL = product.frameworkDirUrl.appendingPathComponent("Info.plist")
let plistURL = product.frameworkDirUrl.appendingPathComponent("Resources").appendingPathComponent("Info.plist")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified to appendingPathComponent("Resources/Info.plist").

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While that is true both Resources and Info.plist are individual path components as by the definition of the Apple API as in https://developer.apple.com/documentation/foundation/nsstring/1414489-pathcomponents

While in this case irrelevant it manually bakes in the path separator.
I would consider it bad practice - but no problem to change this.

@tcurdt
Copy link
Contributor Author

tcurdt commented Aug 24, 2019

I was wondering the very same thing about the location of the Info.plist.
The documentation is a big vague here https://developer.apple.com/library/archive/documentation/CoreFoundation/Conceptual/CFBundles/BundleTypes/BundleTypes.html#//apple_ref/doc/uid/10000123i-CH101-SW26

For one it says:

Bundle structures can vary depending on the type of the bundle and the target platform. The following sections describe the bundle structures used most commonly in both macOS and iOS.

yet the following suggests that the Resources/Info.plist is the place to check

The system identifies a framework bundle by the .framework extension on its directory name. The system also uses the Info.plist file inside the framework’s Resources directory to gather information about the configuration of the framework

(otherwise framework version also would not work).

That said I checked what I have on disk and found these examples for iOS

/System/iOSSupport/System/Library/Frameworks/WebKit.framework/Versions/A/Resources/Info.plist

/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/Library/CoreSimulator/Profiles/Runtimes/iOS.simruntime/Contents/Resources/RuntimeRoot/System/Library/Frameworks/AVFoundation.framework/Info.plist

To me this suggests that altering the behaviour per target of even better checking in both places could be the best strategy.

@tcurdt
Copy link
Contributor Author

tcurdt commented Aug 26, 2019

How do we go forward? Should amend the PR to support both locations, @Dschee?

@Jeehut
Copy link
Contributor

Jeehut commented Aug 27, 2019

Can't we just make Accio more resilient against the placement of the Info.plist file in general? E.g. by checking multiple places (maybe even regardless of the target platform? or by having a different check order per platform?) and use whatever we find. As an additional step, couldn't we simply create a basic Info.plist file if none is existing where we expect one?

As far as I remember, this was exactly the planned approach of @mrylmz. He probably just didn't have the time to finish it or maybe he just didn't think of macOS targets? I don't remember in detail, but feel free to take the approach suggested above. Then I think, we can safely merge this.

@mrylmz
Copy link
Contributor

mrylmz commented Aug 27, 2019

@Dschee there is a difference between a project Info.plist for building and a Info.plist of a build Framework, they contain different informations and as i know the final Info.plist of the build Framework is guaranteed to be existend.

It would be ok if we just check multiple known Info.plist locations as solution.

@Jeehut
Copy link
Contributor

Jeehut commented Aug 27, 2019

Okay, then let's opt for that.

@tcurdt
Copy link
Contributor Author

tcurdt commented Aug 27, 2019

@mrylmz If there is a difference the question is whether we need to handle the content differently depending on where they are. I didn't check that. Do you have more information on that?

@tcurdt
Support framework Info.plist locations for both macOS and iOS
adbe586 
Bundle verification (or rather fixing) now supports multiple locations inside the bundle. This now supports macOS and iOS style framework bundles.

Updated the integration test to be platform aware.
@tcurdt
Copy link
Contributor Author

tcurdt commented Aug 28, 2019

I've updated the PR to reflect the agreed upon path forward. HTH

Jeehut
Jeehut reviewed Aug 28, 2019
View reviewed changes
Copy link
Contributor

@Jeehut Jeehut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Could you add an entry to the CHANGELOG.md?

Sources/AccioKit/Services/XcodeProjectIntegrationService.swift Outdated
@@ -244,17 +244,25 @@ final class XcodeProjectIntegrationService {
}

private func verifyBundleVersion(of product: FrameworkProduct) throws {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this called "verify" when it also actually updates the Info.plist file? Maybe it should be ensureBundleVersionExistence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ask already hinted in the commit message I agree and will change it.

@tcurdt
Copy link
Contributor Author

tcurdt commented Aug 29, 2019

I've updated the PR as suggested.

@tcurdt
Copy link
Contributor Author

tcurdt commented Aug 31, 2019

@Dschee anything else that needs fixing so this can be merged?

Jeehut
Jeehut approved these changes Sep 1, 2019
View reviewed changes
Copy link
Contributor

@Jeehut Jeehut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, I think we can merge. Thanks! 💯

@Jeehut Jeehut merged commit e05aac7 into JamitLabs:stable Sep 1, 2019
@fredpi fredpi mentioned this pull request Sep 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jeehut Jeehut Jeehut approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error: The file “Info.plist” couldn’t be opened
3 participants
@tcurdt @Jeehut @mrylmz