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.

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

Allow copying of resource files from targets #95

Merged
merged 1 commit into from
Oct 18, 2017
Merged

Allow copying of resource files from targets #95

merged 1 commit into from
Oct 18, 2017

Conversation

keith
Copy link
Collaborator

@keith keith commented Oct 17, 2017

Previously we were assuming that all targets that were not app
extensions should be added to the copy frameworks build phase, even
though we didn't have any guarantee they were actually frameworks. This
updates that to ensure that things copied in the copy frameworks phase
are actually frameworks, and then falls back to the resources phase
instead. This fixes the ability to embed bundle targets, and copy them
as resources.

Fixes #91

@yonaskolb
Copy link
Owner

Nice catch @keith!

Previously we were assuming that all targets that were not app
extensions should be added to the copy frameworks build phase, even
though we didn't have any guarantee they were actually frameworks. This
updates that to ensure that things copied in the copy frameworks phase
are actually frameworks, and then falls back to the resources phase
instead. This fixes the ability to embed bundle targets, and copy them
as resources.
@keith
Copy link
Collaborator Author

keith commented Oct 18, 2017

Thanks! Rebased

@@ -261,10 +262,12 @@ public class PBXProjGenerator {
if dependencyTarget.type.isExtension {
// embed app extension
extensions.append(embedFile.reference)
} else if dependencyTarget.type.isFramework {

Choose a reason for hiding this comment

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

We'd need a bit more check here since the extension is not the only one determining if the framework needs to be copied. Only the dynamic ones need to. You can use lipo for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, I thought about that case, we actually have that case and I'm currently setting embed: False on it since we don't currently have a reference to the actual binary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can also use file which might be easier than lipo for this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought this was at least an improvement since that's now the only case we're not handling correctly after this and my other PRs

Choose a reason for hiding this comment

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

I agree this could be the second iteration to make it more concrete 👍. I didn't know it was possible to get that info using file.

@yonaskolb yonaskolb merged commit 689ac58 into yonaskolb:master Oct 18, 2017
@keith keith deleted the ks/copy-resources branch October 18, 2017 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to copy bundle product types into other targets
3 participants