Skip to content
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

Build gutenberg-mobile locally without JitPack #11833

Merged
merged 30 commits into from
May 8, 2020

Conversation

hypest
Copy link
Contributor

@hypest hypest commented May 6, 2020

We currently experience high failure rate on JitPack building the gutenberg-mobile artifact for WPAndroid, so this PR uses the "JITPACK" mode in gutenberg-mobile to launch Node/Yarn to build the block editor's JS bundle and embed it in the gutenberg-mobile bridge artifact.

No need to install Node or Yarn, that's done automatically but the android-maven-gradle-plugin Gradle plugin in a temporary folder.

Related gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#2231

Changes in this PR

  1. Remove dependency to the JitPack-built gutenberg-mobile library artifact

  2. Check out the full (albeit shallow) gutenberg-mobile git submodule, including its sub-submodules. Needed for building the gutenberg-mobile native and JS code locally.

  3. Have a dedicated CircleCI job (gutenberg-bundle-build) to build the JS bundle of gutenberg-mobile. Other build jobs depend on this job to finish first. The JS bundle is stored in a CircleCI workspace to be picked up by the other jobs.

  4. Locally, the JS bundle task is called always. It's suppressed on CI by using an env flag.

  5. The JS bundle job is using a nested Node+Yarn toolchain, managed via gradle. No need to manually install the tools, and they are not installed globally either.

  6. Added quite a few of lint checks to the "ignore list" (lint-baseline.xml). Those violations should be wrangled separately, inside the gutenberg-mobile repo.

  7. Added the gutenberg-mobile regex to the checkstyle ignore list for the same reason.

8. Bumped the gradle daemon JVM mem to 3g and disabled the parallel option too, to help the jobs succeed on CI, although that was an experiment for when the JS bundling was performed as part of each gradle build. Probably not needed now that the JS bundle has its own job, and will have to revert the change.

Known issues

  1. The yarn_install gradle task is running on every build, even though the gutenberg-mobile artifact doesn't need to be built again. This should be fixed with a better tasks dependency configuration.
  2. There are build artifacts left behind in the libs/gutenberg-mobile/bundle/android/raw/ folder, that are not clear yet what to do with (git ignore them or something else). Also, the react-native-gutenberg-bridge/android/src/main/assets/ folder is also not handled in VSC yet.

To test A:

Run the app and launch the block editor, it should work normally.

To test B:

Check out the "Builds" tab in https://jitpack.io/#wordpress-mobile/gutenberg-mobile and ensure that there's no build for d74a56 listed, the gutenberg-mobile hash currently pointed to.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 6, 2020

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 6, 2020

You can test the changes on this Pull Request by downloading the APK here.

@mchowning
Copy link
Contributor

mchowning commented May 7, 2020

Thanks for making so much progress on this @hypest ! 🙇

I have a few concerns about the developer experience here, but I definitely haven't thought through this as much as you, so I'm probably misunderstanding some things. Let me ask a few questions to clear things up.

  1. It looks like it won't be possible to build WPAndroid now without JS set up (yarn, node, etc.). Is that right?
  2. Will all debug builds now require the metro server running on a connected computer in order to use the block editor?
  3. You mention that "The yarn_install gradle task is running on every build, even though the gutenberg-mobile artifact doesn't need to be built again." It looks to me like it is only running if you are building the js bundle (i.e., buildGutenbergMobileJSBundle is true). Am I missing something? (relevant code here and here).
  4. Related to 3, will yarn install now need to be run (either when building WPAndroid or just doing it when switching branches would probably be sufficient) in order to develop WPAndroid? It seems to me this results in a switching branches workflow that looks like (1) switch branch; (2) update all the submodules recursively; and (3) run yarn install for gutenberg-mobile (and possibly, depending on the kind of change, you may need to start or start:reset the metro server as well).
  5. The interaction between these changes and the old buildGutenbergFromSource flag, which we're still using in various places, seems confusing. Is the intention to keep both flags?

@hypest
Copy link
Contributor Author

hypest commented May 7, 2020

Thanks for having a look @mchowning !

won't be possible to build WPAndroid now without JS set up (yarn, node, etc.). Is that right?

Yes and no I guess. The tools (Node, Yarn) will be maintained by the gradle plugin and only used locally so, the developer won't need to set up anything. I think that this way if they even happen to have the tools already installed, their setup won't be used nor disrupted. In other words, gradle will employ fresh instances of the tools behind the scenes and only for building the JS bundle.

Will all debug builds now require the metro server running on a connected computer in order to use the block editor?

Nope, the debug build will still use the built gutenberg-mobile bridge library artifact as before. Unless one uses the BUILD_GUTENBERG_FROM_SOURCE flag ofc, although no work has been added to this PR yet to integrate with the "source mode" too.

It looks to me like it is only running if you are building the js bundle (i.e., buildGutenbergMobileJSBundle is true). Am I missing something? (relevant code here and here).

Right, so, the PR is not fully up-to-date with that actually. The flag is false for now while I'm focusing on getting CircleCI to get green, but the idea is to always be true on local builds. I think if you try to set it to true yarn_install runs all the times. Needs more work to make it a good incremental build citizen.

Related to 3, will yarn install now need to be run (either when building WPAndroid or just doing it when switching branches would probably be sufficient) in order to develop WPAndroid?

That'd be ideally up to gradle to figure out. Ideally, if the JS sources are changed for any reason then yes, yarn_install needs to run again, but only if gradle doesn't have a cache of the final bridge artifact needed anyway.

The interaction between these changes and the old buildGutenbergFromSource flag, which we're still using in various places, seems confusing. Is the intention to keep both flags?

Not quite sure yet, but probably yes, the whole setup should be integrated to only if someone needs to have live coding and debugging (via Metro) would need to enable the "source mode" flag. We probably will need to get to that point first before it becomes more clear what's practical or not.

@hypest
Copy link
Contributor Author

hypest commented May 7, 2020

FYI, a R8: Program type already present: com.facebook.hermes.BuildConfig error was encountered while trying to build VanillaRelease locally, and 904f17c should fix it.

hypest added 4 commits May 8, 2020 01:44
Internally, passing down a "jitpack" flag to emulate the nested
gutenberg-mobile build, which behind the scenes uses Node and Yarn to
install the needed gutenberg-mobile RN dependencies to build.
@hypest hypest requested review from oguzkocer and loremattei May 7, 2020 22:53
@hypest hypest force-pushed the gutenberg/embedded-build-no-jitpack branch from a88965f to 4164366 Compare May 7, 2020 23:10
@hypest hypest changed the base branch from develop to release/14.8 May 7, 2020 23:13
@hypest hypest modified the milestones: 14.9, 14.8 ❄️ May 7, 2020
The gb-mobile JS bundle has already been built and shared as via a
CircleCI attached workspace so, no need for the nested submodules that
only contribute JS code anyway.
@hypest hypest force-pushed the gutenberg/embedded-build-no-jitpack branch from 4164366 to c05efbc Compare May 8, 2020 00:30
@hypest
Copy link
Contributor Author

hypest commented May 8, 2020

Will open this PR up for review at this stage.

Tested over at #11865 (comment), I have rewritten some of the git history to remove touching of files that ended up diff-less (gradle.properties-example), and added some more refinements (mentioned in the description).

@hypest hypest marked this pull request as ready for review May 8, 2020 00:33
Copy link
Contributor

@loremattei loremattei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a short term solution to unblock the release, this looks good to me.
There are some issues that make me think it's not a good long term solution: added complexity, longer build times on CI and on local machines (especially for releases), fragility of the setup, need to replicate (and then maintain) the setup on every client that uses the library.

But we can iterate on this at a later stage.

@hypest hypest self-assigned this May 8, 2020
Copy link
Contributor

@oguzkocer oguzkocer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've put this through several tests and all the important ones passed. I had some minor issue, most of which was already known. I've reported non-blocking issues on Slack and although I prefer to have all details in PR, I am specifically not mentioning them here due to the nature of this PR.

I think this is good enough to merge right now and I am sure we'll have many conversations about what the permanent solution will be, so no reason to bog down in little details right now.

:shipit:

@hypest
Copy link
Contributor Author

hypest commented May 8, 2020

Thanks for the approvals and the comments @loremattei and @oguzkocer . There are quite a few rough edges in this solution that we wouldn't merge just yet before polishing more. The Jitpack issue is forcing our hand here and since we need to unblock teams work. This is not the end of the line yet and we shall polish+assess 👍.

@hypest hypest merged commit 74851ea into release/14.8 May 8, 2020
@hypest hypest deleted the gutenberg/embedded-build-no-jitpack branch May 8, 2020 15:30
mchowning pushed a commit that referenced this pull request May 11, 2020
…ild-no-jitpack

Build gutenberg-mobile locally without JitPack
SergioEstevao pushed a commit that referenced this pull request May 12, 2020
…ild-no-jitpack

Build gutenberg-mobile locally without JitPack
# Conflicts:
#	.circleci/config.yml
mchowning pushed a commit that referenced this pull request May 12, 2020
…ild-no-jitpack

Build gutenberg-mobile locally without JitPack
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants