[CI] Fix e2e detox build step #24953

nossbigg · 2019-05-19T16:24:38Z

Summary

Fixes the e2e detox build step by manually overriding PROJECT_ROOT for the project's JS bundle build step.

After seeing quite a few comments suggesting running some variant of the react-native bundle manually on your own so as to build the jsbundle required as part of the build step for RNTester project...

The main issue I found was that the working directory in which react-native-xcode.sh executed the CLI bundle step was not correct. The PROJECT_ROOT was not resolving to the root of the react-native project directory, but instead to something to the effect of /Users/gibson.cheng/IdeaProjects/react-native/../.. - of which of course the build step would not be able to find the react-native project to run the build against.

I'm not sure if new generated react-native projects require this manual override, so I only applied it to the RNTester project. Reviewers are welcome to correct my understanding and solutioning to this matter :)

@hramos, if this works, perhaps there would not be a need to push through with #24936. Also, this contributes to #23561.

Changelog

[Internal] [Fixed] - Fix build-ios-e2e build step

Test Plan

build-ios-e2e should work now

Manually provide PROJECT_ROOT for RNTester project js bundle build step

hramos · 2019-05-20T05:09:10Z

Thanks for the PR! I'll update mine and avoid disabling the Detox tests.

hramos

It looks like the Detox e2e tests finished successfully. Thanks for doing this. Could you verify if it would be possible to override the $PROJECT_ROOT envvar when invoking the react-native-xcode.sh script instead?

hramos · 2019-05-20T05:10:36Z

RNTester/RNTester.xcodeproj/project.pbxproj

 			outputPaths = (
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 			shellPath = /bin/sh;
-			shellScript = "export NODE_BINARY=node\n$SRCROOT/../scripts/react-native-xcode.sh RNTester/js/RNTesterApp.ios.js\n";
+			shellScript = "export NODE_BINARY=node\n$SRCROOT/../scripts/react-native-xcode.sh RNTester/js/RNTesterApp.ios.js $SRCROOT/..\n";


Suggested change

shellScript = "export NODE_BINARY=node\n$SRCROOT/../scripts/react-native-xcode.sh RNTester/js/RNTesterApp.ios.js $SRCROOT/..\n";

shellScript = "export NODE_BINARY=node\nPROJECT_ROOT=$SRCROOT/.. $SRCROOT/../scripts/react-native-xcode.sh RNTester/js/RNTesterApp.ios.js\n";

Would this work, in place of adding a second parameter to the script? It would remove the need for $PROJECT_ROOT_OVERRIDE.

Supplying PROJECT_ROOT via env var whilst starting the react-native-xcode.sh won't override the PROJECT_ROOT var inside the script, as the var is constructed inside the shell script (with no existing logic to accept overrides externally).

react-native/scripts/react-native-xcode.sh

Lines 55 to 60 in fb8a7c3

REACT_NATIVE_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"

# The project should be located next to where react-native is installed

# in node_modules.

PROJECT_ROOT=${PROJECT_ROOT:-"$REACT_NATIVE_DIR/../.."}

cd "$PROJECT_ROOT" || exit

I agree with you that supplying the PROJECT_ROOT_OVERRIDE via script arg is not as elegant as supplying it via env var. In the case of the latter approach, the initialization of the script will look something like:

PROJECT_ROOT_OVERRIDE=$SRCROOT/.. $SRCROOT/../scripts/react-native-xcode.sh RNTester/js/RNTesterApp.ios.js;

If this is workable, i'll go ahead with it 👍

My understanding is it should be overridable. The following line sets PROJECT_ROOT to the current value of PROJECT_ROOT, unless it is not defined, in which case it is set to "$REACT_NATIVE_DIR/../..".

PROJECT_ROOT=${PROJECT_ROOT:-"$REACT_NATIVE_DIR/../.."}

I verified that's the case in https://circleci.com/gh/hramos/react-native/5739
. I'll apply those changes and land.

RNTester/RNTester.xcodeproj/project.pbxproj

scripts/react-native-xcode.sh

facebook-github-bot

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

react-native-bot · 2019-05-20T17:30:07Z

This pull request was successfully merged by @nossbigg in adc0878.

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

nossbigg · 2019-05-20T21:40:17Z

Thanks for the merge, @hramos! :)

Summary: Fixes the e2e detox build step by manually overriding `PROJECT_ROOT` for the project's JS bundle build step. After seeing [quite](facebook#18472 (comment)) [a](facebook#18472 (comment)) [few](facebook#15432 (comment)) [comments](https://stackoverflow.com/a/49506072) suggesting running some variant of the `react-native bundle` manually on your own so as to build the jsbundle required as part of the build step for RNTester project... The main issue I found was that the working directory in which `react-native-xcode.sh` executed the CLI bundle step was not correct. The `PROJECT_ROOT` was not resolving to the root of the `react-native` project directory, but instead to something to the effect of `/Users/gibson.cheng/IdeaProjects/react-native/../..` - of which of course the build step would not be able to find the `react-native` project to run the build against. I'm not sure if new generated `react-native` projects require this manual override, so I only applied it to the RNTester project. Reviewers are welcome to correct my understanding and solutioning to this matter :) hramos, if this works, perhaps there would not be a need to push through with facebook#24936. Also, this contributes to facebook#23561. ## Changelog [Internal] [Fixed] - Fix build-ios-e2e build step Pull Request resolved: facebook#24953 Differential Revision: D15415850 Pulled By: hramos fbshipit-source-id: baaff09f81f01be4da1608e0b2898d037db35c23 (cherry picked from commit adc0878)

Summary: Fixes the e2e detox build step by manually overriding `PROJECT_ROOT` for the project's JS bundle build step. After seeing [quite](facebook#18472 (comment)) [a](facebook#18472 (comment)) [few](facebook#15432 (comment)) [comments](https://stackoverflow.com/a/49506072) suggesting running some variant of the `react-native bundle` manually on your own so as to build the jsbundle required as part of the build step for RNTester project... The main issue I found was that the working directory in which `react-native-xcode.sh` executed the CLI bundle step was not correct. The `PROJECT_ROOT` was not resolving to the root of the `react-native` project directory, but instead to something to the effect of `/Users/gibson.cheng/IdeaProjects/react-native/../..` - of which of course the build step would not be able to find the `react-native` project to run the build against. I'm not sure if new generated `react-native` projects require this manual override, so I only applied it to the RNTester project. Reviewers are welcome to correct my understanding and solutioning to this matter :) hramos, if this works, perhaps there would not be a need to push through with facebook#24936. Also, this contributes to facebook#23561. ## Changelog [Internal] [Fixed] - Fix build-ios-e2e build step Pull Request resolved: facebook#24953 Differential Revision: D15415850 Pulled By: hramos fbshipit-source-id: baaff09f81f01be4da1608e0b2898d037db35c23

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 May 19, 2019

react-native-bot added the Bug label May 19, 2019

Fix e2e RNTester build step

e608c4c

Manually provide PROJECT_ROOT for RNTester project js bundle build step

nossbigg force-pushed the fix-rntester-e2e-build branch from ece87d2 to e608c4c Compare May 19, 2019 16:41

nossbigg changed the title ~~Fix e2e RNTester build step~~ [CI] Fix e2e RNTester build step May 19, 2019

nossbigg changed the title ~~[CI] Fix e2e RNTester build step~~ [CI] Fix e2e detox build step May 19, 2019

hramos reviewed May 20, 2019

View reviewed changes

hramos mentioned this pull request May 20, 2019

[CI] Consolidate test_end_to_end back into other jobs #24936

Closed

hramos added the 🔩Test Infrastructure Test infrastructure and continuous integration. label May 20, 2019

Receive PROJECT_ROOT_OVERRIDE as env var

f8483ee

nossbigg force-pushed the fix-rntester-e2e-build branch from 67924c0 to f8483ee Compare May 20, 2019 10:49

hramos approved these changes May 20, 2019

View reviewed changes

hramos added 2 commits May 20, 2019 09:49

Update project.pbxproj

7ec300c

Update react-native-xcode.sh

a6679fc

facebook-github-bot approved these changes May 20, 2019

View reviewed changes

facebook-github-bot closed this in adc0878 May 20, 2019

react-native-bot added the Merged This PR has been merged. label May 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CI] Fix e2e detox build step #24953

[CI] Fix e2e detox build step #24953

nossbigg commented May 19, 2019 •

edited

Loading

hramos commented May 20, 2019

hramos left a comment

hramos May 20, 2019

hramos May 20, 2019

nossbigg May 20, 2019

hramos May 20, 2019 •

edited

Loading

facebook-github-bot left a comment

react-native-bot commented May 20, 2019

nossbigg commented May 20, 2019

	shellScript = "export NODE_BINARY=node\n$SRCROOT/../scripts/react-native-xcode.sh RNTester/js/RNTesterApp.ios.js $SRCROOT/..\n";
	shellScript = "export NODE_BINARY=node\nPROJECT_ROOT=$SRCROOT/.. $SRCROOT/../scripts/react-native-xcode.sh RNTester/js/RNTesterApp.ios.js\n";

	REACT_NATIVE_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
	# The project should be located next to where react-native is installed
	# in node_modules.
	PROJECT_ROOT=${PROJECT_ROOT:-"$REACT_NATIVE_DIR/../.."}

	cd "$PROJECT_ROOT" \|\| exit

[CI] Fix e2e detox build step #24953

[CI] Fix e2e detox build step #24953

Conversation

nossbigg commented May 19, 2019 • edited Loading

Summary

Changelog

Test Plan

hramos commented May 20, 2019

hramos left a comment

Choose a reason for hiding this comment

hramos May 20, 2019

Choose a reason for hiding this comment

hramos May 20, 2019

Choose a reason for hiding this comment

nossbigg May 20, 2019

Choose a reason for hiding this comment

hramos May 20, 2019 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented May 20, 2019

nossbigg commented May 20, 2019

nossbigg commented May 19, 2019 •

edited

Loading

hramos May 20, 2019 •

edited

Loading