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

[MAJOR] Gradle upgrade from 4.10.3 to 6.x #792

Merged
merged 7 commits into from
Jan 21, 2020
Merged

Conversation

breautek
Copy link
Contributor

@breautek breautek commented Jul 24, 2019

Platforms affected

Android

Motivation and Context

Updates gradle version to the latest release (currently 6.1). It solves a few issue tickets that supposedly was caused after upgrading Android Studio.

Fixes #780
Fixes #754
Fixes #718
Fixes #834

Description

Changed the desired gradle version from 4.10.3 to 6.1

Testing

I have tested using npm test and all tests passed. Note that if you have ran npm test before, you will need to remove the /test/gradlew file so the test project will download the new gradle version.

I have also tested by creating a brand new cordova app, and adding the cordova-android platform by using cordova platform add https://github.com/breautek/cordova-android.git#gradle-5.x
Then running the cordova build android command.

I'd also like to make a note that gradle 5.5.1 does currently produce a warning stating:

Deprecated Gradle features were used in this build, making it incompatible with Gradle 6.0.

Running gradlew manually with --verbose --stacktrace shows the code triggering the warning is android internal code.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@breautek
Copy link
Contributor Author

This PR is intended for the android 9.x milestone.

@breautek breautek marked this pull request as ready for review July 24, 2019 00:06
@breautek breautek changed the title Updated gradle from 4.10.3 to 5.5.1 Gradle upgrade from 4.10.3 to 5.5.1 Jul 24, 2019
@erisu erisu added this to the 9.0.0 milestone Jul 24, 2019
@codecov-io
Copy link

codecov-io commented Jul 24, 2019

Codecov Report

Merging #792 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #792   +/-   ##
=======================================
  Coverage   66.17%   66.17%           
=======================================
  Files          19       19           
  Lines        1839     1839           
=======================================
  Hits         1217     1217           
  Misses        622      622
Impacted Files Coverage Δ
...lates/cordova/lib/config/GradlePropertiesParser.js 76.66% <ø> (ø) ⬆️
...n/templates/cordova/lib/builders/ProjectBuilder.js 67.42% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64ef13c...9595ba9. Read the comment docs.

@janpio
Copy link
Member

janpio commented Jul 24, 2019

Note that if you have ran npm test before, you will need to remove the /test/gradlew file so the test project will download the new gradle version.

We should open an issue for this so this can get fixed.

I'd also like to make a note that gradle 5.5.1 does currently produce a warning stating:

Deprecated Gradle features were used in this build, making it incompatible with Gradle 6.0.

Running gradlew manually with --verbose --stacktrace shows the code triggering the warning is android internal code.

So nothing we can do about?

@breautek
Copy link
Contributor Author

breautek commented Jul 24, 2019

So nothing we can do about?

Not really. There is an open bug ticket on Android Issue Tracker.

We should open an issue for this so this can get fixed.

I could just add an npm script to do this for you, so that we can just use something like npm run-script clean.

@erisu
Copy link
Member

erisu commented Jul 25, 2019

I think git clean -fdx should be enough as well. This is typically what I use to clean out any left over files and folder that are untracked.

@breautek
Copy link
Contributor Author

breautek commented Jul 30, 2019

I think git clean -fdx should be enough as well. This is typically what I use to clean out any left over files and folder that are untracked.

Actually ran into an issue on "cleaning" gradlew as I was writing the script I remembered that not everybody is going to be running in a bash environment...

I think windows have different commands for using rm and I don't think they support the && syntax in their command line to run another command immediately after the first command... (could be wrong, been forever since I used windows...)

using git clean -fdx will also remove node_modules, which will require using npm install again, so if I used this, I'd still have to run a second command and run into the issue described above.

So I think the best way to handle this is to use a task system such as gulp or grunt so the clean code can be ran in Node, allowing more cross-platform flexibility. Currently the repository doesn't have either and I'm not sure if such systems are okay in Cordova, or if it is, if one is preferred over the other.

@erisu
Copy link
Member

erisu commented Jul 30, 2019

I don't think they support the && syntax in their command line to run another command immediately after the first command

&& is supported and runs only if the first command was successful. Last I tested.

using git clean -fdx will also remove node_modules, which will require using npm install again, so if I used this, I'd still have to run a second command and run into the issue described above.

git clean -fdx -e node_modules

cross-platform flexibility

The command above should work on all platforms: linux, macOS (tested), and Windows (tested).

@breautek
Copy link
Contributor Author

breautek commented Aug 1, 2019

I've added a clean script that does git clean -fdx -e node_modules. I run linux and it works excellent for this case as well. Thanks for the suggestion!

So anybody with existing repositories can now simply run npm run clean and it will remove gradle and other build artefacts. I don't call this automatically as I think that might be rather annoying having gradle be redownloaded/redeployed everytime running npm test.

@Kevin-Hamilton
Copy link

This assumes 1) that the project is currently being managed under git, and 2) that the developer does not have any untracked or uncommitted work anywhere in the current git repository (which may be tracking files and folders beyond the scope of the package.json that this command is in).

I think this is a bad idea. Unless I am misunderstanding the target user for the npm run clean command?

@breautek
Copy link
Contributor Author

breautek commented Aug 8, 2019

The clean command is something that a developer would use when working in the cordova-android repo. It's not intended to be used as a part of a broad script.

However, there is some annoying dangers. I've incorporated using git clean in my workflow in another project and accidentally deleted newly added but untracked files.

So maybe just writing a gulp script might be better, to avoid accidents like that.

@breautek breautek changed the title Gradle upgrade from 4.10.3 to 5.5.1 Gradle upgrade from 4.10.3 to 5.x Sep 8, 2019
@breautek
Copy link
Contributor Author

breautek commented Sep 9, 2019

@erisu FYI, I opted away from the git clean method as you suggested. I originally liked the idea and even used it my own personal project. However, I had a situation where I accidentally deleted untracked files that was intended to added to the repo because git clean was ran as a part of a script.

Due to this danger I decided to replace it with a gulpfile implementation. I see that gulp is already used in the cordova-docs library, so I assume it wouldn't be a big deal adding a dev dependency here. The gulpfile contains a clean task to clean files that we need to specifically clean, which is currently ./test/gradlew && ./test/gradlew.bat.

package.json Outdated Show resolved Hide resolved
@brodycj brodycj changed the title Gradle upgrade from 4.10.3 to 5.x [MAJOR] Gradle upgrade from 4.10.3 to 5.x Sep 23, 2019
Copy link
Member

@timbru31 timbru31 left a comment

Choose a reason for hiding this comment

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

LGTM! I'd like another maintainer to have a look, too, though. :)

@breautek
Copy link
Contributor Author

LGTM! I'd like another maintainer to have a look, too, though. :)

This PR is intended for 9.x, perhaps a branch should be created on the apache repo for 9.x? Currently this PR is actually targeting apache's master.

@brodycj
Copy link
Contributor

brodycj commented Sep 24, 2019

LGTM for 9.0. I did verify from [1] that this proposal is referencing the most recent Gradle version to date.

Here are my personal feelings:

  • I would downvote making a new branch for 9.x due to the potential for confusion and extra merging that may be needed as a consequence.
  • I would favor committing this change now and maybe using cordova-coho to set the version to 9.0.0-dev. Then 8.x patch releases could still be made from the existing 8.1.x branch, if needed.

I wonder if we should discuss in [email protected] before merging?

[1] https://services.gradle.org/distributions/

Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@breautek
Copy link
Contributor Author

I would favor committing this change now and maybe using cordova-coho to set the version to 9.0.0-dev. Then 8.x patch releases could still be made from the existing 8.1.x branch, if needed.

This works for me.

@erisu
Copy link
Member

erisu commented Sep 24, 2019

I do not recommend merging this PR into master until we are ready to prepare the next major release. There is no official release date and from various discussions is looking to be more around mid-January to early February. It is better to keep master available for potential future minor or patch releases until then.

@breautek
Copy link
Contributor Author

Note to myself: #834 (comment) may be affected by this PR. I should double check this.

@piotr-cz
Copy link

Gradle 6.0 is out.
I wonder if there is a B/C coming anyway maybe it's worth to skip to latest Gradle version?

@breautek
Copy link
Contributor Author

Gradle 6.0 is out.
I wonder if there is a B/C coming anyway maybe it's worth to skip to latest Gradle version?

Definitely worth looking into.

@breautek breautek changed the title [MAJOR] Gradle upgrade from 4.10.3 to 5.x [MAJOR] Gradle upgrade from 4.10.3 to 6.x Nov 15, 2019
bin/templates/project/app/build.gradle Outdated Show resolved Hide resolved
bin/templates/project/build.gradle Outdated Show resolved Hide resolved
test/build.gradle Outdated Show resolved Hide resolved
Copy link
Member

@erisu erisu left a comment

Choose a reason for hiding this comment

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

I performed the following test.

cordova create testApp
cordova platform add github:breautek/cordova-android\#gradle-5.x
cordova build android
cordova run android

I also opened the project in Android Studio and the Android project synced successfully with out any errors or warnings. As a side note, it is always recommended to build the application once with the cordova run build command before opening the projects platforms/android directory in Android Studio so it can checkout Gradle wrapper file.

In Android Studio, I confirmed that I am able to build (Make project) and run the app.

@erisu erisu merged commit 8ef742e into apache:master Jan 21, 2020
@wilywork
Copy link

wilywork commented Jan 21, 2020

@erisu command "cordova platform add github:breautek/cordova-android\#gradle-5.x" not work for me..

Any solution ?

C:\android_build>cordova platform add github:breautek/cordova-android\#gradle-5.x
Using cordova-fetch for github:breautek/cordova-android\#gradle-5.x
Failed to fetch platform github:breautek/cordova-android\#gradle-5.x
Probably this is either a connection problem, or platform spec is incorrect.
Check your connection and platform name/version/URL.
Error: npm: Command failed with exit code 1 Error output:
npm ERR! Error while executing:
npm ERR! C:\Program Files\Git\cmd\git.EXE ls-remote -h -t ssh://[email protected]/breautek/cordova-android%5C.git
npm ERR!
npm ERR! Host key verification failed.
npm ERR! fatal: Could not read from remote repository.
npm ERR!
npm ERR! Please make sure you have the correct access rights
npm ERR! and the repository exists.
npm ERR!
npm ERR! exited with error code: 128

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\xandy\AppData\Roaming\npm-cache\_logs\2020-01-21T12_36_03_938Z-debug.log```

@RafaelKr
Copy link

RafaelKr commented Jan 21, 2020

@wilywork Try cordova platform add https://github.com/breautek/cordova-android\#gradle-5.x instead.

@erisu
Copy link
Member

erisu commented Jan 21, 2020

There might a difference between the Windows and Linux/macOS syntax when passing in the GitHub URL.

For example, I escape the # which is needed. But for Windows maybe that is not required and causes your problem.

Additionally, since this PR had been merged, you can use the Apache repo instead. I would not recommend using his branch as he could delete the branch at any time.

Try what @RafaelKr suggested.

Lastly, please note that we do not consider or suggest using the master branch in production. Master is still an active development branch and would not go through an official release vote.

Testing and reporting is always welcomed.

@breautek
Copy link
Contributor Author

You can only use ssh urls if you have write access to the repo, which in this case he does not.

Additionally, because this PR is merged in, I'll be deleting my gradle-5.x branch, so using the Apache repo will now be required.

Thanks @erisu for picking this up.

@breautek breautek deleted the gradle-5.x branch January 21, 2020 13:03
@RafaelKr
Copy link

RafaelKr commented Jan 21, 2020

@breautek No, I also thought the problem comes from using SSH. But @erisu mentioned the escaped # and that's right.

His log says Files\Git\cmd\git.EXE ls-remote -h -t ssh://[email protected]/breautek/cordova-android%5C.git and %5C is the url encoded backslash.
On my Linux system it's working with SSH.

@wilywork try this: cordova platform add github:apache/cordova-android#8ef742e
breautek just deleted his branch, so all the other mentioned methods won't work anymore.

Edit: @erisu pointed out using the master branch is not suggested. To get exactly the current state I included the specific commit id (8ef742e).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants