-
Notifications
You must be signed in to change notification settings - Fork 58
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
Publish React Native Bridge to Bintray from CI #3230
Conversation
This is a key step in allowing the Android app to build without the overhead of the `gutenberg-mobile` submodule. Currently, there is a manual step to unlock the process if the Android tests passed. That's because of how long it takes for the build step to run: ~15 minutes. Eventually, it would be great to have some intelligence in the PR to be able to determine whether it should start a build for the Android bridge.
Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job! |
.circleci/config.yml
Outdated
# The build process for the react-navite-bridge requires the App.js | ||
# bundle and also access to the node_modules folder. |
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.
Should I a link to the line of code that requires node_modules
diff on GitHub once the PR for that is merged?
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 build process for the react-navite-bridge requires the App.js | |
# bundle and also access to the node_modules folder. | |
# The build process for the react-native-bridge requires the App.js and the node_modules folder. |
We technically don't need the node_modules
once the bundle is created. However, without it configuration gets a bit messy. I don't think it's worth adding a link here and I am hoping we can work towards not needing the node_modules
folder.
I'm not sure where all those |
.circleci/config.yml
Outdated
steps: | ||
- checkout | ||
- checkout-submodules | ||
- npm-install |
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 name is misleading :-) This actually runs npm ci
as expected 👍
.gitmodules
Outdated
@@ -1,6 +1,6 @@ | |||
[submodule "gutenberg"] | |||
path = gutenberg | |||
url = ../../WordPress/gutenberg.git | |||
url = [email protected]:oguzkocer/gutenberg.git |
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'm pretty sure this is intentional to test this PR, but I'm leaving this comment just to make sure this is not something which fell between the cracks :-)
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.
Correct & great idea!
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.
@mchowning @cameronvoell @ceyhun @loremattei here's an updated version of the workflow that incorporates the versioning workflow discussed internally and most of your suggestions.
Can I ask you to have another look?
I updated the test PR on WordPress Android to use the binary from the latest CI build, if you want to verify it builds and works.
# This is a partial duplication of the logic to compute the version | ||
# to use for the AAR from below. It's useful to have it here | ||
# because there are conditions under which we shouldn't run the job | ||
# that can't be captured in a CircleCI filter. | ||
# | ||
# Exiting here saves a non-trivial amount of CI time because of how | ||
# heavy the repo is to checkout (it takes at least 1 minute) and | ||
# how time consuming running `npm ci` is. |
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.
As per comment, I tried having this in a dedicated script, but the repo checkout takes at least 1 minute.
On the other hand, the builds that would actually exit from here are a small % of the total builds running on this repo 🤔 Maybe by taking that into consideration the cost in CI has less weight than the duplication cost here?
Keen to hear your thoughts. It's a straightforward change to make, but it might be best to make it in a followup PR, just so we can get this moving along.
- run: | ||
# Setting up Android before fetching the Node dependencies because | ||
# this step is faster, so if it fails we'll learn about it sooner and | ||
# avoid wasting cycles. | ||
name: Setup Android Tooling | ||
command: .circleci/setup-android-on-ubuntu.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.
@ceyhun suggested looking into spinning up a Docker container to avoid this manual setup.
I definitely want to look into it but opted for this approach which I already know works for this first version (which has already been dragging along for a while 😞 )
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.
Thanks @mokagio 🙇 This already looks awesome and we can surely improve it later once we have it in place and working first 👍
# isn't a PR open for it yet. | ||
echo "Running on a feature branch with no open PR." | ||
echo "The checks previous to this step should not have let this happen." | ||
exit 1 |
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 decided to fail the build here because if this happen there's something wrong in the setup and we should know about it sooner rather than later.
filters: | ||
branches: | ||
# Don't run on trunk, otherwise, run on every other branch | ||
ignore: trunk |
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 reason I set this to not run on trunk
is that from my understanding of our workflows discussions, we never need to access a build from that branch.
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.
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.
Thanks @mchowning!
Every time a tag is pushed, this workflow will run and upload an AAR with the version being the tag name. See this build for an example: it started from GitHub receiving the tag mokagio-test-20210317-173215
and uploaded a binary to Bintray with version mokagio-test-20210317-173215
.
Do you ever need to reference in WordPress Android a commit from gutenbreg-mobile's trunk
that has not a matching tag? If that's the case, I can easily make builds on trunk
behave like builds on develop
: publish a trunk-< commit SHA1 >
version to Bintray.
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.
Do you ever need to reference in WordPress Android a commit from gutenbreg-mobile's trunk that has not a matching tag?
I don't think so. As long as all the tags get pushed we should be fine. 👍
# We end up here on the first push of a new branch, when there | ||
# isn't a PR open for it yet. | ||
echo "Build initiated from a feature branch with no pull request: aborting." | ||
circleci-agent step halt |
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 build is an example of this scenario
Unfortunately, CircleCI doesn't treat halts differently and the UI might be misleading as it looks like the build passed.
tags: | ||
# Run for every tag | ||
only: /.*/ |
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.
Here's an example of build from a tag: mokagio-test-20210317-173215
.
…-bintray-with-script-merge-test-both-on-latest
@cameronvoell @mchowning @oguzkocer the tests still fail, even after syncing with This is the failure on both steps:
diff of the error output between the two steps to show it's the same4,5c4,5
< Could not determine the dependencies of task ':app:testDebugUnitTest'.
< > Could not resolve all task dependencies for configuration ':app:debugUnitTestRuntimeClasspath'.
---
> Could not determine the dependencies of task ':app:processDebugResources'.
> > Could not resolve all task dependencies for configuration ':app:debugRuntimeClasspath'. 🤔 I bet the issue has to do with this project not having the S3 repository where our mirror lives. Let's see if I can do something about it... |
All green in CI 🎉 Thanks @oguzkocer for the help. I'm going to turn this "Ready for review" even though the "Before merging this PR, we need to merge WordPress/gutenberg#29488 into Gutenberg's @mchowning @cameronvoell let me know if you think we should merge before or after the Gutenberg counterpart lands into Gutenberg's |
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.
trunk
and update the submodule has here.This PR adds CI jobs to build and publish the AAR for the
react-native-bridge
package (which lives in Gutenberg).I originally planned to open this against the
gutenberg-composite-build
project WIP branch, but given this doesn't change anything in the codebase itself other than the Gutenberg submodule hash, I thinkdevelop
is appropriate.To test:
localGutenbergMobile
path in yourlocal-builds.gradle
is commented out, otherwise it'll kick off a composite build, which won't use the binary.PR submission checklist: