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

Make APPLICATION_EXTENSION_API_ONLY not break when performing a cached incremental install #9142

Merged

Conversation

igor-makarov
Copy link
Contributor

@igor-makarov igor-makarov commented Sep 4, 2019

Closes #9141
Closes #8967

@igor-makarov igor-makarov changed the title Make APPLICATION_EXTENSION_API_ONLY not break when performing a cached install Make APPLICATION_EXTENSION_API_ONLY not break when performing a cached incremental install Sep 4, 2019
lib/cocoapods/target.rb Outdated Show resolved Hide resolved
lib/cocoapods/target.rb Outdated Show resolved Hide resolved
@dnkoutso
Copy link
Contributor

dnkoutso commented Sep 4, 2019

Needs a changelog entry

@dnkoutso dnkoutso added this to the 1.8.0 milestone Sep 4, 2019
@dnkoutso
Copy link
Contributor

dnkoutso commented Sep 4, 2019

cc @sebastianv1

@igor-makarov
Copy link
Contributor Author

Made the property readonly, added changelog, fixed the code comments.


next unless is_app_extension

aggregate_target.mark_application_extension_api_only
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the logic for determining is_app_extension live inside AggregateTarget. It looks like you're only using properties off aggregate_target. This allows you to remove mark_application_extension_api_only which is kinda ugly and get rid of the default false value.

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 the logic for is_app_extension can be put in AggregateTarget, mark_application_extension_api_only cannot be done away with - PodTarget also needs to be marked and it has no idea who its parent is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah bummer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a chat with @segiddins about it and it makes sense for the Analyzer to be the single one responsible for this weird flag.

@sebastianv1
Copy link
Collaborator

sebastianv1 commented Sep 4, 2019

Please add a spec for project_cache_analyzer thats verifies it'll mark the target as dirty for incremental installs. I'm surprised this doesn't just work since we mark a target as dirty if any of the build settings change.

@igor-makarov
Copy link
Contributor Author

The cache analyzer worked correctly, the problem was that the app extension flag was being set after generation in some obscure corner which was bypassed in an incremental cached install.

extension_target.product_type = 'com.apple.product-type.watchkit2-extension'
extension_target.name = 'watchOS Extension Target'
extension_target.symbol_type.should == :watch2_extension
@user_project.save
Copy link
Contributor

Choose a reason for hiding this comment

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

does this actually leave the file system dirty after this test runs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't - this is a temp copy (create_sample_app_copy_from_fixture).

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.

None yet

3 participants