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

Introduce .xcode.env configuration file to source node #33546

Closed
wants to merge 1 commit into from

Conversation

cipolleschi
Copy link
Contributor

Summary:
This Diff does 2 things:

  1. Removes all the remnant of the find-node.sh script. This allows React Native to stay agnostic from any other node manager
  2. Introduces a way for the developers to specify which node executable they want to use, through a simple .env file.

Changelog

[iOS][Changed] - This PR removes the find-node.sh scripts and replaces it with an .xcode.env file that is sourced by the script phases that needs it. The .xcode.env file is versioned: to customize a local environment, an unversioned .xcode.local.env can be used.

Differential Revision: D35317070

@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 fb-exported labels Apr 1, 2022
@facebook-github-bot
Copy link
Contributor

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

@react-native-bot react-native-bot added the Platform: iOS iOS applications. label Apr 1, 2022
template/_gitignore Outdated Show resolved Hide resolved
# PATH to the node executable. You can use the `command -v node` to check what value make sense for your set up
# This `.xcode.env` file is versioned. To customize your local environment, you can create an `.xcode.local.env`
# file that is not versioned.
export NODE_BINARY=/usr/local/bin/node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we just use command -v node here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what worked for me:

. "$(brew --prefix asdf)/asdf.sh"
export NODE_BINARY=$(command -v node)

so I think we should definitely add some examples in the comment, and probably change it to export NODE_BINARY=$(command -v node) by default.
I could see this working:

Suggested change
export NODE_BINARY=/usr/local/bin/node
# if you use:
# nvm with brew, uncomment the following
# . "$(brew --prefix nvm)/nvm.sh" --no-use
# asdf with brew, uncomment the following
# . "$(brew --prefix asdf)/asdf.sh"
export NODE_BINARY=$(command -v node)

Or something similar, maybe a link to a subsection of https://reactnative.dev/docs/environment-setup where we put a bunch of examples that the community can update? like

Suggested change
export NODE_BINARY=/usr/local/bin/node
# if you use any node version managers like nvm, asdf etc,
# make sure to add the right command to make `node` discoverable for your setup.
# Look at https://reactnative.dev/docs/environment-setup#finding-node for more info.
export NODE_BINARY=$(command -v node)

and in that subsection we could also have a button like "Add your own node version manager if its not here", with a button to create a PR on that file. Maybe we can explain also how and what is needed, like "asdf and other managers needs to run a script to make sure the right node version is in your PATH. You probably have this already in your .zshrc. Xcode needs this too, as it does not read your .zshrc, so if your node version manager requires a line like that, please add it here and in your .xcode.env or .xcode.local.env".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we would like to avoid a long list of commented examples that users can uncomment.

What happens when something changes in one of these node managers and the commented code does not work anymore? We have to maintain it.
Encouraging users to add their favourite node manager also defeats the purpose of having a versioned .env file that can be different project by project. At that point, we could have kept the old find-node buried in the build steps.

I think that it's ok to add one example, to show the developers how to configure a different node manager, perhaps one with a complex setup, but we should not accept PRs where people add different managers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true. ok lets keep at least one example, maybe a tiny explanation of what should go there, and/or maybe a link to some doc?

i think a doc could easily have a bunch of examples that everyone can make a PR to add/update an example with what they have found, so others can just copy/paste?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pvinis I added some documentation within the .xcode.env file itself and I'm going to explain it also in the website. Do you think that it is enough documentation? Do you have something else in mind?

template/ios/HelloWorld.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
@analysis-bot
Copy link

analysis-bot commented Apr 1, 2022

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

Base commit: 9f1fa91
Branch: main

@analysis-bot
Copy link

analysis-bot commented Apr 1, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,766,485 +0
android hermes armeabi-v7a 7,175,406 +0
android hermes x86 8,074,156 +0
android hermes x86_64 8,053,329 +0
android jsc arm64-v8a 9,643,725 +0
android jsc armeabi-v7a 8,417,964 +0
android jsc x86 9,592,906 +0
android jsc x86_64 10,190,099 +0

Base commit: 9f1fa91
Branch: main

Copy link
Contributor

@pvinis pvinis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a commend with some suggestions.

Really great work!
I think this will simplify the RN Core/Community work to maintain that find-node script.

It might cause some small headaches as people try to figure out how to use the .env and .local.env files. I can see teams using the main way in .env and devs with different setups using the .local.env, and I can see teams using basically the old find-node way, of some ifs and then the command -v thing. Both cool, but we should tell/show people how and why this can work.

As follow-up work, I think:

  • we should edit the scripts/react_native_pods_utils/script_phases.sh to instead of error: Could not find node. Make sure it is in bash PATH or set the NODE_BINARY environment variable. say something like error: Could not find node. Edit .xcode.env to make sure it is found in bash PATH or set the NODE_BINARY environment variable..
  • we should definitely notify people, possibly marking this as a BREAKING CHANGE.
  • we should have some notes/help in the rn website with some explanation and guidance and examples, as well as explain the difference and use cases of .xcode.env and .xcode.local.env.

# PATH to the node executable. You can use the `command -v node` to check what value make sense for your set up
# This `.xcode.env` file is versioned. To customize your local environment, you can create an `.xcode.local.env`
# file that is not versioned.
export NODE_BINARY=/usr/local/bin/node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what worked for me:

. "$(brew --prefix asdf)/asdf.sh"
export NODE_BINARY=$(command -v node)

so I think we should definitely add some examples in the comment, and probably change it to export NODE_BINARY=$(command -v node) by default.
I could see this working:

Suggested change
export NODE_BINARY=/usr/local/bin/node
# if you use:
# nvm with brew, uncomment the following
# . "$(brew --prefix nvm)/nvm.sh" --no-use
# asdf with brew, uncomment the following
# . "$(brew --prefix asdf)/asdf.sh"
export NODE_BINARY=$(command -v node)

Or something similar, maybe a link to a subsection of https://reactnative.dev/docs/environment-setup where we put a bunch of examples that the community can update? like

Suggested change
export NODE_BINARY=/usr/local/bin/node
# if you use any node version managers like nvm, asdf etc,
# make sure to add the right command to make `node` discoverable for your setup.
# Look at https://reactnative.dev/docs/environment-setup#finding-node for more info.
export NODE_BINARY=$(command -v node)

and in that subsection we could also have a button like "Add your own node version manager if its not here", with a button to create a PR on that file. Maybe we can explain also how and what is needed, like "asdf and other managers needs to run a script to make sure the right node version is in your PATH. You probably have this already in your .zshrc. Xcode needs this too, as it does not read your .zshrc, so if your node version manager requires a line like that, please add it here and in your .xcode.env or .xcode.local.env".

@elicwhite
Copy link
Member

elicwhite commented Apr 3, 2022

Is there any way to help make sure people get this set up correctly and are able to get past this upgrade step successfully?

Maybe react native doctor could be helpful here? Could it check that these files are set up properly and that it can actually find a node binary?

@cipolleschi
Copy link
Contributor Author

Thank you everyone for the comments! I'll answer here to the points that do not have any line reference

@pvinis

  • we should edit the scripts/react_native_pods_utils/script_phases.sh to instead of error: Could not find node. Make sure it is in bash PATH or set the NODE_BINARY environment variable. say something like error: Could not find node. Edit .xcode.env to make sure it is found in bash PATH or set the NODE_BINARY environment variable..

Yes, you are right. I'll improve the error messages! 👍

  • we should definitely notify people, possibly marking this as a BREAKING CHANGE.

I think this will be done with version 0.69 or 0.70. The changelog will be updated accordingly!

  • we should have some notes/help in the rn website with some explanation and guidance and examples, as well as explain the difference and use cases of .xcode.env and .xcode.local.env.

Yes, the work on this will be considered concluded with that documentation as well. But the website resides on a different repo, so I have to create a separate PR! ;)

@TheSavior

Is there any way to help make sure people get this set up correctly and are able to get past this upgrade step successfully?

Maybe react native doctor could be helpful here? Could it check that these files are set up properly and that it can actually find a node binary?

I was thinking about something similar. I think that the best solution will be to add those files when the devs update the version of RN to the one that support this feature. I mean adding these files during the yarn update step, if that's possible... But I'm not super expert of yarn, I have to research the topic a little bit.

Do we already have a react-native doctor command? I remember talking with @cortinico about adding it, so I think we don't have something similar, yet.

To start with, I think that good error messages and a proper documentation will be of a lot of help already.

@tido64
Copy link
Collaborator

tido64 commented Apr 4, 2022

Do we already have a react-native doctor command? I remember talking with @cortinico about adding it, so I think we don't have something similar, yet.

We do have one: https://github.com/react-native-community/cli/blob/master/docs/commands.md#doctor

Please don't create a new one 😅

@facebook-github-bot
Copy link
Contributor

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

cipolleschi pushed a commit to cipolleschi/react-native that referenced this pull request Apr 4, 2022
)

Summary:
Pull Request resolved: facebook#33546

This Diff does 2 things:
1. Removes all the remnant of the `find-node.sh` script. This allows React Native to stay agnostic from any other node manager
2. Introduces a way for the developers to specify which `node` executable they want to use, through a simple `.env` file.

## Changelog
[iOS][Changed] - This PR removes the `find-node.sh` scripts and replaces it with an `.xcode.env` file that is sourced by the script phases that needs it. The `.xcode.env` file is versioned: to customize a local environment, an unversioned `.xcode.local.env` can be used.

Differential Revision: D35317070

fbshipit-source-id: f1188c6b0b95c2358fecf3bb14791944a3d358cf
@cipolleschi cipolleschi requested a review from hramos as a code owner April 4, 2022 10:06
@pull-bot
Copy link

pull-bot commented Apr 4, 2022

Warnings
⚠️ 🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against 14fdac9

Comment on lines 18 to 42
else
echo "[Error] Could not find the .xcode.env file. " \
"Make sure to have that file properly configured with a valid configuration for the NODE_BINARY variable. " \
"You can set it up quickly by running: " \
"echo 'export NODE_BINARY=$(command -v node)' > .xcode.env " \
"in the ios folder." >&2
exit 1
fi
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as command -v node returns a valid path, I don't think we should fail. We should move the else-block further down.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I got your suggestion.
This error occurs when the .xcode.env file is missing and it contains a suggestion about how to configure it.

Plus, I removed node from my system folder and to me $(command -v node) returns an empty path when run from Xcode, while it returns a valid brew path when run outside Xcode.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think .xcode.env should be opt-in. As long as node can be found, we shouldn't stop because .xcode.env is missing.

Copy link
Collaborator

@tido64 tido64 Apr 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words:

ENV_PATH="$PODS_ROOT/../.xcode.env"
if [ -f "$ENV_PATH" ]; then
    source "$ENV_PATH"
fi

LOCAL_ENV_PATH="${ENV_PATH}.local"
if [ -f "$LOCAL_ENV_PATH" ]; then
    source "$LOCAL_ENV_PATH"
fi

if command -v node >/dev/null 2>&1; then
  # Execute argument, if present
  if [ -n "$1" ]; then
    # Use `$@` here to forward all arguments
    $@
  fi
else
  # Tell people how to set up .xcode.env so Xcode can find Node here
fi

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I like this approach. I think that introduces complexity and ambiguity.

Imagine that I have two different node managers and I setup one of them in my .env.xcode while the command -v node returns the other one (That was actually my setup before start working on this pr: the command -v node returned /usr/local/bin/node from Xcode but outside of Xcode was returning the nvm installation in my brew).

We are continuing the execution under the assumption that Xcode will use the node returned by command -v node while it will actually use the one in the .xcode.env file. I can see users get lost because of this misalignment. Also, they will need to actually know how Xcode works behind the curtains to understand that command -v node could return something different within the IDE.

And we lose the possibility to fail fast in case of misconfigurations.

I think that it's better to ask everyone to have a proper .xcode.env file configured on their system for the simplicity it could bring and the disambiguation it provides. Plus, it is not that we are adding a strong opinion on anything: people can keep using their favourite manager.

What do you think? does it make sense?

I'd like to have also @cortinico opinion on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have also @cortinico opinion on this.

My 2 cents here: have we considered keeping find-node-for-xcode.sh as a fallback scenario?

I know this would complicate the script a bit but it would provide a full fallback proof solution, that we can evolve further in the next release. Specifically something like:

export NODE_BINARY=$(command -v node)
source .xcode.env
source .xoce.env.local

if [ -z $NODE_BINARY]; then
  echo "[Warning] You need to configure your node path in the `'.xcode.env' file` environment." \
  "You can set it up quickly by running: " \
  "echo 'export NODE_BINARY=$(command -v node)' > .xcode.env " \
  " This is needed in order for React Native to work correctly. We will fallback to the deprecated behavior of finding `node`, but this will be deprecated in a future version of RN." \
  " You can read more about this here: <TODO-ADD LINK HERE>"
  source find-node.sh
fi

if [ -n $1 ]; then
  $1
fi

That's essentially what @tido64 suggested here: https://github.com/facebook/react-native/pull/33546/files#r842486987 but with a full fallback to the current behavior.

This will also print a warning to the users if the fallback is invoked, so we could safely remove find-node.sh in a future release.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposal of putting back find-node-for-xcode is more conservative and for sure gives us more space and time to improve the setup.

The [Warning] in the message is tricky: the script is executed in Xcode and the IDE does not have a particularly good way to present them. They got summered in a lot of other warnings and they will go unnoticed for sure...

I am not sure about what's the best path in this case.

Copy link
Collaborator

@tido64 tido64 Apr 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear, I do not want to bring back find-node.sh. As I said earlier:

All my issues have been with find-node.sh failing to find some random nvm config file that I don't use.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear, I do not want to bring back find-node.sh

Same for me, but if removing it creates so much trouble for our users and we end up creating a blocking scenario for most of our users, I'm in for deprecating it + moving to a better approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I updated the PR with another alternative.
I'm following the approach suggested by @cortinico, putting back the find-node-for-xcode.sh.

  1. Notice that that script is sourced as last alternative. Therefore, if command -v node works or the .xcode.env are set up properly, it won't be sourced (avoiding all the issues @tido64 and many other users may had in the past)
  2. In case of weird configurations, nothing changes w.r.t. the previous setup. So these changes will be transparent to all the users
  3. This gives us more leeway to smoothly transition away from the find-node.sh script.

I think that it is the best solution for this release. In the next one, we can finally dump the find-node.sh for good.

What do you think?

.gitignore Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

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

cipolleschi pushed a commit to cipolleschi/react-native that referenced this pull request Apr 5, 2022
)

Summary:
Pull Request resolved: facebook#33546

This Diff does 2 things:
1. Removes all the remnant of the `find-node.sh` script. This allows React Native to stay agnostic from any other node manager
2. Introduces a way for the developers to specify which `node` executable they want to use, through a simple `.env` file.

## Changelog
[iOS][Changed] - This PR removes the `find-node.sh` scripts and replaces it with an `.xcode.env` file that is sourced by the script phases that needs it. The `.xcode.env` file is versioned: to customize a local environment, an unversioned `.xcode.local.env` can be used.

Differential Revision: D35317070

fbshipit-source-id: 4973e9c6b4c39e8a60c44670f25799a1b723b07b
packages/rn-tester/.xcode.env Outdated Show resolved Hide resolved
# PATH to the node executable. You can use the `command -v node` to check what value make sense for your set up
# This `.xcode.env` file is versioned. To customize your local environment, you can create an `.xcode.env.local`
# file that is not versioned.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dot files inside the template should have be underscored _xcode.env and should be added to the cli:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 18 to 42
else
echo "[Error] Could not find the .xcode.env file. " \
"Make sure to have that file properly configured with a valid configuration for the NODE_BINARY variable. " \
"You can set it up quickly by running: " \
"echo 'export NODE_BINARY=$(command -v node)' > .xcode.env " \
"in the ios folder." >&2
exit 1
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have also @cortinico opinion on this.

My 2 cents here: have we considered keeping find-node-for-xcode.sh as a fallback scenario?

I know this would complicate the script a bit but it would provide a full fallback proof solution, that we can evolve further in the next release. Specifically something like:

export NODE_BINARY=$(command -v node)
source .xcode.env
source .xoce.env.local

if [ -z $NODE_BINARY]; then
  echo "[Warning] You need to configure your node path in the `'.xcode.env' file` environment." \
  "You can set it up quickly by running: " \
  "echo 'export NODE_BINARY=$(command -v node)' > .xcode.env " \
  " This is needed in order for React Native to work correctly. We will fallback to the deprecated behavior of finding `node`, but this will be deprecated in a future version of RN." \
  " You can read more about this here: <TODO-ADD LINK HERE>"
  source find-node.sh
fi

if [ -n $1 ]; then
  $1
fi

That's essentially what @tido64 suggested here: https://github.com/facebook/react-native/pull/33546/files#r842486987 but with a full fallback to the current behavior.

This will also print a warning to the users if the fallback is invoked, so we could safely remove find-node.sh in a future release.

)

Summary:
Pull Request resolved: facebook#33546

This Diff does 2 things:
1. Removes all the remnant of the `find-node.sh` script. This allows React Native to stay agnostic from any other node manager
2. Introduces a way for the developers to specify which `node` executable they want to use, through a simple `.env` file.

## Changelog
[iOS][Changed] - This PR removes the `find-node.sh` scripts and replaces it with an `.xcode.env` file that is sourced by the script phases that needs it. The `.xcode.env` file is versioned: to customize a local environment, an unversioned `.xcode.local.env` can be used.

Differential Revision: D35317070

fbshipit-source-id: d368fa95bab7e39fcdccec4962df337fb33a48c9
@facebook-github-bot
Copy link
Contributor

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

@cipolleschi
Copy link
Contributor Author

@TheSavior @tido64 I tried to add support to the .xcode.env file to the doctor command here. Let me know what do you think.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @cipolleschi in 0480f56.

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 Apr 11, 2022
Kudo added a commit to expo/expo that referenced this pull request Jul 25, 2022
# Why

the `source-login-scripts.sh` introduced a lot of pain where the community reported much build errors from it. it doesn't support shells other than zsh and bash. this pr find a new way to deal with xcode building issues that it cannot find the correct nodejs path.

close ENG-4864
close ENG-5242

# How

react-native introduces `.xcode.env` and `.xcode.env.local` for developers to override the `$NODE_BINARY`: facebook/react-native#33546

to make sure building success from expo-constants, expo-updates, and also the app target when generating bundles. i would like to reuse the `.xcode.env` and `.xcode.env.local` from react-native. this pr further generates `.xcode.env.local` automatically for the app during `pod install`. we can ensure that `pod install` is executed from shell and nodejs is available. so we will generate `export NODE_BINARY="$(command -v node)"` in the `.xcode.env.local`. for xcode, the path will be `$PODS_ROOT/../.xcode.env.local`, every xcode subprojects can source the file to get correct `$NODE_BINARY`.

# Test Plan

- building bare-expo from xcode (opening xcode by macos finder)
- building bare-expo by `yarn ios`
- updates e2e ci passed
- building expo-go (prerequisite: #18344) 

Co-authored-by: James Ide <[email protected]>
brentvatne pushed a commit to expo/expo that referenced this pull request Jul 25, 2022
# Why

the `source-login-scripts.sh` introduced a lot of pain where the community reported much build errors from it. it doesn't support shells other than zsh and bash. this pr find a new way to deal with xcode building issues that it cannot find the correct nodejs path.

close ENG-4864
close ENG-5242

# How

react-native introduces `.xcode.env` and `.xcode.env.local` for developers to override the `$NODE_BINARY`: facebook/react-native#33546

to make sure building success from expo-constants, expo-updates, and also the app target when generating bundles. i would like to reuse the `.xcode.env` and `.xcode.env.local` from react-native. this pr further generates `.xcode.env.local` automatically for the app during `pod install`. we can ensure that `pod install` is executed from shell and nodejs is available. so we will generate `export NODE_BINARY="$(command -v node)"` in the `.xcode.env.local`. for xcode, the path will be `$PODS_ROOT/../.xcode.env.local`, every xcode subprojects can source the file to get correct `$NODE_BINARY`.

# Test Plan

- building bare-expo from xcode (opening xcode by macos finder)
- building bare-expo by `yarn ios`
- updates e2e ci passed
- building expo-go (prerequisite: #18344) 

Co-authored-by: James Ide <[email protected]>
@Adnan-Bacic
Copy link
Contributor

Adnan-Bacic commented Jul 28, 2022

do we have to do something so that this file is "recognized"?

in the changes here i see these lines:

inputPaths = (
+ "$(SRCROOT)/.xcode.env.local",
+ "$(SRCROOT)/.xcode.env",
);

but i have no such lines in my own project. if i search my project for .xcode.env i only get results from .gitignore and the file itself. nothing in inputPaths

i also tried creating a new project running 0.69 and there i can see the inputPaths just like the changes shown here.

Solution

okay i found out how to get the desired result. these changes can be done in xcode.

you can see these changes are right under somewhere where it says: Begin PBXShellScriptBuildPhase section. under here it says Bundle React Native code and images.

you can find this section in xcode:

  1. open project
  2. click the project name under "Targets"
  3. go to the "Build Phases" tab
  4. here you will see a section with the name from earlier, Bundle React Native code and images. expand it
  5. here you will see a script. this is the one that is currently used which has to be changed. replace it with the new script for this update.
  6. still in the Bundle React Native code and images section, go to "Input Files" and add the 2 lines with the .xcode.env files.

then i can see the changes in git.

for reference, this is new script to paste in:

set -e

WITH_ENVIRONMENT="../node_modules/react-native/scripts/xcode/with-environment.sh"
REACT_NATIVE_XCODE="../node_modules/react-native/scripts/react-native-xcode.sh"

/bin/sh -c "$WITH_ENVIRONMENT $REACT_NATIVE_XCODE"

for simplicity, i created a new react native project running 0.69 and just copy-pasted the script.

@cipolleschi
Copy link
Contributor Author

@Adnan-Bacic: are you migrating to 0.69 or are you creating a new project?

When creating a new project, you should see a _xcode.env file close to your .xcodeproj and you should just rename it to .xcode.env (the cli should do this for you, but it is not working properly...). See here.

If you are migrating from a version < 0.69 to version 0.69, you have to create it yourself, as shown here, point 6.

To make it work, you just have to have the file as a sibling of the .xcodeproj file. Xcode will pick this up automatically. ;)

@Adnan-Bacic
Copy link
Contributor

im migrating from 0.68 to 0.69.

i created the file with the correct name(.xcode.env) in the correct location(ios/.xcode.env - this makes it a sibling of the .xcodeproj folder).

the filename still only appears the 2 files i mentioned in my previous comment.

i tried creating a new project and the filename appears in 5 files. the 2 mentioned + 2x project.pbxproj and FBReactNativeSpec.podspec.json.

new test project:
Screenshot 2022-07-29 at 12 54 46

project i updated from 0.68:
Screenshot 2022-07-29 at 12 55 31

after running bundle exec pod install it also appears in FBReactNaticeSpec.podspec.json file:
Screenshot 2022-07-29 at 13 17 53

though it still doesnt appear in the other 2 files, which im guessing it should.

@cipolleschi
Copy link
Contributor Author

I'm sorry but I'm a bit lost. What's the problem with that?
If the file is in the proper path, it will be picked up automatically.

Is it giving you build errors or something? If yes, could you share what's happening?

@danilobuerger
Copy link
Contributor

@Adnan-Bacic I think you didn't follow the upgrade helper properly. It shows how you need to modify your xcode project:
https://react-native-community.github.io/upgrade-helper/?from=0.68.0&to=0.69.0
Screenshot 2022-07-29 at 13 42 52

@Adnan-Bacic
Copy link
Contributor

I'm sorry but I'm a bit lost. What's the problem with that? If the file is in the proper path, it will be picked up automatically.

Is it giving you build errors or something? If yes, could you share what's happening?

no the project builds and it works. i just assume the file is not being used if it is not referenced anywhere. basically making it redundant unless i can see that the file path appears somewhere.

@Adnan-Bacic I think you didn't follow the upgrade helper properly. It shows how you need to modify your xcode project: https://react-native-community.github.io/upgrade-helper/?from=0.68.0&to=0.69.0 Screenshot 2022-07-29 at 13 42 52

but isent this an autogenerated file you are never supposed to manually change? usually this file will automatically adjust when some changes i made. for example, when changing the minimum supported ios version in this update from 11 to 12.4, i changed it in xcode, and this file was automatically updated, where it changed the IPHONEOS_DEPLOYMENT_TARGET value.

@danilobuerger
Copy link
Contributor

Same thing is true for modifying a script. You can either do it in XCode via the UI or you can modify the pbxproj directly. Same as you did with the deployment target.

@Adnan-Bacic
Copy link
Contributor

makes sense. but where in xcode would we change this and how? it seems to me like this comes from /node_modules/react-native/scripts/react-native-xcode.sh

@danilobuerger
Copy link
Contributor

This seems more like something you should ask in the appropriate channels (e.g. https://github.com/react-native-community/upgrade-support), not in a PR.

Ddv0623 pushed a commit to omniaintranet/expo that referenced this pull request Aug 3, 2022
# Why

the `source-login-scripts.sh` introduced a lot of pain where the community reported much build errors from it. it doesn't support shells other than zsh and bash. this pr find a new way to deal with xcode building issues that it cannot find the correct nodejs path.

close ENG-4864
close ENG-5242

# How

react-native introduces `.xcode.env` and `.xcode.env.local` for developers to override the `$NODE_BINARY`: facebook/react-native#33546

to make sure building success from expo-constants, expo-updates, and also the app target when generating bundles. i would like to reuse the `.xcode.env` and `.xcode.env.local` from react-native. this pr further generates `.xcode.env.local` automatically for the app during `pod install`. we can ensure that `pod install` is executed from shell and nodejs is available. so we will generate `export NODE_BINARY="$(command -v node)"` in the `.xcode.env.local`. for xcode, the path will be `$PODS_ROOT/../.xcode.env.local`, every xcode subprojects can source the file to get correct `$NODE_BINARY`.

# Test Plan

- building bare-expo from xcode (opening xcode by macos finder)
- building bare-expo by `yarn ios`
- updates e2e ci passed
- building expo-go (prerequisite: expo#18344) 

Co-authored-by: James Ide <[email protected]>
@Adnan-Bacic
Copy link
Contributor

Adnan-Bacic commented Aug 12, 2022

fair enough, but many other people have had similar problems with these files, and there arent any answers about how to actually solve it:

react-native-community/upgrade-support#191

react-native-community/upgrade-helper#270

react-native-community/upgrade-helper#255

react-native-community/upgrade-support#136

edit:

okay i found out how to solve this specific problem. edit can be found in my original question: #33546 (comment)

Ddv0623 pushed a commit to omniaintranet/expo that referenced this pull request Sep 26, 2022
# Why

the `source-login-scripts.sh` introduced a lot of pain where the community reported much build errors from it. it doesn't support shells other than zsh and bash. this pr find a new way to deal with xcode building issues that it cannot find the correct nodejs path.

close ENG-4864
close ENG-5242

# How

react-native introduces `.xcode.env` and `.xcode.env.local` for developers to override the `$NODE_BINARY`: facebook/react-native#33546

to make sure building success from expo-constants, expo-updates, and also the app target when generating bundles. i would like to reuse the `.xcode.env` and `.xcode.env.local` from react-native. this pr further generates `.xcode.env.local` automatically for the app during `pod install`. we can ensure that `pod install` is executed from shell and nodejs is available. so we will generate `export NODE_BINARY="$(command -v node)"` in the `.xcode.env.local`. for xcode, the path will be `$PODS_ROOT/../.xcode.env.local`, every xcode subprojects can source the file to get correct `$NODE_BINARY`.

# Test Plan

- building bare-expo from xcode (opening xcode by macos finder)
- building bare-expo by `yarn ios`
- updates e2e ci passed
- building expo-go (prerequisite: expo#18344) 

Co-authored-by: James Ide <[email protected]>
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
)

Summary:
Pull Request resolved: facebook#33546

This Diff does 2 things:
1. Removes all the remnant of the `find-node.sh` script. This allows React Native to stay agnostic from any other node manager
2. Introduces a way for the developers to specify which `node` executable they want to use, through a simple `.env` file.
[iOS][Changed] - This PR removes the `find-node.sh` scripts and replaces it with an `.xcode.env` file that is sourced by the script phases that needs it. The `.xcode.env` file is versioned: to customize a local environment, an unversioned `.xcode.local.env` can be used.

Reviewed By: cortinico

Differential Revision: D35317070

fbshipit-source-id: 4b400ba56aa2d574db563fa67b2008e1ddde1c59
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 Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.