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

Fix a bug where an incremental install missed library resources #9431

Merged
merged 3 commits into from Jan 14, 2020

Conversation

igor-makarov
Copy link
Contributor

@igor-makarov igor-makarov commented Dec 24, 2019

I ran into an issue where having a (local) pod that has resources behaves incorrectly when performing an incremental install:

If a resource file (png, xib, storyboard etc.) is renamed/moved, the containing target gets regenerated. However, the aggregate target does not.

This leads to a state where the generated resource install script contains incorrect resource paths. The script fails silently, causing the build to complete and the app to crash at runtime.

This PR fixes this issue. I've added specs for the case I ran into.

@sebastianv1
Copy link
Collaborator

Ah interesting. In this case it might make more sense to store a checksum of the contents of the aggregate target script phases (i.e. resources, embed frameworks). I imagine that's how it's materializing as a bug.

@igor-makarov
Copy link
Contributor Author

I've checked and it won't be possible. The live cache keys (the ones compared to the saved ones) are generated from an AggregateTarget, which doesn't have the scripts. They are created much later.

In fact, I think that is actually a good thing because generating scripts takes time and avoiding it is good.

@dnkoutso dnkoutso added this to the 1.9.0 milestone Jan 2, 2020
@dnkoutso
Copy link
Contributor

dnkoutso commented Jan 2, 2020

How do we want to proceed here?

@igor-makarov
Copy link
Contributor Author

igor-makarov commented Jan 2, 2020

I've been using this patch in our project and can confirm it has no side effects that I know of.

return :project if other.key_hash['PROJECT_NAME'] != key_hash['PROJECT_NAME']
end

this_files = key_hash['FILES']
other_files = other.key_hash['FILES']
return :project if this_files != other_files
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did this get pulled out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we need to compare file lists for all targets now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool

@sebastianv1
Copy link
Collaborator

The change seems good. This seems to target one particular scenario, and doesn't address what looks to be a bigger issue of determining if an AggregateTarget is dirty (not a slam on this PR, it's likely been a bug since incremental launched. Thank you for finding this bug and fixing it @igor-makarov). I'll spend some time looking more into what other ways AggregateTargets can become dirty (maybe xcframeworks?).

@igor-makarov
Copy link
Contributor Author

Yeah, frameworks are being missed by the cache.

@dnkoutso
Copy link
Contributor

just needs a rebase and we can land!

@igor-makarov
Copy link
Contributor Author

Rebased 💪🏻

@dnkoutso dnkoutso merged commit e62532c into CocoaPods:1-9-stable Jan 14, 2020
@igor-makarov igor-makarov deleted the fix-incremental-resource branch January 14, 2020 07:52
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