-
Notifications
You must be signed in to change notification settings - Fork 984
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
upgrade react-native to 0.75.3 #21268
base: develop
Are you sure you want to change the base?
Conversation
Jenkins BuildsClick to see older builds (57)
|
169d36e
to
9539861
Compare
6a2ebae
to
8abe656
Compare
org.junit:junit-bom:5.9.3 | ||
org.junit:junit-bom:5.9.2 | ||
org.codehaus.mojo:animal-sniffer-annotations:1.23 | ||
com.android.tools.lint:lint-gradle:31.5.0' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to self : I need to try https://github.com/gradle/github-dependency-graph-gradle-plugin/tree/main#:~:text=Using%20the%20plugin%20to%20generate%20dependency%20reports as suggested by @mendelskiv93
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shit is tragic and unsustainable...
@@ -72,5 +100,7 @@ else | |||
nixOpts+=("--option" "build-use-sandbox" "true") | |||
fi | |||
|
|||
# needed since react-native v0.75 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a better comment
0dced72
to
96d667e
Compare
17180a0
to
9ed6679
Compare
96d667e
to
e469409
Compare
e64d894
to
3c5b671
Compare
e469409
to
307b76d
Compare
@@ -476,6 +478,10 @@ android-install: export BUILD_TYPE ?= release | |||
android-install: ##@other Install APK on device using adb | |||
adb install result/app-$(BUILD_TYPE).apk | |||
|
|||
generate-autolink-android: export TARGET := clojure | |||
generate-autolink-android: ##@other Generate autolinking.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if it's required for Android why is it not a dependency in the Makefile
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only need this in Jenkins file for Android build step, and this step runs one of the npm libraries react-native-config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually making this a dependency for android would be a good idea. I will try it out locally.
org.junit:junit-bom:5.9.3 | ||
org.junit:junit-bom:5.9.2 | ||
org.codehaus.mojo:animal-sniffer-annotations:1.23 | ||
com.android.tools.lint:lint-gradle:31.5.0' \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shit is tragic and unsustainable...
react-native config > "$AUTOLINKING_DIR/autolinking.json" | ||
echo "Generated autolinking.json" | ||
|
||
ROOT_VALUE=$(jq -r '.root' "$AUTOLINKING_DIR/autolinking.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a single comment explaining what this is and why we need to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that autolinking.json
generated contains a full static path to the dependencies in node_modules
and we face issues when android nix derivation tries to access those paths.
In this script i take the root key which is present in this json and which also contains the path to my project. similar to $GIT_ROOT and I replace that with ".." to make sure that the path becomes a relative path to node_modules
directory.
I'll add a comment and explain why I had to do this.
@@ -72,5 +72,7 @@ else | |||
nixOpts+=("--option" "build-use-sandbox" "true") | |||
fi | |||
|
|||
# needed since react-native v0.75 | |||
source "${GIT_ROOT}/scripts/generate_autolink_android.sh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you sourcing it if you don't use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my bad this maybe confusing, The thing is that generate_autolink_android.sh
has a function named generate_android_autolink()
and that function is called at the end of the script.
So sourcing it also ensures that the required function is executed.
@@ -49,6 +49,7 @@ pipeline { | |||
script { | |||
utils.symlinkEnv() | |||
println("Build Number: ${utils.genBuildNumber()}") | |||
sh 'make generate-autolink-android' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we doing this here? It seems kinda awkward do do this as part of Makefile
if it's always necessary for Android.
Why can't this be part of the Gradle build process for example? Is there ever a case where it don't need to run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do this in 2 places, here for PR builds and inside scripts/build-android.sh
for Debug builds which are run when we do make run-android
.
The autolinking json file was introduced originally as a gradle build step by react-native team.
However the issue with that is this autolinking stuff runs npx @react-native-community/cli config
at runtime and we can't run this inside our nix derivation since that runs in a sandbox environment.
Hence I have made these changes.
I have also patched out this behaviour from react-native to make sure it works well with our build process.
The second question about is there ever a case where it doesn't need to be run, well we only need the autolinking json file right before we have to do an android build. Otherwise we don't need it.
4a62604
to
0070bfc
Compare
Converting this PR to draft mode till the following things happen :
|
btw, i've looked to the code regarding |
@flexsurfer @siddarthkay is it really a must that we migrate to |
@flexsurfer : would you mind creating an issue for the migration? Thank you. |
0070bfc
to
898c6c4
Compare
@siddarthkay, you know I'm all in for keeping software up-to-date, but there's one contention point. At the moment, we are committed to deliver a continuous stream of work for Keycard because we have a soft deadline coming up. To give a bit more context, all other devs are allocated to important tasks and we anticipate some hiccups as usual in releases, therefore there's no other dev that can work in the Keycard stream while @flexsurfer works on the migration. Just a matter of capacity. @flexsurfer it would be helpful to know a rough, but realistic estimate of the time it would take to migrate to What I think we should do is migrate and upgrade after 2.31 is out in a few weeks because that will give us plenty of time to test and fix possible regressions. 2.32 won't take too long to be published after 2.31, but we will have a nice & safe window of time. This is more or less how I envision all risky upgrades, never too close to releases, ideally soon after a release. Does this plan make sense to everybody? cc @churik |
yeah I don't mind waiting till after 2.31, this upgrade is not urgent. |
blocked until 2.31 is cut |
14% of end-end tests have passed
Failed tests (6)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (1)Click to expandClass TestCommunityOneDeviceMerged:
|
@status-im/mobile-qa : pls ignore this E2E I was testing the new automation on this PR |
29% of end-end tests have passed
Not executed tests (1)Failed tests (5)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Passed tests (2)Click to expandClass TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
|
29% of end-end tests have passed
Not executed tests (1)Failed tests (5)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Passed tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
|
Summary
This PR upgraded
react-native
tov0.75.3
Following changes were also made :
NDK
upgraded to26.1.10909125
@react-native/gradle-plugin
is set up usingincludeBuild
gradle mechanism which is a better way.reactjs
is upgraded to18.3.1
react-native-config
is upgraded to1.5.3
react-native-navigation
is upgraded to7.40.1
react-native-reanimated
is upgraded to3.15.2
react-native
to prevent generatingautolinking.json
in a gradle step. We run gradle in a sandbox with--offline
so this is not feasible for us. We do the same thing viamake generate-autolink-android
in CI and this is also part ofscripts/build-android.sh
whenmake run-android
is called.AGP
is bumped to8.5.0
andaapt2
is bumped to8.5.0-11315950
-X
fromscript_phases-patched.sh
rn/pressible
becausern/touchable-without-feedback
breaks the rules of react.gradle
deps were updated.pods
were updated.Testing notes
Intense testing is required.
Platforms
Side Effects
status: ready