-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
[iOS] Fix release build error due to a casing issue in hermes tarball path after download prebuilt tarball #42160
Conversation
Hi @wfern! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
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.
Thank you for opening this PR and fixing this issue.
I left a comment on lowercasing the version. I don't think it is required, because the versions usually have only numbers and dots. The only exception is for RCs, so it might make sense to keep the lowercase.
@@ -53,7 +53,7 @@ function shouldReplaceHermesConfiguration(configuration) { | |||
} | |||
|
|||
function replaceHermesConfiguration(configuration, version, podsRoot) { | |||
const tarballURLPath = `${podsRoot}/hermes-engine-artifacts/hermes-ios-${version}-${configuration}.tar.gz`; | |||
const tarballURLPath = `${podsRoot}/hermes-engine-artifacts/hermes-ios-${version.toLowerCase()}-${configuration.toLowerCase()}.tar.gz`; |
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.
I don't think we have to lower case the version, or am I missing something?
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.
In stable versions it is really not necessary but I added it for 2 reasons:
- Because of the .RC versions as you said and I saw in the issue: iOS Release build from XCode fails at "[CP-User] [Hermes] Replace Hermes for the right configuration, if needed" step after adding a package #39903 some people having this same error and it could be because of this issue.
- The method that generates the files in
hermes_utils.js
reduces the entire value including the version. So I think it really affects RC versions.
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.
Thank you for opening this PR and fixing this issue.
I left a comment on lowercasing the version. I don't think it is required, because the versions usually have only numbers and dots. The only exception is for RCs, so it might make sense to keep the lowercase.
@cipolleschi merged this pull request in 2e2f8a6. |
…after download prebuilt tarball (facebook#42160) Summary: In my mac, I use a case-sensitive volume and when I build a react-native 0.73 project it failed with an error that can't find the hermes release tarball to extract: ``` Node found at: /usr/local/bin/node Preparing the final location Extracting the tarball tar: Error opening archive: Failed to open '/Volumes/Workspace/meet-art-link/ios/Pods/hermes-engine-artifacts/hermes-ios-0.73.1-Release.tar.gz' ``` Note the `...-Release.tar.gz` in the error. In the disk it's `...-release.tar.gz`. The build fails in after download the release tarball in release mode because the hermes tarball name in the `replace_hermes_version.js` build script is capitalized, while the file is lowercase on disk. The fix is to ensure the hermes tarball name's "build type" is lowercase just like the function that creates the tarballs in react-native release located in `hermes_utils.js` in `getHermesPrebuiltArtifactsTarballName()`. Perhaps it's better to retrieve the tarball name from the same method it's generated? E.g.: ```js const { getHermesPrebuiltArtifactsTarballName } = require('react-native/scripts/hermes/hermes-utils'); const tarballName = getHermesPrebuiltArtifactsTarballName(`${version}-${configuration}`); const tarballURLPath = `${podsRoot}/hermes-engine-artifacts/${tarballName}`; ``` If yes, let me know to update the PR. ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [iOS][Fixed] - Fix release build error due to a casing issue in hermes tarball path after download prebuilt tarball Pull Request resolved: facebook#42160 Test Plan: Use a case sensitive volume or system and build react-native 0.73 in release mode, it will fail. Apply the patch in this PR and it will work fine. Reviewed By: cortinico Differential Revision: D52603439 Pulled By: cipolleschi fbshipit-source-id: 41ed8d8202874f338e4aa3af88d9d28ec1b8b3d5
…after download prebuilt tarball (facebook#42160) Summary: In my mac, I use a case-sensitive volume and when I build a react-native 0.73 project it failed with an error that can't find the hermes release tarball to extract: ``` Node found at: /usr/local/bin/node Preparing the final location Extracting the tarball tar: Error opening archive: Failed to open '/Volumes/Workspace/meet-art-link/ios/Pods/hermes-engine-artifacts/hermes-ios-0.73.1-Release.tar.gz' ``` Note the `...-Release.tar.gz` in the error. In the disk it's `...-release.tar.gz`. The build fails in after download the release tarball in release mode because the hermes tarball name in the `replace_hermes_version.js` build script is capitalized, while the file is lowercase on disk. The fix is to ensure the hermes tarball name's "build type" is lowercase just like the function that creates the tarballs in react-native release located in `hermes_utils.js` in `getHermesPrebuiltArtifactsTarballName()`. Perhaps it's better to retrieve the tarball name from the same method it's generated? E.g.: ```js const { getHermesPrebuiltArtifactsTarballName } = require('react-native/scripts/hermes/hermes-utils'); const tarballName = getHermesPrebuiltArtifactsTarballName(`${version}-${configuration}`); const tarballURLPath = `${podsRoot}/hermes-engine-artifacts/${tarballName}`; ``` If yes, let me know to update the PR. ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [iOS][Fixed] - Fix release build error due to a casing issue in hermes tarball path after download prebuilt tarball Pull Request resolved: facebook#42160 Test Plan: Use a case sensitive volume or system and build react-native 0.73 in release mode, it will fail. Apply the patch in this PR and it will work fine. Reviewed By: cortinico Differential Revision: D52603439 Pulled By: cipolleschi fbshipit-source-id: 41ed8d8202874f338e4aa3af88d9d28ec1b8b3d5
…after download prebuilt tarball (#42160) Summary: In my mac, I use a case-sensitive volume and when I build a react-native 0.73 project it failed with an error that can't find the hermes release tarball to extract: ``` Node found at: /usr/local/bin/node Preparing the final location Extracting the tarball tar: Error opening archive: Failed to open '/Volumes/Workspace/meet-art-link/ios/Pods/hermes-engine-artifacts/hermes-ios-0.73.1-Release.tar.gz' ``` Note the `...-Release.tar.gz` in the error. In the disk it's `...-release.tar.gz`. The build fails in after download the release tarball in release mode because the hermes tarball name in the `replace_hermes_version.js` build script is capitalized, while the file is lowercase on disk. The fix is to ensure the hermes tarball name's "build type" is lowercase just like the function that creates the tarballs in react-native release located in `hermes_utils.js` in `getHermesPrebuiltArtifactsTarballName()`. Perhaps it's better to retrieve the tarball name from the same method it's generated? E.g.: ```js const { getHermesPrebuiltArtifactsTarballName } = require('react-native/scripts/hermes/hermes-utils'); const tarballName = getHermesPrebuiltArtifactsTarballName(`${version}-${configuration}`); const tarballURLPath = `${podsRoot}/hermes-engine-artifacts/${tarballName}`; ``` If yes, let me know to update the PR. ## Changelog: <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [iOS][Fixed] - Fix release build error due to a casing issue in hermes tarball path after download prebuilt tarball Pull Request resolved: #42160 Test Plan: Use a case sensitive volume or system and build react-native 0.73 in release mode, it will fail. Apply the patch in this PR and it will work fine. Reviewed By: cortinico Differential Revision: D52603439 Pulled By: cipolleschi fbshipit-source-id: 41ed8d8202874f338e4aa3af88d9d28ec1b8b3d5
Summary:
In my mac, I use a case-sensitive volume and when I build a react-native 0.73 project it failed with an error that can't find the hermes release tarball to extract:
Note the
...-Release.tar.gz
in the error. In the disk it's...-release.tar.gz
.The build fails in after download the release tarball in release mode because the hermes tarball name in the
replace_hermes_version.js
build script is capitalized, while the file is lowercase on disk.The fix is to ensure the hermes tarball name's "build type" is lowercase just like the function that creates the tarballs in react-native release located in
hermes_utils.js
ingetHermesPrebuiltArtifactsTarballName()
.Perhaps it's better to retrieve the tarball name from the same method it's generated? E.g.:
If yes, let me know to update the PR.
Changelog:
[iOS][Fixed] - Fix release build error due to a casing issue in hermes tarball path after download prebuilt tarball
Test Plan:
Use a case sensitive volume or system and build react-native 0.73 in release mode, it will fail. Apply the patch in this PR and it will work fine.