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

Fix: OSS builds can accidentally using coreutils cp #44097

Closed
wants to merge 1 commit into from

Conversation

blakef
Copy link
Contributor

@blakef blakef commented Apr 15, 2024

Summary:
There are two places where we use a feature specific to the system version of 'cp', the:

-X Do not copy Extended Attributes (EAs) or resource forks.

This feature isn't available in GNU's cp, which is commonly installed on macOS using:

brew install coreutils && brew link coreutils

We can avoid the problem alltogether by being specific about the path of the system cp.

Changelog: [General][Fixed] don't break script phase and codegen when coreutils installed on macOS

Differential Revision: D56143216

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Apr 15, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D56143216

Summary:

There are two places where we use a feature specific to the system version of 'cp', the:

  -X    Do not copy Extended Attributes (EAs) or resource forks.

This feature isn't available in GNU's cp, which is commonly installed on macOS using:

  brew install coreutils && brew link coreutils

We can avoid the problem alltogether by being specific about the path of the system cp.

Changelog: [General][Fixed] don't break script phase and codegen when coreutils installed on macOS

Differential Revision: D56143216
@blakef blakef force-pushed the export-D56143216 branch from e059123 to f49960c Compare April 15, 2024 16:57
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D56143216

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 19,385,256 -13
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 22,760,259 -6
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 7047563
Branch: main

blakef added a commit to blakef/react-native that referenced this pull request Apr 17, 2024
Summary:

There are two places where we use a feature specific to the system version of 'cp', the:

  -X    Do not copy Extended Attributes (EAs) or resource forks.

This feature isn't available in GNU's cp, which is commonly installed on macOS using:

  brew install coreutils && brew link coreutils

We can avoid the problem alltogether by being specific about the path of the system cp.

Changelog: [General][Fixed] don't break script phase and codegen when coreutils installed on macOS

Reviewed By: cipolleschi

Differential Revision: D56143216
@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Apr 17, 2024
@facebook-github-bot
Copy link
Contributor

This pull request has been merged in e079953.

Copy link

This pull request was successfully merged by @blakef in e079953.

When will my fix make it into a release? | How to file a pick request?

Titozzz pushed a commit that referenced this pull request Jul 22, 2024
Summary:
Pull Request resolved: #44097

There are two places where we use a feature specific to the system version of 'cp', the:

  -X    Do not copy Extended Attributes (EAs) or resource forks.

This feature isn't available in GNU's cp, which is commonly installed on macOS using:

  brew install coreutils && brew link coreutils

We can avoid the problem alltogether by being specific about the path of the system cp.

Changelog: [General][Fixed] don't break script phase and codegen when coreutils installed on macOS

Reviewed By: cipolleschi

Differential Revision: D56143216

fbshipit-source-id: f1c1ef9ea2f01614d6d89c4e9eedf43113deb80c
@isaacl
Copy link

isaacl commented Jul 31, 2024

Although this PR is reasonable, the description of what it fixes is inaccurate.

This never affected users who simply ran brew install coreutils && brew link coreutils. This is because of the caveat on the formula:

==> Caveats
Commands also provided by macOS and the commands dir, dircolors, vdir have been installed with the prefix "g".
If you need to use these commands with their normal names, you can add a "gnubin" directory to your PATH with:
PATH="/opt/homebrew/opt/coreutils/libexec/gnubin:$PATH"

In short, this only affects users who have manipulated their PATH variable to put gnu tools higher than the OS X standard tooling. I'm sure there's all sorts of similarly weird and dangerous environment changes I could make which would break XCode or the react-native build...

So while this patch is safe, I'm not sure RTN should be accepting patches like this. They adds complexity to the project and every PR has a risk of introducing regressions or derailing a release. All for the meager gain of building better on an incorrectly configured computer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants