Skip to content

Conversation

@mchowning
Copy link
Contributor

@mchowning mchowning commented May 14, 2020

Trying to improve the avoid-jitpack workflow, particularly the build speeds.

See WPAndroid PR for testing instructions

PR submission checklist:

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

@mchowning mchowning added this to the 1.28 milestone May 14, 2020
@mchowning mchowning requested a review from hypest May 14, 2020 16:49
@mchowning mchowning self-assigned this May 14, 2020
@mchowning mchowning requested a review from cameronvoell May 14, 2020 16:49
@mchowning mchowning marked this pull request as ready for review May 14, 2020 21:26
@mchowning mchowning marked this pull request as draft May 14, 2020 22:07
@mchowning
Copy link
Contributor Author

mchowning commented May 14, 2020

The remaining work on this is to figure out a way to include all of the relevant js, css, scss files (any others?) in the task's input so that if any of them change the bundle is regenerated. This worries me because it seems very fragile and when it fails it will not be obvious that the reuse of a previously generated bundle is the problem. 🤔

This is making me think we may be better off publishing the artifacts ourself for devs who are not building Gutenberg from source (i.e., basically create our own Jitpack).

@mchowning
Copy link
Contributor Author

mchowning commented May 15, 2020

The remaining work on this is to figure out a way to include all of the relevant js, css, scss files (any others?) in the task's input so that if any of them change the bundle is regenerated. This worries me because it seems very fragile...

Pushed an update that I think addresses that. (6d63c07) Its a bit hacky though, and I'm still concerned about this being fragile. Remaining things I think should be addressed:

  • Running the clean gradle task from WPAndroid should regenerate the bundle. This seems really important to me because it makes it relatively trivial to recover if a bundle gets out of date for some reason. Currently a clean does not cause the bundle to regenerate. Addressing this seems critical to me because it makes it significantly easier to recover from unexpected build problems.
  • Running a WPAndroid build should not make the gutenberg-mobile submodule "dirty". Currently running a WPAndroid build generates a js bundle modifies ~30 files in the libs/gutenberg-mobile/bundle/ directory. This seems more like a nice-to-have.

@mchowning
Copy link
Contributor Author

In trying to make clean rerun the task I've realized that for some reason my task is not actually checking the output the way I thought it was. I can delete the bundle and the bundleUpToDateCheck task still shows as UP-TO-DATE and builds an app that crashes when the editor is loaded. 😬

@mchowning
Copy link
Contributor Author

mchowning commented May 15, 2020

Just added fc9fbdb, which moves the bundle to the build folder and also has the bundleUpToDateCheck task add an indicator file next to the bundle. I registered that indicator file as an output of the bundleUpToDateCheck task, and it seems to detect that it needs to rerun when that is deleted (still don't understand why it doesn't do the same with the actual bundle file). This means that if you run the clean task, the bundle and the indicator file will both be deleted (because clean deletes the entire build/ folder), and the next build will properly detect that it needs to regenerate the bundle.

@mchowning mchowning changed the base branch from develop to release/1.28.0 May 15, 2020 20:47
Copy link
Contributor

@cameronvoell cameronvoell left a comment

Choose a reason for hiding this comment

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

mchowning and others added 4 commits May 15, 2020 16:53
Now running `clean` will delete the current js bundle and the next build
will know to generate a new one.
In the effort to fix the "Couldn't follow symbolic link." issue reported
here:
wordpress-mobile/WordPress-Android#11921 (comment)
@mchowning mchowning force-pushed the avoid-jitpack-updates branch from 883d9b9 to 7556716 Compare May 15, 2020 21:17
@mchowning mchowning marked this pull request as ready for review May 15, 2020 22:03
@cameronvoell
Copy link
Contributor

Tested via wordpress-mobile/WordPress-Android#11921. Looks good! :shipit:

Did another round of checks after retargeting and rebase with release branch. Looks good!

@mchowning mchowning merged commit 6281b1d into release/1.28.0 May 15, 2020
@mchowning mchowning deleted the avoid-jitpack-updates branch May 15, 2020 22:14
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