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

experimental implementation of Configurer that uses maven-publish #32

Merged
merged 2 commits into from
Oct 15, 2018
Merged

experimental implementation of Configurer that uses maven-publish #32

merged 2 commits into from
Oct 15, 2018

Conversation

gabrielittner
Copy link
Collaborator

Android libraries aren't supported right. We'd need a SoftwareComponent implementation for those which is tracked here: https://issuetracker.google.com/issues/37055147. Which is blocked by gradle/gradle#1842. The latter is also preventing us of providing our own implementation of SoftwareComponent unless we're ok with internal APIs. Here is an example of how to do it without SoftwareComponent, but for Android adding the dependencies to the pom is more complex

This brings one behavior change for the old implementation. The general configuration inside the Configurers init is now done in afterEvaluate because we need to wait until we can read the new useMavenPublish property.

I still want tests for at least the generated poms. Going to look into that next week.

@codecov
Copy link

codecov bot commented Oct 14, 2018

Codecov Report

Merging #32 into master will decrease coverage by 21.85%.
The diff coverage is 7.69%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master      #32       +/-   ##
=============================================
- Coverage     88.95%   67.09%   -21.86%     
  Complexity       40       40               
=============================================
  Files             6        8        +2     
  Lines           172      234       +62     
  Branches         15       20        +5     
=============================================
+ Hits            153      157        +4     
- Misses           11       67       +56     
- Partials          8       10        +2
Impacted Files Coverage Δ Complexity Δ
.../main/kotlin/com/vanniktech/maven/publish/Utils.kt 0% <0%> (ø) 0 <0> (?)
...vanniktech/maven/publish/MavenPublishConfigurer.kt 0% <0%> (ø) 0 <0> (?)
...nniktech/maven/publish/UploadArchivesConfigurer.kt 66.66% <100%> (+5.95%) 8 <0> (ø) ⬇️
...ktech/maven/publish/MavenPublishPluginExtension.kt 72.22% <100%> (+1.63%) 4 <0> (ø) ⬇️
...vanniktech/maven/publish/MavenPublishPlugin.groovy 91.95% <42.85%> (-4.35%) 3 <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 fe4759c...fd48f06. Read the comment docs.

releaseRepositoryUrl
}
// the releaseRepositoryUrl is null checked in the plugin
return URI.create(@Suppress("UnsafeCallOnNullableType") url!!)
Copy link
Owner

Choose a reason for hiding this comment

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

Could use requireNotNull(url) instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done fd48f06


private fun MavenPublishTarget.repositoryUrl(version: String): URI {
val url = if (version.endsWith("SNAPSHOT")) {
snapshotRepositoryUrl ?: releaseRepositoryUrl
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we default to release url here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To mirror the uploadArchives behavior. When no snapshot repository is specified it will always upload to the non snapshot repository. This is also how installArchives works which doesn't have a separate snapshot repository.

We could make it more explicit and require a snapshotRepositoryUrl for snapshots. For installArchives we'd then just set it to the same path.

repo.name = target.repositoryName
repo.url = target.repositoryUrl(project.version.toString())
if (target.repositoryUsername != null) {
repo.credentials { it ->
Copy link
Owner

Choose a reason for hiding this comment

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

It is the default and hence we can remove it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done fd48f06

import org.gradle.plugins.signing.SigningExtension
import java.util.concurrent.Callable

internal val Project.signing get() = extensions.getByType(SigningExtension::class.java)
Copy link
Owner

Choose a reason for hiding this comment

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

Make all of them inline?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done fd48f06

@gabrielittner gabrielittner merged commit 71160ea into vanniktech:master Oct 15, 2018
@gabrielittner gabrielittner deleted the maven-publish branch October 15, 2018 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants