GH-485: (android) Replace deprecated "compile" with "implementation"

[jedrivisser]

Platforms affected

android

What does this PR do?

generates gradle dependencies in the build.gradle from <framework> elements found in the plugin.xml as the new implementation dependencies instead of deprecated compile dependencies

What testing has been done on this change?

ran npm run test

Checklist

• [✘] Reported an issue in the JIRA database
  Created a github Issue: Cordova generates build.gradle with deprecated "compile" dependencies #485

• [✓] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.

• [✓ ] Added automated test coverage as appropriate for this change.
  None were needed

There are some more references to compile dependencies in other files but I left them alone bacause I am not sure what they are used for, see:

https://github.com/apache/cordova-android/blob/7.1.x/bin/templates/cordova/lib/plugin-build.gradle#L41-L43
https://github.com/apache/cordova-android/blob/7.1.x/spec/fixtures/android_studio_project/app/build.gradle#L23-L25

[codecov-io]

Codecov Report

Merging #486 into 7.1.x will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##            7.1.x     #486   +/-   ##
=======================================
  Coverage   44.24%   44.24%           
=======================================
  Files          17       17           
  Lines        1695     1695           
  Branches      312      312           
=======================================
  Hits          750      750           
  Misses        945      945

Impacted Files	Coverage Δ
...in/templates/cordova/lib/builders/GradleBuilder.js	20.75% <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 750531c...48a3800. Read the comment docs.

[janpio]

There are some more references to compile dependencies in other files but I left them alone bacause I am not sure what they are used for, see:

Doesn't Android Studio complain now when one opens a project? Does it complain about these two files?

I would say they could be changed as well - but I don't know too much about Android ;)

[erisu]

@janpio

Initial Questions

• Do we have a Gradle versions requirement for Cordova projects?
• How far back do we support?

Overview

The later Android Studio should not complain.

compile had been deprecated from Gradle since 3.4, but still exists for backwards compatibility.

implementation and api was introduced to replace compile. The key differences is that implementation dependencies are identified as internal while api are external.

implementation configuration should be used to declare dependencies which are internal to the component.

Dependencies appearing in the api configurations will be transitively exposed to consumers of the library, and as such will appear on the compile classpath of consumers.

Dependencies found in the implementation configuration will, on the other hand, not be exposed to consumers, and therefore not leak into the consumers' compile classpath.

https://docs.gradle.org/current/userguide/java_library_plugin.html#sec:java_library_configurations_graph

Final Question

• Should we suppose the ability where the user can identify the framework library as implementation or api?

In most cases, implementation should be enough.

With this in mind, is there any concern for merging this PR or should we improve to support identify the type.

[janpio]

(I have no idea @erisu - best create an issue with label discussion here for your first two questions and get some eyes on it [e.g. by linking to it in Slack or dev mailing list])

[jedrivisser]

@erisu @janpio half the compile dependencies were already changes to implementation in this commit:
9fdb126
and
18d6884
otherwise I would have had the same questions

the current output is a mixed one now, for example:

[erisu]

As for the changes itself, OK with this PR change.

I suspect you will also make a separate PR for the master branch as well?

I noticed this is targeting 7.1.x .

Additionally, want to point out that the master branch does not have GradleBuilder.js and StudioBuilder.js anymore. It had been refactored and consolidated into ProjectBuilder.js

@knight9999 and @dpogue do you have and other feedback or concerns with this PR?

[janpio]

Oh, we should then probably change the target to master here. Whoever creates a patch release for 7.1.x (if at all) should cherry pick up this change over from there.

[brodycj]

I just crossed paths with @janpio, deleted my own comment to avoid the possibility of any extra confusion. I would definitely agree with @janpio to change the target to master, would favor cherry-pick only if absolutely necessary.

[erisu]

I would also agree that cherry picking is the typical and preferred way, but the files being modifying does not exist in master anymore. Also, the file that would need to be modified in master, to be equivalent to this change, is new and does not exist in 7.1.x.

In this case, I don't really think cherry picking will work... Correct me if I am wrong...
@janpio @brodybits

[janpio]

Nope, then I agree. Can you point @jedrivisser to the correct file so they can create another PR for that file?

[erisu]

Sure, the file and line number in master:

https://github.com/apache/cordova-android/blob/master/bin/templates/cordova/lib/builders/ProjectBuilder.js#L196

[jedrivisser]

So... I assume I need to create a new pull request? because I cannot change the from location of the pull request. Do I just close/reject this one?

Changes are available here:
https://github.com/jedrivisser/cordova-android/commit/f4478aeffd269f72be7399aa6ac93a562909d524

[jedrivisser]

ah, nevermind, I found a way, should be ready now