Don't download hermes nightly tarball if it exists #36368
Closed
Conversation
This file contains 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
facebook-github-bot
added
CLA Signed
Base commit: 5746533 |
cipolleschi
approved these changes
Mar 4, 2023
| unless File.exist?(destination_path) | ||
| # Download to a temporary file first so we don't cache incomplete downloads. | ||
| tmp_file = "#{destination_folder}/hermes-ios.download" | ||
| `mkdir -p "#{destination_folder}" && curl "#{tarball_url}" -Lo "#{tmp_file}" && mv "#{tmp_file}" "#{destination_path}"` |
There was a problem hiding this comment.
nit: I know it is the same thing, but this makes more evident that we are moving the file.
Suggested change
| `mkdir -p "#{destination_folder}" && curl "#{tarball_url}" -Lo "#{tmp_file}" && mv "#{tmp_file}" "#{destination_path}"` | |
| `mkdir -p "#{destination_folder}" && curl "#{tarball_url}" -Lo "#{tmp_file}"` | |
| `mv "#{tmp_file}" "#{destination_path}"` |
There was a problem hiding this comment.
What if the first command fail, do you know if it would still execute the rename command?
There was a problem hiding this comment.
You are right, it looks like pod install won't fail and it will continue with the pod installation, with a potentially broken project.
|
It should be fine as it is still in node_modules so when someone updates react-native to a version with this the file will be deleted. |
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@cipolleschi merged this pull request in d2dd79f. |
OlimpiaZurek
pushed a commit
to OlimpiaZurek/react-native
that referenced
this pull request
May 22, 2023
Summary: Currently the hermes tarball will be re-downloaded every time we run pod update even if the file already exists. This adds a check to avoid downloading it if it already exists. To avoid partial downloads causing issues (if the script is interrupted) we download first to a different file and rename it when done. ## Changelog [IOS] [FIXED] - Don't download hermes nightly tarball if it exists Pull Request resolved: facebook#36368 Test Plan: Tested in an app using nightly builds that the download is only done once when running multiple pod update. Reviewed By: sshic Differential Revision: D43815772 Pulled By: cipolleschi fbshipit-source-id: 9dedb79cab1f1cb37cd82b425d93515c6e1728c1
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Currently the hermes tarball will be re-downloaded every time we run pod update even if the file already exists. This adds a check to avoid downloading it if it already exists. To avoid partial downloads causing issues (if the script is interrupted) we download first to a different file and rename it when done.
Changelog
[IOS] [FIXED] - Don't download hermes nightly tarball if it exists
Test Plan
Tested in an app using nightly builds that the download is only done once when running multiple pod update.