fix(local-e2e-script): add logic to handle maven local for iOS and Android accordingly #35104

kelset · 2022-10-27T13:06:30Z

Summary

This PR is a follow up of #35075 and 1546666 to ensure that even in the local e2e testing scenario the new maven approach is followed - without this, RNTestProject on Android won't work, like so:

And iOS will always build everything from source every time.

This PR addresses both by generating the artifacts locally, and passing them to RNTestProject as if they were coming from a url (mimicking as closely as possible the behaviour for the final user)

In doing so, there's been some refactoring to prep the ground for follow up work.

refactor CI to rely less on scripts directly in the CircleCI config, but invoke .js ones
we should be able to trigger more the "manual" artifacts generation so that it will only happen once between RNTester and RNTestProject, and we can pass existing artifacts to the other flows.
once all of this in place, a very good improvement would be to be able to download the maven artifacts kind of like nightlies and stables do. This will only be viable by checking that there's no local changes, after which there needs to be logic to pull down from CircleCI the artifacts based on git commit <-> circleCI job references.

While at it, I've also fixed the hermes-engine podspec logic for detecting if it's on CI: basically the local e2e script needs to align with the changes done here: 4b51207

but as you can see there, the condition was actually inconsistent across the various files, so realigned to CI === 'true'. We probably didn't catch that so far 'cause the other condition in the hermes podspect (existence of hermestag_file) is only true on release branches and this new logic has not been in any release branches yet.

Changelog

[Internal] [Fixed] - add logic to local e2e script to handle maven local for iOS and Android accordingly

Test Plan

Run yarn test-e2e-local -t RNTestProject -p Android successfully.

Run yarn test-e2e-local -t RNTestProject -p iOS successfully. On the pod install stage, you will see [Hermes] Using pre-built Hermes binaries from local path.

analysis-bot · 2022-10-27T13:23:58Z

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

Base commit: 64c3906
Branch: main

analysis-bot · 2022-10-27T13:51:39Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	6,992,247	+0
android	hermes	armeabi-v7a	6,368,778	+0
android	hermes	x86	7,404,941	+0
android	hermes	x86_64	7,268,948	+0
android	jsc	arm64-v8a	8,855,839	+0
android	jsc	armeabi-v7a	7,594,355	+0
android	jsc	x86	8,913,589	+0
android	jsc	x86_64	9,396,951	+0

Base commit: 64c3906
Branch: main

cipolleschi · 2022-10-28T14:27:23Z

scripts/test-e2e-local.js

+    // FIXME: need to figure out how to generate the hermes stuff
+    // (can I just do a pod install for RNTester iOS and use those files? need to find where they are stored)
+    // and get the Hermes reference here for the next phase
+    const hermesDir = 'help me';


As discussed on discord, we should probably call these two scripts:

build_mac_framework

build_ios_framework
Then, we need to prepare the apple frameworks and finally, we can invoke createHermessTarball.

I still believe that we should also provide a way where we tell the scripts from where it can download all the stuff, saving hours of building, though.

But let's start by having a version that works!

hramos · 2022-10-31T16:42:06Z

@kelset I am planning to rename the tarball methods in hermes-utils.js in order to make a clear distinction between the different tarballs that we deal with:

Hermes source code tarball, generated by GitHub and downloaded from github.com/facebook/hermes
Hermes prebuilt artifact tarball, generated by Circle CI and downloaded from Maven
Hermes debug symbols tarball, generated by Circle CI and downloaded from Maven (WIP)

Here's the PR: #35156 - I have updated all the internal references to these methods.

I realize this would conflict with your PR. What do you think? Would renaming these have an adverse impact on other scripts from the community? Would you be okay rebasing your work on top of #35156 if it is merged?

cipolleschi

Nice! Practically, we may potentially use the same generateiOSArtifacts in CI directly! XD

kelset · 2022-10-31T17:13:38Z

@hramos go ahead and merge yours, yeah I'll have conflicts but I'm still in the testing phase for this one so it's likely that your PR will be quicker to land

cipolleschi · 2022-11-01T09:14:25Z

Question/idea for a later PR: do we want to setup a circleci job that runs this script? In that way, if someone commits something that could potentially break it, the check will ask him to also fix the script.

WDYT?

kelset · 2022-11-01T12:07:52Z

update: now the PR should be ready, I've run a few more tests and done more tweaks. Every flow seems to be working as expected.

re: @cipolleschi

Question/idea for a later PR: do we want to setup a circleci job that runs this script? In that way, if someone commits something that could potentially break it, the check will ask him to also fix the script.

No, I don't think there's value in running this script specifically - on CI it'd basically just build RNTester and RNTestProject, take a lot of time and not verify much that is not already verified by other CI jobs. The main advantage of this script is in the local scenario, where you want to have 1 handy command to get easily to have a local build working so that you can play around with it in the emulator/simulator.

This topic overlaps with the E2E conversation that is been happening in parallel, so I'm sure that we'll be able to do some refactoring so that both the E2E flows and this script will have less custom code and we can generalize certain sections

facebook-github-bot · 2022-11-01T12:13:29Z

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

kelset · 2022-11-01T13:50:34Z

scripts/test-e2e-local.js

-    }
+    // set HERMES_ENGINE_TARBALL_PATH to point to the local artifacts I just created
+    exec(
+      `HERMES_ENGINE_TARBALL_PATH=${tarballOutputPath} USE_HERMES=${


this part I don't particularly like because it means that if the tester needs to test something else with RNTestProject - ex. test that everything still works on JSC, they will have to do a new pod install without hermes, but if after that they want to test Hermes again they will have to manually pass this HERMES_ENGINE_TARBALL_PATH variable during the pod install :(

Sadly because of limitations of shelljs we can't set ENV variables through it, and since there's no gradle.properties equivalent we're stuck with this. Hopefully it won't create many attritions

facebook-github-bot · 2022-11-01T14:08:19Z

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

scripts/release-utils.js

scripts/test-e2e-local.js

cipolleschi

left a nit comment with some context.

…ll use the local maven artifacts

This reverts commit 620a810.

…eded

…Project will succeed

…r RNTestProject will succeed" This reverts commit 0398ace.

facebook-github-bot · 2022-11-02T11:29:36Z

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

facebook-github-bot · 2022-11-02T12:29:29Z

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

facebook-github-bot · 2022-11-02T13:10:30Z

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

facebook-github-bot · 2022-11-02T14:07:17Z

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

kelset · 2022-11-02T14:10:16Z

quick headsup, I've updated this PR to account for some of the commits that landed in the last 12/24 hrs, such as:

And especially 6b8e13f which for some reason did add some new scripts but not as executable, so took care of it.

Before:

After:

kelset · 2022-11-02T14:29:11Z

final update (I hope 🤣):
everything working as expected:

facebook-github-bot · 2022-11-02T14:45:20Z

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

react-native-bot · 2022-11-02T15:35:05Z

This pull request was successfully merged by @kelset in c540ff7.

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

…droid accordingly (facebook#35104) Summary: This PR is a follow up of facebook#35075 and facebook@1546666 to ensure that even in the local e2e testing scenario the new maven approach is followed - without this, RNTestProject on Android won't work, like so: <img width="1905" alt="Screenshot 2022-10-27 at 12 15 38" src="https://user-images.githubusercontent.com/16104054/198334105-30fb2037-4e7c-4814-8c3f-2412ba0bd49f.png"> And iOS will always build everything from source every time. This PR addresses both by generating the artifacts locally, and passing them to RNTestProject as if they were coming from a url (mimicking as closely as possible the behaviour for the final user) In doing so, there's been some refactoring to prep the ground for follow up work. * refactor CI to rely less on scripts directly in the CircleCI config, but invoke .js ones * we should be able to trigger more the "manual" artifacts generation so that it will only happen once between RNTester and RNTestProject, and we can pass existing artifacts to the other flows. * once all of this in place, a very good improvement would be to be able to download the maven artifacts kind of like nightlies and stables do. This will only be viable by checking that there's no local changes, after which there needs to be logic to pull down from CircleCI the artifacts based on git commit <-> circleCI job references. --- While at it, I've also fixed the hermes-engine podspec logic for detecting if it's on CI: basically the local e2e script needs to align with the changes done here: facebook@4b51207 but as you can see there, the condition was actually inconsistent across the various files, so realigned to `CI === 'true'`. We probably didn't catch that so far 'cause the other condition in the hermes podspect (existence of `hermestag_file`) is only true on release branches and this new logic has not been in any release branches yet. ## Changelog  [Internal] [Fixed] - add logic to local e2e script to handle maven local for iOS and Android accordingly Pull Request resolved: facebook#35104 Test Plan: Run ` yarn test-e2e-local -t RNTestProject -p Android` successfully. Run ` yarn test-e2e-local -t RNTestProject -p iOS` successfully. On the pod install stage, you will see `[Hermes] Using pre-built Hermes binaries from local path.` Reviewed By: dmytrorykun Differential Revision: D40893239 Pulled By: cipolleschi fbshipit-source-id: a31217ec4f177383c62292d00fabc4cbe4391cfd

kelset force-pushed the kelset/ensure-e2e-works-with-new-maven branch from 0d6979c to cb123ee Compare October 27, 2022 13:27

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 Oct 27, 2022

cortinico changed the title ~~make rngp consider the local e2e test case~~ Extend the local e2e test case to provide the REACT_NATIVE_MAVEN_LOCAL_REPO property Oct 27, 2022

react-native-bot added the Bug label Oct 27, 2022

kelset force-pushed the kelset/ensure-e2e-works-with-new-maven branch from d8feea7 to d484acf Compare October 28, 2022 10:46

kelset changed the title ~~Extend the local e2e test case to provide the REACT_NATIVE_MAVEN_LOCAL_REPO property~~ fix(local-e2e-script): add logic to handle maven local for iOS and Android accordingly Oct 28, 2022

kelset requested review from cipolleschi and cortinico October 28, 2022 14:16

cipolleschi reviewed Oct 28, 2022

View reviewed changes

kelset force-pushed the kelset/ensure-e2e-works-with-new-maven branch from 72f3ca4 to a643659 Compare October 31, 2022 11:41

cipolleschi approved these changes Oct 31, 2022

View reviewed changes

kelset force-pushed the kelset/ensure-e2e-works-with-new-maven branch from babac46 to 3befaef Compare November 1, 2022 10:25

kelset marked this pull request as ready for review November 1, 2022 11:51

kelset requested review from hramos and cipolleschi November 1, 2022 12:08

kelset commented Nov 1, 2022

View reviewed changes

dmytrorykun reviewed Nov 1, 2022

View reviewed changes

scripts/release-utils.js Outdated Show resolved Hide resolved

scripts/test-e2e-local.js Outdated Show resolved Hide resolved

cipolleschi approved these changes Nov 1, 2022

View reviewed changes

kelset added 4 commits November 2, 2022 11:07

make rngp consider the local e2e test case

47d5804

need to clean also the maven-local folder

106c573

by adding this, we should be able to ensure that the RNTestProject wi…

4e7ee79

…ll use the local maven artifacts

Revert "make rngp consider the local e2e test case"

28ce2f7

This reverts commit 620a810.

kelset added 10 commits November 2, 2022 11:07

update the script to manually trigger the generation of all that's ne…

08cb75a

…eded

fix

f15a052

getting closer to have it fully working

f11b931

post Hector commit change

db138ff

some cleanup

00c9451

add one more comment for a follow up

5dbb564

a bit more cleanup, and work

ce929f1

fix android working even when I generate the RNTestProject for iOS

2ff8975

setting the iOS tarball variable so that every pod install for RNTest…

664e7cf

…Project will succeed

Revert "setting the iOS tarball variable so that every pod install fo…

838bcd9

…r RNTestProject will succeed" This reverts commit 0398ace.

kelset force-pushed the kelset/ensure-e2e-works-with-new-maven branch from 0609a02 to 838bcd9 Compare November 2, 2022 11:08

feedback

34c9a3c

kelset added 2 commits November 2, 2022 12:05

update to match what's going on in latest commits

f4d70e9

typo

651f065

give correct rights to the new scripts

3b288da

kelset added 2 commits November 2, 2022 13:48

move pod install outside

f7e9352

changing logic to account for moving the utils script into hermes source

8f7a6ed

facebook-github-bot closed this in c540ff7 Nov 2, 2022

kelset deleted the kelset/ensure-e2e-works-with-new-maven branch November 2, 2022 15:32

react-native-bot added the Merged This PR has been merged. label Nov 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(local-e2e-script): add logic to handle maven local for iOS and Android accordingly #35104

fix(local-e2e-script): add logic to handle maven local for iOS and Android accordingly #35104

kelset commented Oct 27, 2022 •

edited

Loading

analysis-bot commented Oct 27, 2022 •

edited

Loading

analysis-bot commented Oct 27, 2022 •

edited

Loading

cipolleschi Oct 28, 2022

hramos commented Oct 31, 2022

cipolleschi left a comment

kelset commented Oct 31, 2022

cipolleschi commented Nov 1, 2022

kelset commented Nov 1, 2022 •

edited

Loading

facebook-github-bot commented Nov 1, 2022

kelset Nov 1, 2022 •

edited

Loading

facebook-github-bot commented Nov 1, 2022

cipolleschi left a comment

facebook-github-bot commented Nov 2, 2022

facebook-github-bot commented Nov 2, 2022

facebook-github-bot commented Nov 2, 2022

facebook-github-bot commented Nov 2, 2022

kelset commented Nov 2, 2022

kelset commented Nov 2, 2022

facebook-github-bot commented Nov 2, 2022

react-native-bot commented Nov 2, 2022

fix(local-e2e-script): add logic to handle maven local for iOS and Android accordingly #35104

fix(local-e2e-script): add logic to handle maven local for iOS and Android accordingly #35104

Conversation

kelset commented Oct 27, 2022 • edited Loading

Summary

Changelog

Test Plan

analysis-bot commented Oct 27, 2022 • edited Loading

analysis-bot commented Oct 27, 2022 • edited Loading

cipolleschi Oct 28, 2022

Choose a reason for hiding this comment

hramos commented Oct 31, 2022

cipolleschi left a comment

Choose a reason for hiding this comment

kelset commented Oct 31, 2022

cipolleschi commented Nov 1, 2022

kelset commented Nov 1, 2022 • edited Loading

facebook-github-bot commented Nov 1, 2022

kelset Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Nov 1, 2022

cipolleschi left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 2, 2022

facebook-github-bot commented Nov 2, 2022

facebook-github-bot commented Nov 2, 2022

facebook-github-bot commented Nov 2, 2022

kelset commented Nov 2, 2022

kelset commented Nov 2, 2022

facebook-github-bot commented Nov 2, 2022

react-native-bot commented Nov 2, 2022

kelset commented Oct 27, 2022 •

edited

Loading

analysis-bot commented Oct 27, 2022 •

edited

Loading

analysis-bot commented Oct 27, 2022 •

edited

Loading

kelset commented Nov 1, 2022 •

edited

Loading

kelset Nov 1, 2022 •

edited

Loading