-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Fix a bug where an incremental install missed library resources #9431
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
Conversation
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. |
I've checked and it won't be possible. The live cache keys (the ones compared to the saved ones) are generated from an In fact, I think that is actually a good thing because generating scripts takes time and avoiding it is good. |
fad478e
to
617960d
Compare
How do we want to proceed here? |
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 |
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.
Why did this get pulled out?
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.
Because we need to compare file lists for all targets now.
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.
Cool
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 |
Yeah, frameworks are being missed by the cache. |
just needs a rebase and we can land! |
617960d
to
2fd2645
Compare
Rebased 💪🏻 |
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.