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

Use readlink instead of realpath in packager.sh #34145

Closed
wants to merge 1 commit into from

Conversation

dminkovsky
Copy link
Contributor

@dminkovsky dminkovsky commented Jul 6, 2022

Summary

realpath is not available on macOS 12.2 and 12.4. Because of this, the following error is shown when launching an RN app with XCode (which calls packager.sh via launchPackage.command):

Screen Shot 2022-07-06 at 1 26 08 PM

I am running Bash 5 but realpath is also not available with zsh.

This issue was introduced in bb8ddd6#diff-6ca7c99209bdf630550bb9e2946ce8611948c5a23b32ffb25028792ef5d48b8d, which interestingly did not change launchPackage.command. There's a recent comment on that commit that confirms this issue:

bb8ddd6#commitcomment-77818917

Changelog

[iOS] [Fixed] - Use readlink instead of realpath in packager.sh

Test Plan

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 6, 2022
dminkovsky referenced this pull request Jul 6, 2022
Summary:
Changelog: [Internal]

Generated with:

```
python3 codemod.py -d xplat/js --extensions=sh 'THIS_DIR=\$\(cd -P "\$\(dirname "\$\(readlink "\${BASH_SOURCE\[0\]}"' 'THIS_DIR=$(cd -P "$(dirname "$(realpath "${BASH_SOURCE[0]}"'```

Reviewed By: motiz88

Differential Revision: D34379955

fbshipit-source-id: c60521cd6508b203f48ca8c890c450319991c2d4
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jul 6, 2022
@analysis-bot
Copy link

analysis-bot commented Jul 6, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,823,071 +0
android hermes armeabi-v7a 7,216,282 +0
android hermes x86 8,136,194 +0
android hermes x86_64 8,114,049 +0
android jsc arm64-v8a 9,700,797 +0
android jsc armeabi-v7a 8,456,276 +0
android jsc x86 9,652,143 +0
android jsc x86_64 10,249,870 +0

Base commit: b66db7a
Branch: main

@analysis-bot
Copy link

analysis-bot commented Jul 6, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: b66db7a
Branch: main

@facebook-github-bot
Copy link
Contributor

@philIip has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@cortinico
Copy link
Contributor

Thanks for sending this over @dminkovsky

realpath is not available on macOS 12.2 and 12.4

Do you have a reference? I'm on 12.3 and I effectively have it. I'm wondering why this got removed in 12.3.

Also there are other usages of realpath in other scripts as well in the codebase, we'll have to replace it as well if this is the case.

@dminkovsky
Copy link
Contributor Author

dminkovsky commented Jul 7, 2022

Hey thanks for taking a look at this @cortinico.

My only reference for 12.4 is that I'm running it, and the other reporter on the breaking commit said they were running 12.2. So, I have no idea about 12.3, but realpath is, generally, to my knowledge, a common but not ubiquitous utility, not like readlink anyway. I don't think it's ever been available by default on macOS or BSD. For example: whatwg/html-build#90 (2016).

In general, though, I find this issue a bit confusing because the breaking commit (bb8ddd6) is from February 22nd, quite some time ago. Why did this only surface now? What motivated bb8ddd6? Maybe this https://stackoverflow.com/questions/3572030/bash-script-absolute-path-with-os-x?

@dminkovsky
Copy link
Contributor Author

Maybe just make brew install coreutils a required dependency for RN on macOS?

@cortinico
Copy link
Contributor

What motivated bb8ddd6?

Happy to provide a bit more context here. (Also cc @IanChilds as the original author).

It seems like our internal Buck setup needed to use realpath to resolve path of some of our paths. What happend was that a search-and-replace was executed on all the .sh files, replacing readlink with realpath.

I think we should be fine by either:

  1. Updating the RN website mentioning coreutils as one of the util to install.
  2. Merge this PR as it is. Specifically the file you're touching is used to kick-off Metro for RN-Tester manually and is never invoked by Buck. We might have other occurrencies of realpath in user facing scripts, so this issue migth surface again in the future.

So, I have no idea about 12.3

FYI, I'm on 12.3.1 and I do have realpath without having to install coreutils. However which realpath ends up being inside the brew folder, so I suspect I have it pulled in because of gnupg.

@cortinico cortinico requested a review from IanChilds July 12, 2022 10:25
@danilobuerger
Copy link
Contributor

I can confirm that realpath is not available per default on macOS 12.4

@cortinico
Copy link
Contributor

@dminkovsky Could you rebase?

@dminkovsky
Copy link
Contributor Author

Sure @cortinico, will be at a computer in a couple hours will let you know.

@dminkovsky
Copy link
Contributor Author

@cortinico I rebased

@dminkovsky dminkovsky closed this Jul 19, 2022
@dminkovsky dminkovsky deleted the patch-1 branch July 19, 2022 21:40
@dminkovsky dminkovsky restored the patch-1 branch July 19, 2022 21:41
@dminkovsky dminkovsky reopened this Jul 19, 2022
@dminkovsky
Copy link
Contributor Author

(Tried renaming my branch, which didn't go as intended)

@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Jul 20, 2022
@facebook-github-bot
Copy link
Contributor

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @dminkovsky in 698b147.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jul 20, 2022
@cortinico
Copy link
Contributor

Following up here: we decided to merge this as this usage of realpath was user facing and not used in the internal infra so we were good to go. If other occurrencies of realpath are offending a user flow, we will update them as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants