Skip to content
This repository has been archived by the owner on Feb 11, 2022. It is now read-only.

Issues/177: Remove usage of internal Gradle API #228

Closed
wants to merge 10 commits into from
Closed

Conversation

mr-archano
Copy link
Contributor

@mr-archano mr-archano commented Oct 4, 2018

Instead of creating a SoftwareComponent implementation for the Android library publication we have decided to follow @bmuschko advice and generate the missing nodes in the POM file manually. The approach should be functionally similar to the previous implementation, where we consider all the ModuleDependency from the relevant configurations: api and implementation (and compile for backward-compatibility reasons).
We have also added the generation of the exclusions node for those dependencies that have specified some.

This should fix #177 and #179

NOTE: this is the minimum amount of work needed to make the plugin work with Gradle 4.5 and above. There's few more refactoring and improvements we are planning to tackle, but the aim here is to publish a new version of the plugin with a fix asap.

@mr-archano mr-archano reopened this Oct 4, 2018
@mr-archano
Copy link
Contributor Author

mr-archano commented Oct 4, 2018

I am not sure what's going on, we should have at least another test failing (TestGeneratePomTask. testGeneratePomTaskForAndroidLibrary), but the CI instead looks happy 🤔

Basically with this new solution at the moment we are not generating the scope node in the pom file for Android libraries, so that test should fail (it does for me locally). We need to find a way to include that functionality too. The problem unfortunately is a bit more complicated than anticipated, as there is some logic needed to order dependencies per scope and at the same time avoid duplications. This logic for instance is implemented in the MavenPublishPlugin via DefaultPomDependenciesConverter, an internal API that we probably don't want to be tied to.

Any suggestions @devisnik?

@mr-archano
Copy link
Contributor Author

@tasomaniac any input would be appreciated ;)

@devisnik
Copy link

devisnik commented Oct 5, 2018

@mr-archano The tests are passing for me locally. 🤔

I am not sure what's going on, we should have at least another test failing (TestGeneratePomTask. testGeneratePomTaskForAndroidLibrary), but the CI instead looks happy 🤔

Basically with this new solution at the moment we are not generating the scope node in the pom file for Android libraries, so that test should fail (it does for me locally). We need to find a way to include that functionality too. The problem unfortunately is a bit more complicated than anticipated, as there is some logic needed to order dependencies per scope and at the same time avoid duplications. This logic for instance is implemented in the MavenPublishPlugin via DefaultPomDependenciesConverter, an internal API that we probably don't want to be tied to.

Any suggestions @devisnik?

@mr-archano
Copy link
Contributor Author

I have added support for snapshot builds in feb0c46. This means that we could give this a go and revert back the changes if anything is wrong.

@mr-archano
Copy link
Contributor Author

@devisnik I have looked into the tests again, and I found out that the assertions were completely bogus 😓
The assertions in TestGeneratePomTask are not querying the XML correctly: if you change the strings there for instance the test is still successful 🙈 🙈 🙈

@mr-archano
Copy link
Contributor Author

@passsy mentioned in #112 (comment) that if we want to generate scopes and exclusions correctly for Android artifacts, we will basically end up re-implementing the internal behaviour of the MavenPublishPlugin.
In that sense we will be still bound to internals, so I wonder if we should really just keep relying on SoftwareComponentInternal 🤔 🤔 🤔

It would be great if we could get input from @bmuschko or @jjohannes here.

@mr-archano
Copy link
Contributor Author

Closing this PR as after some discussion we decided to change approach. New PR incoming.

@mr-archano mr-archano closed this Oct 15, 2018
@mr-archano mr-archano deleted the ISSUES/177 branch October 15, 2018 21:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants