android build: Follow some changes from RN v0.68 #5603
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
chrisbobbe
requested changes
Dec 13, 2022
tools/test
Outdated
Comment on lines
148
to
156
| ( | ||
| cd android | ||
|
|
||
| ./gradlew -q :app:assembleDebug :app:assembleDebugUnitTest \ | ||
| :app:bundleDebug || exit | ||
|
|
||
| # The `-q` suppresses noise from warnings about obsolete build config | ||
| # in our dependencies from the React Native ecosystem. | ||
| # But it also suppresses the names of tests that failed. | ||
| # So on failure, rerun without it. | ||
| ./gradlew -q :app:testDebugUnitTest \ | ||
| || ./gradlew :app:testDebugUnitTest || exit | ||
| ) | ||
| tools/gradle -q :app:assembleDebug :app:assembleDebugUnitTest \ | ||
| :app:bundleDebug || exit | ||
|
|
||
| # The `-q` suppresses noise from warnings about obsolete build config | ||
| # in our dependencies from the React Native ecosystem. | ||
| # But it also suppresses the names of tests that failed. | ||
| # So on failure, rerun without it. | ||
| tools/gradle -q :app:testDebugUnitTest \ | ||
| || tools/gradle :app:testDebugUnitTest || exit |
There was a problem hiding this comment.
Hmm, now it seems that an Android native test failure makes the whole tools/test command exit rather than just report a failure for the native suite.
If I introduce an Android error in FcmMessageTest.kt, and a Flow and a Jest error, I get the below output, when I expected output ending in FAILED: native flow jest:
$ tools/test
Running native...
Running Android native tests...
Using expo modules
- expo-application (4.1.0)
- expo-constants (13.1.1)
- expo-error-recovery (3.1.0)
- expo-file-system (14.0.0)
- expo-font (10.1.0)
- expo-keep-awake (10.1.1)
- expo-modules-core (0.9.2)
- expo-screen-orientation (4.2.0)
- expo-sqlite (10.2.0)
- expo-web-browser (10.2.1)
Using expo modules
- expo-application (4.1.0)
- expo-constants (13.1.1)
- expo-error-recovery (3.1.0)
- expo-file-system (14.0.0)
- expo-font (10.1.0)
- expo-keep-awake (10.1.1)
- expo-modules-core (0.9.2)
- expo-screen-orientation (4.2.0)
- expo-sqlite (10.2.0)
- expo-web-browser (10.2.1)
14 tests completed, 1 failed
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':app:testDebugUnitTest'.
> There were failing tests. See the report at: file:///Users/chrisbobbe/dev/zulip-mobile/android/app/build/reports/tests/testDebugUnitTest/index.html
* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.
* Get more help at https://help.gradle.org
BUILD FAILED in 15s
> Configure project :app
Warning: The 'kotlin-android-extensions' Gradle plugin is deprecated. Please use this migration guide (https://goo.gle/kotlin-android-extensions-deprecation) to start working with View Binding (https://developer.android.com/topic/libraries/view-binding) and the 'kotlin-parcelize' plugin.
> Configure project :expo
Using expo modules
- expo-application (4.1.0)
- expo-constants (13.1.1)
- expo-error-recovery (3.1.0)
- expo-file-system (14.0.0)
- expo-font (10.1.0)
- expo-keep-awake (10.1.1)
- expo-modules-core (0.9.2)
- expo-screen-orientation (4.2.0)
- expo-sqlite (10.2.0)
- expo-web-browser (10.2.1)
> Task :expo-constants:createDebugExpoConfig
Execution optimizations have been disabled for task ':expo-constants:createDebugExpoConfig' to ensure correctness due to the following reasons:
- Gradle detected a problem with the following location: '/Users/chrisbobbe/dev/zulip-mobile/android'. Reason: Task ':expo-constants:createDebugExpoConfig' uses this output of task ':app:checkDebugAarMetadata' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.3.3/userguide/validation_problems.html#implicit_dependency for more details about this problem.
- Gradle detected a problem with the following location: '/Users/chrisbobbe/dev/zulip-mobile/android'. Reason: Task ':expo-constants:createDebugExpoConfig' uses this output of task ':app:compileDebugAidl' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.3.3/userguide/validation_problems.html#implicit_dependency for more details about this problem.
- Gradle detected a problem with the following location: '/Users/chrisbobbe/dev/zulip-mobile/android'. Reason: Task ':expo-constants:createDebugExpoConfig' uses this output of task ':app:compileDebugRenderscript' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.3.3/userguide/validation_problems.html#implicit_dependency for more details about this problem.
- Gradle detected a problem with the following location: '/Users/chrisbobbe/dev/zulip-mobile/android'. Reason: Task ':expo-constants:createDebugExpoConfig' uses this output of task ':app:compileDebugShaders' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.3.3/userguide/validation_problems.html#implicit_dependency for more details about this problem.
- Gradle detected a problem with the following location: '/Users/chrisbobbe/dev/zulip-mobile/android'. Reason: Task ':expo-constants:createDebugExpoConfig' uses this output of task ':app:copyReactNativeVectorIconFonts' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.3.3/userguide/validation_problems.html#implicit_dependency for more details about this problem.
- Gradle detected a problem with the following location: '/Users/chrisbobbe/dev/zulip-mobile/android'. Reason: Task ':expo-constants:createDebugExpoConfig' uses this output of task ':app:generateDebugBuildConfig' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.3.3/userguide/validation_problems.html#implicit_dependency for more details about this problem.
- Gradle detected a problem with the following location: '/Users/chrisbobbe/dev/zulip-mobile/android'. Reason: Task ':expo-constants:createDebugExpoConfig' uses this output of task ':app:mergeDebugShaders' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.3.3/userguide/validation_problems.html#implicit_dependency for more details about this problem.
> Task :app:testDebugUnitTest FAILED
com.zulipmobile.notifications.MessageFcmMessageTest > fields get parsed right in 'message' happy path FAILED
com.google.common.truth.Truth$SimpleAssertionError
14 tests completed, 1 failed
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':app:testDebugUnitTest'.
> There were failing tests. See the report at: file:///Users/chrisbobbe/dev/zulip-mobile/android/app/build/reports/tests/testDebugUnitTest/index.html
* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.
* Get more help at https://help.gradle.org
Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.
You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.
See https://docs.gradle.org/7.3.3/userguide/command_line_interface.html#sec:command_line_warnings
Execution optimizations have been disabled for 1 invalid unit(s) of work during this build to ensure correctness.
Please consult deprecation warnings for more details.
BUILD FAILED in 15s
431 actionable tasks: 8 executed, 423 up-to-date
And atop main just now I did get output ending in FAILED: native flow jest.
There was a problem hiding this comment.
Ah good catch, thanks. I even did this part:
If I introduce an Android error in
FcmMessageTest.kt,
though not the others, and apparently didn't look close enough to notice that the other suites didn't run.
I think the fix is just that those || exit should become || return. (And only the one that's not the last command in the function is still needed. Likely only that one was needed in the first place.) They were exit because the ( … ) meant a subshell, i.e. a new shell process as a child process of the main one; but now that they're running in the main shell process (because there's no longer that cd that needs a subshell to contain its effects), that's the wrong thing.
And simplify various scripts and docs accordingly.
Done to parallel the RN template app at facebook/react-native@cd4c6659d, on the way to RN v0.68. Upstream release notes indicate it's a handful of bug fixes: https://docs.gradle.org/7.3.3/release-notes.html including some for the major vulnerability last year in Log4j. (This release went out 2021-12-22.)
Done to parallel the RN template app at facebook/react-native@cd4c6659d, on the way to RN v0.68. Upstream release notes are... well, if they existed they would be under "7.0.0 (July 2021)" here: https://developer.android.com/studio/releases/past-releases#agp but it looks like that doesn't mention any 7.0.x after 7.0.1. In any case, the version number means this should be a bugfix release.
This corresponds to two changes in the RN template app, leading up to RN v0.68: facebook/react-native@43c38cdc8 Allow specifying an architecture in RNTester and release builds facebook/react-native@0f39a1076 Make the `reactNativeArchitectures` property more discoverable Nothing will break without this, but it seems like potentially a convenient feature sometimes -- speed up an iteration cycle by limiting the build to just one architecture. And including it helps keep down the diff from the template app, slightly simplifying future RN upgrades.
After the upcoming RN v0.68 upgrade, I start seeing failures in `tools/test native` with the error message "Java heap space". So, I guess we need to let Gradle's JVM use still more RAM. Previously in 1e2390d in 2020-09 we increased this limit from half a GiB to a whole GiB. Make it 1.25 GiB; that seems to be enough for now. Compare facebook/react-native@a2a703247, which happened on the way to RN v0.68. That one had the explanation > Updates maximum heap size for the gradle build to account for > building RN from source when new architecture is enabled. which doesn't apply as we aren't yet adopting RN's New Architecture. But perhaps if we did, we'd need to increase the heap size even more: that commit takes it to 2 GiB. Even on my desktop with its 32 GiB of RAM, that kind of memory consumption can get to be a strain together with everything else: Android Studio; an Android emulator; a browser; VS Code, which is basically another browser; Flow; and so on. More RAM used by programs directly means less for the system's disk cache, which means everything runs slower as the machine keeps re-reading stuff from disk, stuff that it just read a little while ago but didn't have the spare memory to keep around. Hence trying to keep this number down as long as we can.
gnprice
force-pushed
the
pr-android-build
branch
from
e611f7d to
3677788
Compare
|
Thanks for the review! Fixed and merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #5344.
Plus a handly little
tools/gradlescript, to save typingcd androidandcd ..all the time when running Gradle tasks (and to simplify various scripts and docs by not having to track a different working directory.)