breautek commented 4 years ago

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

[x] I've run the tests to see all new and existing tests pass
[x] I added automated test coverage as appropriate for this change
[x] Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
[x] 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)
[x] I've updated the documentation if necessary

breautek commented 4 years ago

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

codecov-io commented 4 years ago

Codecov Report

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

@@           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% <ø> (ø)`	:arrow_up:
...n/templates/cordova/lib/builders/ProjectBuilder.js	`67.42% <0%> (ø)`	:arrow_up:

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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.

breautek commented 4 years ago

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.

brodybits commented 4 years ago

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 dev@cordova.apache.org before merging?

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

breautek commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Note to myself: https://github.com/apache/cordova-android/issues/834#issuecomment-553973463 may be affected by this PR. I should double check this.

piotr-cz commented 4 years ago

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 commented 4 years ago

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.

wilywork commented 4 years ago

@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://git@github.com/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 commented 4 years ago

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

erisu commented 4 years ago

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 commented 4 years ago

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.

RafaelKr commented 4 years ago

@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://git@github.com/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).

apache / cordova-android

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

Platforms affected

Motivation and Context

Description

Testing

Checklist

Codecov Report