[RN][OSS]Fix edge case where prepare_hermes_workspace and build_hermes_macos where using different hermes commits#34329
Closed
cipolleschi wants to merge 2 commits into
Closed
[RN][OSS]Fix edge case where prepare_hermes_workspace and build_hermes_macos where using different hermes commits#34329cipolleschi wants to merge 2 commits into
cipolleschi wants to merge 2 commits into
Conversation
facebook-github-bot
added
CLA Signed
cipolleschi
force-pushed
the
fix/investigate_test_ios
branch
from
e3ddd5a to
584a1d1
Compare
Base commit: 463af23 |
cipolleschi
force-pushed
the
fix/investigate_test_ios
branch
6 times, most recently
from
f6c2a6c to
b0ef9d3
Compare
Base commit: 463af23 |
cipolleschi
force-pushed
the
fix/investigate_test_ios
branch
3 times, most recently
from
b130f21 to
83327ef
Compare
cipolleschi
force-pushed
the
fix/investigate_test_ios
branch
from
83327ef to
7d5e23f
Compare
cipolleschi
force-pushed
the
fix/investigate_test_ios
branch
from
6e4cd03 to
7c49294
Compare
cipolleschi
changed the title
Contributor
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Collaborator
|
This pull request was successfully merged by @cipolleschi in 2fa04be. When will my fix make it into a release? | Upcoming Releases |
Contributor
|
@cipolleschi do we need to pick this in the 0.70 stable branch? |
Contributor
Author
|
@kelset: it could help but it is not necessary. I see that 0.70 CI is green, so the keys used their are not suffering from the poisoning! 👍 |
roryabraham
pushed a commit
to Expensify/react-native
that referenced
this pull request
Aug 17, 2022
…here using different hermes commits (react#34329) Summary: This PR fixes an edge case where `prepare_hermes_workspace` job was using a commit to build hermes but `build_hermes_macos` was using a different one. This resulted in cache poisoning where subsequent jobs thoughts to be using a version of Hermes while the restored cache was loading a different one. <img width="1440" alt="Screenshot 2022-08-03 at 06 26 14" src="https://user-images.githubusercontent.com/11162307/182570809-5c6d9323-c3fb-4834-952f-7d07b99c4880.png"> This PR simplifies the flow, creating a single `.hermesversion` file in the `prepare_hermes_workspace` workspace and using that file as key for all the caches. ## Changelog [iOS] [Changed] - upload test result as artifact Pull Request resolved: react#34329 Test Plan: CircleCI is now green and all the caches are using the same file to create the checksum. We can verify that by looking at the `Save cache`/`Restore cache` commands related to Hermes. (In the workflow, their hash is always `B1NEL0P0OKhQYtk8DE150bXSoGrdWUweedHKmqNqnjo`) Also, we removed completely the code that could create a version misalignment. Reviewed By: cortinico Differential Revision: D38382895 Pulled By: cipolleschi fbshipit-source-id: 5f5501a7ef313eb56abda336716b24b486a34a1f
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes an edge case where
prepare_hermes_workspacejob was using a commit to build hermes butbuild_hermes_macoswas using a different one. This resulted in cache poisoning where subsequent jobs thoughts to be using a version of Hermes while the restored cache was loading a different one.This PR simplifies the flow, creating a single
.hermesversionfile in theprepare_hermes_workspaceworkspace and using that file as key for all the caches.This whole investigation started with some failing tests, the PR also adds the
xcresultarchive as artifact for future failures.Changelog
[iOS] [Changed] - Simplified the cache code for Hermes and added the test
xcresultarchive as artifact, if we need to explore the tests in the future.Test Plan
CircleCI is now green and all the caches are using the same file to create the checksum.
We can verify that by looking at the
Save cache/Restore cachecommands related to Hermes. (In the workflow, their hash is alwaysB1NEL0P0OKhQYtk8DE150bXSoGrdWUweedHKmqNqnjo)Also, we removed completely the code that could create a version misalignment.
Verified that the
xcresultartifact is uploaded correctly.