Skip to content

Conversation

@oguzkocer
Copy link
Contributor

@oguzkocer oguzkocer commented Jul 27, 2021

This PR upgrades Gradle to 7.1.1 and Android Gradle Plugin to 4.2.2. The changes in the PR are minimal as all the necessary changes have been made in other PRs and already got merged. Besides the upgrades, the PR references the Gradle 7 branch of FluxC, gutenberg-mobile & stories-android libraries so that it can all be tested in this PR.

Note that I am leaving this PR as a draft because all Gradle 7 upgrade PRs need to be merged together. We'll merge all 3 libraries first and update the references before merging this PR.

To test:

  • Smoke test the app - login & editor should be enough
  • Use composite build for fluxc & gutenberg-mobile libraries (following the instructions in local-builds.gradle-example file) and smoke test the app again. Make sure you're on the Gradle 7 branches for fluxc & gutenberg-mobile. Don't forget to run npm install & npm run start:reset from gutenberg-mobile folder to start the metro server. You'll also need to do nvm use from gutenberg-mobile/gutenberg folder before running npm install or you might have errors.

Regression Notes

  1. Potential unintended areas of impact
    Probably N/A - The build file changes should only impact whether the project builds or not. However, since Android Gradle Plugin is updated, it's hard to be sure that nothing else will be impacted.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manual tests and current CI checks mostly around the build

  3. What automated tests I added (or what prevented me from doing so)
    N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • 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.

@oguzkocer oguzkocer added this to the 18.0 milestone Jul 27, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jul 27, 2021

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jul 27, 2021

You can test the changes on this Pull Request by downloading the APKs:

@oguzkocer oguzkocer force-pushed the upgrade-gradle-to-7.1.1 branch 2 times, most recently from 1f5818f to 5adccea Compare August 5, 2021 06:51
@AliSoftware AliSoftware modified the milestones: 18.0, 18.1 Aug 9, 2021
Copy link
Contributor

@jkmassel jkmassel left a comment

Choose a reason for hiding this comment

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

I smoke tested this using FluxC as a composite build (since it has an associated PR to update Gradle) and it worked great, and also tested the PR in isolation.

The editor and login both worked great, so I think this is good to go.

@oguzkocer oguzkocer marked this pull request as ready for review September 1, 2021 23:10
@oguzkocer oguzkocer enabled auto-merge September 1, 2021 23:16
@oguzkocer oguzkocer merged commit 795c4c7 into develop Sep 1, 2021
@oguzkocer oguzkocer deleted the upgrade-gradle-to-7.1.1 branch September 1, 2021 23:34
@AliSoftware
Copy link
Contributor

At first builds were working but trying to run on my phone kept failing with "Unable to determine application id" error, whatever variant I tried. I then tried "Build > Clean Project" then "Build > Rebuild Project" but still got the same issue. Then tried "File > Invalidate Caches / Restart", rebooted AS, tried yet another Gradle Sync, then rebuilt the project and it finally launched. So I figured I'd leave that info here in case anyone has trouble building after the transition and the PR gets merged.

I then decided to smoke-test Jetpack – given Jeremy already tested WP and that JP is often overlooked and forgotten when testing things 😛 – and it worked.


I was unable to test composite builds with gutenberg, because my npm install fails with the failure below, while I have npm 7.4.3 installed and there's a package-lock.json file, so not sure what is happening.

npm ERR! The `npm ci` command can only install with an existing package-lock.json or
npm ERR! npm-shrinkwrap.json with lockfileVersion >= 1. Run an install with npm@5 or
npm ERR! later to generate a package-lock.json file, then try again.

I tried following the instructions in gutenberg-mobile, installed nvm too (but that failed with other errors 😒), etc. I also tried cd jetpack/projects/plugins/jetpack && npm install since that error on npm ci and the package-lock.json file were trying to be run in that submodule, and got even more errors.
After trying some other stuff with SO & all to solve it, I bailed to avoid spending my whole afternoon on it. Maybe I'll look at making local gutenberg-mobile work on my machine one day, but not today

@oguzkocer
Copy link
Contributor Author

@AliSoftware It looks like you might have mixed things up a bit, so at this point you may need to clean things up before it works, but here are the instructions if you were to start from scratch: (since all PRs are merged at this point, please run them in the develop branch for WPAndroid and gb-mobile)

In WordPress-Android:

  1. cp local-builds.gradle-example local-builds.gradle
  2. In local-builds.gradle un-comment the localGutenbergMobilePath property and make sure it's relative path is correct

In gb-mobile:

  1. pushd gutenberg/ && nvm install && nvm use && popd
  2. npm install - make sure this succeeds and don't run the npm commands in any other directory, it should always be run from the gb-mobile folder.
  3. npm run start:reset - this will start the metro server which is necessary for the editor to load

Then you can run the app normally. You don't need to test this anymore but I think it's important for you to be familiar with how to setup gb-mobile. Please let me know if you have any issues.

@AliSoftware
Copy link
Contributor

Yep that's exactly the instructions I followed, and:

  • while the composite build for FluxC worked,
  • for G I got stuck at cd gutenberg && nvm install because I couldn't even install nvm itself. I followed the instructions for it from the gutenberg-mobile README, but got some errors during install of nvm at that point that I failed to find a solution for.
  • After failing to go anywhere with nvm, I decided to give a try at cheating and jumping to the next step despite failing that first one, aka try running npm install at the root of the repo, and got the errors I mentioned.

Those npm install errors might very well be because I bailed on the first step about nvm after a while; so hopefully if I manage to solve the errors I kept having in the first step, and install nvm properly and run it without error one day, the next steps will start working too.🤞

@oguzkocer
Copy link
Contributor Author

Ahh.. I see. Let me know if you want to look into the nvm installation errors on Slack together. You can use our Docker image if you prefer not to deal with it at all, but you'd need to map out the metro server port for it, so you might want to add a docker-compose configuration on top of it. It might be worth adding the docker-compose configuration in gb-mobile so it can be easily run with just a docker compose up command. I'll try to squeeze that task in somewhere 😅

@mokagio
Copy link
Contributor

mokagio commented Sep 2, 2021

Yeah!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants