Add PublishCredentialsModule, MavenPublish and MavenPublishModule

[maxstreese]

Summary

Implements #5592 and builds upon #5601.

High Level Overview of Changes

This PR implements three traits:

• MavenPublishModule is meant to mirror SonatypeCentralPublishModule for plain (!?) Maven repositories
• MavenModule contains the actual publishing logic. This has been taken from parts of SonatypeCentralPublishModule and is now extended by both MavenPublishModule as well ass SonatypeCentralPublishModule to share code
• MavenCredentialsModule contains tasks for obtaining Maven/Sonatype credentials. These tasks have been extracted from the SonatypeCentralPublishModule to be shared by both it and the new MavenPublishModule

Differences between this PR and #5601

Code Structure

#5601 essentially copied parts of SonatypeCentralPublishModule to create MavenPublishModule. Apart from duplicating code, this also created code you may find confusing in places (e.g. by containing a function named publishSnapshot which is used to publish both release and snapshot artifacts). This PR tries to improve upon that by instead factoring out all code which can be factored out.

Removal of Unused Settings

#5601 defines the tasks mavenConnectTimeout, mavenReadTimeout and mavenAwaitTimeout as part of the MavenPublishModule. These are however not actually used anywhere. They are used in SonatypeCentralPublishModule and are specific to publishing releases to Sonatype Central.

Bugfix for Choosing the Repo URI

The below was changed after being identified during testing with a private build.

- val uri = if (isSnapshot) releaseUri else snapshotUri
+ val uri = if (isSnapshot) snapshotUri else releaseUri

Open Question: Build Compilation Error When Extending MavenPublishModule

When running ./mill dist.installLocalCache and then using the local SNAPSHOT version of mill including the changes made in this PR in a build and extending MavenPublishModule in a local build module, I currently get the following exception when running ./mill resolve _ on that build:

[build.mill-59] [error] ## Exception when compiling 23 sources to /home/max/Repositories/github.com/ttmzero/rtp-backend/out/mill-build/compile.dest/classes
[build.mill-59] [error] dotty.tools.dotc.core.MissingType: Cannot resolve reference to type com.lumidion.sonatype.central.client.core.type.SonatypeCredentials.
[build.mill-59] [error] The classfile defining the type might be missing from the classpath.

This error can be removed by defining Deps.sonatypeCentralClient as a mvnDeps instead of both a compileMvnDeps and a runMvnDeps in libs/scalalib/package.mill and libs/javalib/package.mill. I do not know why this is the case and would require some help here.

[maxstreese]

Hi @lefou, this is my attempt at implementing #5592, building upon @chikei's work in #5601. In the end it does contain quite a few more changes. I hope that's ok, this is what I could come up with. Let me know if this is a non starter or what changes are required to get this merged.

Many thanks (also to @chikei)!

[maxstreese]

Hi @lefou I provided more details in the PR description and fixed a bug. Please have a look! Regarding the open question mentioned in the description I would require some pointers.

[maxstreese]

Hey @lefou and @lihaoyi, any chance we can move this further along? I know you two are busy and this is all in our free time so I don't want to intrude. It would just be nice to get an indication on timeline.

To lobby a little for this feature (not necessarily this PR):

1. As stated in the Mill docs and Publishing to maven repository other than central #5592, the current way to publish to a private Maven repository is via a deprecated module, which is not ideal
2. Furthermore, using the deprecated approach yields a result which is not completely compliant with the Maven repository layout, because it does not create a maven-metadata.xml. This file is relied upon by most tools to e.g. check for version updates

I'd be happy to implement any changes and suggestions or discuss. Many thanks!

[lihaoyi]

Kicked the failing tests to check if they're just flaky, if so we can merge this

[lihaoyi]

@maxstreese I think maybe the last thing necessary is to update the https://mill-build.org/mill/javalib/publishing.html#_publishing_to_other_repositories section of the docs to include an example of how to use this. Doesn't need to be an executable example test since that would require setting up a supporting maven repository server, just some code snippets and explanation text on what users should expect should be sufficient

[lihaoyi]

CC @lefou in case you have any feedback before I merge this

[lihaoyi]

Could you add some scaladoc to this trait and its companion object, and explain what is the difference between this and the normal trait PublishModule and object SonatypeCentralPublishModule we typically tell people to use?

[lihaoyi]

• Also, do we need this trait at all? Ideally we should just have everyone extend PublishModule and use different companion object external modules to publish them to different places

[maxstreese]

Regarding "do we need this trait at all": I was just exactly following what is already done in trait SonatypeCentralPublishModule and its companion object. Therefore it seems clean to me to not break with that design?

Regarding what the difference is: What SonatypeCentralPublishModule does for Sonatype Central, MavenPublishModule does for "plain" Maven. I can certainly make this distinction clear by providing a scaladoc for both modules.

To try to explain my understanding as best as possible: SonatypeCentralPublishModule is specifically for publishing to the service https://central.sonatype.com/. Because of at least staging releases (not sure if something else), this is not exactly the same as publishing to a repository that "just" follows the Maven2 Repository Layout (even if that repo is managed by the Sonatype software). That is the reason why MavenPublishModule has been brought to life. Now to go even deeper into this, I believe it should be possible and in fact probably also cleaner and closer to reality, to have all of this functionality in one module and/or publish task, which can be configured in a multitude of ways (e.g. publish to central or private; specify credentials or not; cryptographically sign or not). This is what was actually done in the deprecated publishAll task of the PublishModule itself and is stated in the Mill docs (though - if I remember my testing correctly - was not 100 % compliant for "plain" Maven publishing because it did not create maven-metadata.xml). Implementing this unified approach is something that I consider both out-of-scope for this PR and a bit out of my comfort zone. Though I'd certainly be open to try if you agree :)

[lihaoyi]

I think the important thing is to make sure ./mill mill.javalib.MavenPublishModule/ works for publishing normal mill.javalib.publish.PublishModules, and not just this particular trait. The same way that ./mill mill.javalib.SonatypeCentralPublishModule/ works on normal mill.javalib.publish.PublishModules without requiring the individual modules extend mill.javalib.SonatypeCentralPublishModule

If that works, then we can probably do without this trait and just ask people to use the companion object

[maxstreese]

In my understanding ./mill mill.javalib.MavenPublishModule/ works for publishing normal mill.javalib.publish.PublishModules with the current implementation in this PR, because MavenPublishModule is an ExternalModule, it's default task is publishAll and publishAlls publishArtifacts parameter by default resolves to __:PublishModule.publishArtifacts (following exactly what is done in SonatypeCentralPublishModule). Or am I missing something here?

I would propose to still keep the MavenPublishModule trait for two reasons:

1. This follows the design of SonatypeCentralPublishModule and therefore is uniform
2. It gives users the option to either run ./mill mill.javalib.MavenPublishModule/ or (for example) ./mill __.publishMaven. I think giving users the second option is important, because of the following scenario: What if I want to publish some of my modules to Sonatype Central and some to a private repo? Or what if I want to publish to different private repos? When keeping the MavenPublishModule trait, this is easy to express in the build by extending it in different modules and supplying different parameters. I think with the external module alone this would be more tricky?

[lefou]

Just to clarify, the external module's command accepting a --publish-artifacts option itself accepting a task query like __.publishArtifacts or <my-module>.publishArtifacts should already allow to publish selected modules via different methods.

[maxstreese]

Ah I was missing this part, thanks! So what I could do to put this in - say - a shared module in a build is something like the below right?

package millbuild

import mill.*
import mill.api.Task.Simple
import mill.javalib.MavenPublishModule
import mill.scalalib.*
import mill.scalalib.publish.*
import mill.util.Tasks

trait FooModule extends ScalaModule, PublishModule:

  def publishMaven = MavenPublishModule.publishAll(
    publishArtifacts = Tasks(Seq(publishArtifacts)),
    username = "",
    password = "",
    bundleName = "",
    releaseUri = "",
    snapshotUri = "",
  )

  def publishVersion = "0.1.0"

  def pomSettings = PomSettings(
    description = "",
    organization = "",
    url = "",
    licenses = Seq.empty,
    versionControl = VersionControl.github("", ""),
    developers = Seq.empty,
  )

In this case should I provide this option in the docs? And related: Is there a different way than using the Tasks util for this?

[lihaoyi]

I think expecting people to pass the values via the CLI or env variables would be fine for now, that's what we mostly recommend for SonatypePublishModule and it seems to work

[lihaoyi]

If SonatypeCredentialsModule is being used for non-sonatype publishing as well, we should mention that in its scaladoc

[maxstreese]

I actually had not considered this inconsistency. If we continue with my current implementation, we would actually expect the user to provide the environment variables MILL_SONATYPE_USERNAME and MILL_SONATYPE_PASSWORD for publishing to "plain" Maven as well. I guess I was subconsciously thinking "it will also be Sonatype". I am not sure what the right path forward is here. Should we keep it as is and just add the scaladoc? Or should we maybe rename this module and make the environment variable different for Sonatype and "plain" Maven?

[lihaoyi]

I think maybe let's rename the environment variable to plain MAVEN_PUBLISH_USERNAME and MAVEN_PUBLISH_PASSWORD for people using MavenPublishModule?

[maxstreese]

Hi @lihaoyi and @lefou,

I have addressed your code reviews, please have another look!

Things that have been changed

1. SonatypeCredentialsModule has been renamed to PublishCredentialsModule to address @lihaoyi 's review.
2. MavenPublishModule now uses the environment variables beginning with MILL_MAVEN_ instead of MILL_SONATYPE_
3. The trait of MavenPublishModule has been removed and only the object (i.e. ExternalModule) remains
4. Documentation for MavenPublishModule has been added to the publish sections

Dependency bug found during testing

I believe I have also found a bug related to using the ExternalModule of either SonatypePublishModule or MavenPublishModule from within a build definition (in contrast to from the command line). I would be great if you could re-produce this.

Take the below dummy build.mill and run e.g. ./mill resolve _ on it:

//| mill-version: 1.0.6
//| mill-jvm-version: temurin:21

package build

import mill.*
import mill.scalalib.*
import mill.scalalib.publish.*

import mill.util.Tasks
import mill.javalib.SonatypeCentralPublishModule

object `package` extends ScalaModule, PublishModule:

  def scalaVersion = "3.7.4"

  def publishVersion = "0.1.0"

  def pomSettings = PomSettings(
    description = "",
    organization = "",
    url = "",
    licenses = Seq.empty,
    versionControl = VersionControl.github("", ""),
    developers = Seq.empty,
  )

  def publishSonatypeCentral = SonatypeCentralPublishModule.publishAll(
    publishArtifacts = Tasks(Seq(publishArtifacts)),
  )

What I get is:

[build.mill-59] [error] -- [...]/build.mill:28:61
[build.mill-59] [error] 28 │  def publishSonatypeCentral = SonatypeCentralPublishModule.publishAll(
[build.mill-59] [error]    │                                                            ^
[build.mill-59] [error]    │Cannot resolve reference to type com.lumidion.sonatype.central.client.core.type.SonatypeCredentials.
[build.mill-59] [error]    │The classfile defining the type might be missing from the classpath.
[build.mill-59] [error] one error found

I am pretty sure this is due to these lines here (and in the scalalib module respectively). Should I create a separate PR for that or change the dependency to be part of the mvnDeps of those modules as part of this PR?