React native composite and binary build support #29488
Conversation
cameronvoell
added
the
Mobile App - i.e. Android or iOS
There was a problem hiding this comment.
This is neat work, thanks @oguzkocer!
I left a few questions and minor suggestions/nitpicks. Nothing that needs to be addressed before a merge, or at all.
Haven't run the demo app yet.
packages/react-native-bridge/android/react-native-bridge/build.gradle
Outdated
Show resolved
Hide resolved
packages/react-native-bridge/android/react-native-bridge/src/main/res/values/colors.xml
Outdated
Show resolved
Hide resolved
packages/react-native-bridge/android/publish-aztec-and-bridge.sh
Outdated
Show resolved
Hide resolved
| ./gradlew --help > /dev/null 2>&1 | ||
| gradlew_exists=$? | ||
| if [ $gradlew_exists -ne 0 ]; then | ||
| echo "Gradle wrapper is not available. Make sure to run this script from '/packages/react-native-bridge/android'" |
There was a problem hiding this comment.
Why are we just checking whether gradlew can be invoked instead of checking that we're in packages/react-native-bridge/android. I don't think this is a very big deal, I'm more just curious if perhaps there are some considerations I'm overlooking.
There was a problem hiding this comment.
That's a good question. Let me see if I can do that instead, particularly, let me see if I can also verify whether we are in a submodule gutenberg-mobile per this comment.
There was a problem hiding this comment.
I've improved this check in 39b06ca (and then improved the messaging of it in 291441e), now it does the following:
- As the first step, we'll
cdinto the directory of the script. This way, we should be able to call the script from anywhere. 🤞 - Check that script is being run from
gutenberg-mobilesubmodule. Note that this works even if the script was originally called from thegutenberg-mobilefolder, or any other folder for that matter, because of the first change. We do this check by retrieving the parent repo's path withgit rev-parse --show-superproject-working-treeand then by checking thatoriginurl ends with/gutenberg-mobile.git.
These checks all worked in my local tests, however it doesn't mean it covers all cases. For example, if the gutenberg-mobile repo doesn't have origin as one of the remotes, the script will fail even though the developer might be using a different name for it. Having said that, I don't think it'd be valuable to make these checks any more intricate than it currently is. I think we have covered most important cases and will start getting diminishing returns from any extra checks.
@mokagio @mchowning What do you think?
| cp ../../../../bundle/android/App.js ./react-native-bridge/build/assets/index.android.bundle | ||
|
|
||
| if [ $? -ne 0 ]; then | ||
| echo "Make sure to run 'npm install' & 'npm run bundle:android' from the root folder of the project before running this script." |
There was a problem hiding this comment.
Would it be better to include those commands in this script so that way we know that everything is completely up-to-date?
There was a problem hiding this comment.
Thinking about this some more, this script will actually only work if it is run from a Gutenberg repo that is "inside" gutenberg-mobile. Perhaps we should include a check for that. I'm not sure about the best approach here. 🤔
There was a problem hiding this comment.
Would it be better to include those commands in this script so that way we know that everything is completely up-to-date?
I don't feel too comfortable with that idea. The main reason is I don't like when a script does more than what it says it'll do. In this case, I think this script should only publish aztec and bridge libraries. If we really need something like that, I'd rather add a second script in the gutenberg-mobile repo that calls this script after it sets everything up. However, I am not sure if that'll actually be useful since this script shouldn't really be run by developers. Let me know if you feel otherwise.
Aside from that main reason, this would require us to run commands from a parent directory, which again is not something I particularly like to do.
Thinking about this some more, this script will actually only work if it is run from a Gutenberg repo that is "inside" gutenberg-mobile. Perhaps we should include a check for that. I'm not sure about the best approach here. 🤔
I think this comment also covers this issue, so I'll respond to it there.
There was a problem hiding this comment.
However, I am not sure if that'll actually be useful since this script shouldn't really be run by developers.
+1.
We could go down the path of adding CLI options to do the necessary setup work if required, but since CI should do all that for us, I, too, think keeping this script single tasked and "honest" to its name is a better approach.
There was a problem hiding this comment.
I am not sure if that'll actually be useful since this script shouldn't really be run by developers
Ah, I didn't realize this wasn't for use by developers. If a developer wanted to create an apk that included their changes in the js bundle, how would they go about doing that (i.e., if they wanted to send an apk to a designer for early feedback)?
There was a problem hiding this comment.
I can think of 2 main ways for it:
- Use composite build and manually create an apk/aab.
- Create a draft
gutenberg-mobilePR which should publish a new version to Bintray. Then draft a WPAndroid PR with that version number. The apk will be created and posted to the WPAndroid PR as a comment. (If this becomes a common flow, we could automate this for you)
The script can be used to locally publish a version, but I don't think we consider is a main use case. Let me know if you feel otherwise.
P.S: If you can let us know some common workflows, we can include "how to"s for them in the announcement post .
There was a problem hiding this comment.
I can confirm the upload script works in CI.
From my point of view, this is ready to merge.
| PARENT_GIT_REPO_PATH=$(git rev-parse --show-superproject-working-tree) | ||
| if [[ -z $PARENT_GIT_REPO_PATH || | ||
| $(git -C $PARENT_GIT_REPO_PATH config --get remote.origin.url) != *"/gutenberg-mobile.git" ]]; then | ||
| echo "This script needs to be run from the 'gutenberg-mobile' submodule." |
There was a problem hiding this comment.
🤔 totally out of scope right now, but given this is a hard constraint for the script execution, should we move it in gutenberg-mobile and avoid the need for this check?
It's easier to iterate on the script if it lives in Gutenberg. Once it's stable though, I think we should consider that option.
There was a problem hiding this comment.
I like the idea of moving this to Gutenberg-Mobile
There was a problem hiding this comment.
That sounds good to me. However, I'd prefer not to deal with that before the merge and considering we'll need to update this script when we switch away from Bintray, I think that'd be the best time to make the move. I've opened an issue in gutenberg-mobile and assigned it to myself: wordpress-mobile/gutenberg-mobile#3238
| PARENT_GIT_REPO_PATH=$(git rev-parse --show-superproject-working-tree) | ||
| if [[ -z $PARENT_GIT_REPO_PATH || | ||
| $(git -C $PARENT_GIT_REPO_PATH config --get remote.origin.url) != *"/gutenberg-mobile.git" ]]; then | ||
| echo "This script needs to be run from the 'gutenberg-mobile' submodule." |
There was a problem hiding this comment.
The message tricked me up, it made me think I should cd into the gutenberg-mobile submodule of this project, while the requirement is that the script is called when this project is a submodule of gutenberg-mobile.
Would you consider this wording?
| echo "This script needs to be run from the 'gutenberg-mobile' submodule." | |
| echo "Could not find `gutenberg-mobile` parent folder. This script can run only when this project is contained within `gutenberg-mobile` as a Git submodule." |
There was a problem hiding this comment.
Reading it again, I agree that it is not a good error message. I don't want to say gutenberg-mobile folder, because the folder can have a different name, so I came up with the below message in
e58d4d2, what do you think?
This script can only be used if the 'gutenberg' project is a submodule of the 'gutenberg-mobile' project
It looks like I forgot to put . at the end 🤦♂️ We'll remove this message when we move the script to gutenberg-mobile in wordpress-mobile/gutenberg-mobile#3238, so I don't want to add another commit if the message is good enough.
| cp ../../../../bundle/android/App.js ./react-native-bridge/build/assets/index.android.bundle | ||
|
|
||
| if [ $? -ne 0 ]; then | ||
| echo "Make sure to run 'npm install' & 'npm run bundle:android' from the root folder of the project before running this script." |
There was a problem hiding this comment.
However, I am not sure if that'll actually be useful since this script shouldn't really be run by developers.
+1.
We could go down the path of adding CLI options to do the necessary setup work if required, but since CI should do all that for us, I, too, think keeping this script single tasked and "honest" to its name is a better approach.
| // TODO: This is a very fragile check. Since the `settings.gradle` file is part of initialization, | ||
| // it'll not work for even simple tasks such as `./gradlew clean` unless the | ||
| // `publishReactNativeBridgeVersion` property is passed. It's a necessary check for the moment, | ||
| // however by moving the Hermes binary to remote and maybe using dependency substitution for the | ||
| // included projects, we can get rid of it. |
There was a problem hiding this comment.
@mokagio @mchowning @cameronvoell I wanted to let you know about this recent change. I was looking into whether we could run the publish script without the node_modules folders (for CI) and we technically can, but it gets very messy because of this check.
I just wanted to note that this is far from ideal, but also not important enough to work on right now, so I opted to just leave a comment here for now.
There was a problem hiding this comment.
Thanks for the message @oguzkocer.
With your recent work removing this in https://github.com/oguzkocer/gutenberg/pull/3, should we then be able to remove the node_modules requirement in CI? Although, that would be a marginal improvement only, because we'll still need to run npm install (npm ci to be precise) in order to build the JS bundle, so 🤷♂️ ?
There was a problem hiding this comment.
Unfortunately it's the opposite as node_modules is now a hard requirement. Specifically this assertion will not allow any Gradle task to be run if node_modules is missing.
…y-build-support-remote-hermes Use remote hermes dependency in react-native-bridge/android
…site-and-binary-build-support
Description
This PR adds support for publishing
react-native-bridgeandreact-native-aztecAndroid libraries to Bintray. It also makes it possible to addreact-native-bridgeas a composite build which is now howreact-native-editoris setup. This required a lot of changes which are summarized as follows:react-native-bridge,react-native-aztecandreact-native-editorprojects are all upgraded to Gradle 6.react-native-bridgenow has a module with the same name and its previous contents have been moved to this module. This change makes it much easier to work with the composite build setup and it's generally considered a good thing to have modules instead of a singular project.react-native-bridgeandreact-native-aztecprojects are aware of whether they are being used in a composite build or whether they'll be published. This allows us to customize the build.react-native-bridgenow adds all the otherreact-nativeprojects in itssettings.gradlefile in the composite build case. This allows us to expose these asapidependencies and makes it simpler to work with. This is most apparent in thereact-native-editorchanges where we were able to remove a lot ofincludestatements and separate dependencies.react-native-bridgeproject now contains apublish-aztec-and-bridge.shshell script. This script will take a version, run some checks, uploadreact-native-aztecto Bintray, use that version inreact-native-bridgeand publish that to Bintray. This will hopefully simplify the publishing process which will be handled from thegutenberg-mobilerepo.react-native-bridgeproject now has 2 modules forhermesand these are added as a dependency for the composite build case. Directly adding aars is no longer supported, and Gradle says they never worked as expected, and this seems to be the correct way to include them now. Note that in the binary dependency case, this will not work as localaardependencies are not added to the publishedaarof the library. This means we need to separately add the hermes binary to WPAndroid in the binary dependency case. We may be able to improve this in the future, but for the time being, this seems to be the best available solution.react-native-bridgeutilizes Jitpack to add thereact-nativelibraries in the binary dependency case. We looked into fat-aar solution, but it didn't work as well as we hoped, so we reverted back to this solution. Although this shouldn't cause any major inconvenience in the short term, it'd be great to look into how we can improve this. @mchowning mentioned that this requires us to maintain forks for these projects, and I'd love to learn more about this problem and see if we can help with it once the higher priority changes are taken care of.Note that we do NOT have auto-publishing for Bintray yet. We have a WIP PR in
gutenberg-mobilethat's almost there, but it needs to be updated to match the changes in this PR.There is still more I wish I could improve, but I couldn't get some of it working and for others I couldn't justify the time I'd need to spend on it. I hope to re-visit the build files in these projects soon as part of a general "improving build files" project.
How has this been tested?
react-native-bridge&react-native-azteclibraries, with and without thepublish-aztec-and-bridge.shscript. Used these binary dependencies in WPAndroid.react-native-bridgeas a composite build for WPAndroid.Here is the WPAndroid PR that can be used to test these changes: wordpress-mobile/WordPress-Android#14175
Types of changes
Build file changes for
react-native-bridge,react-native-aztec&react-native-editorAndroid projects.Checklist: