Try resolve sbt plugin from valid Maven pattern #2633

adpi2 · 2022-12-21T08:45:06Z

This PR is part of sbt/sbt#7096 whose goal is to fix sbt/sbt#3410 in a smooth way. For a full description please read sbt/sbt#7096.

See also coursier/test-metadata#7 for the added test metadata.

modules/core/shared/src/main/scala/coursier/maven/Pom.scala

julienrf

Thank you for the PR!

If we stick with this approach (special-case the support of sbt 1.x plugins) we should thoroughly document the parts of the code that handle that.

modules/core/shared/src/main/scala/coursier/core/Definitions.scala

modules/core/shared/src/main/scala/coursier/maven/MavenRepository.scala

modules/cli/src/test/scala/coursier/cli/FetchTests.scala

alexarchambault · 2022-12-21T14:46:48Z

As discussed briefly with @julienrf on other channels, I feel a bit reluctant to add more sbt-specific hacks in the resolver. The goal of switching to standard Maven paths for plugins should be to make sbt plugin handling more "standard", not to add more sbt-specific branches in coursier.

Isn't possible to handle this externally, outside of the resolver, from sbt or sbt-coursier say?

I think something along those lines could work:

when users add a plugin, make sbt or sbt-coursier detect if it uses a standard path, or the legacy one (possibly using coursier)
pass standard paths plugins as normal dependencies to coursier (that wouldn't even be aware it's an sbt plugin)
when publishing plugins, put standard paths plugin dependencies as standard dependencies in POMs (no extraDependencyAttributes for them)
to work around the diamond problem (possibly bringing both legacy and standard paths versions of a plugin), detect when such duplicates happen in the resolution output, exclude the legacy plugin, and run the resolution again.

A benefit of that approach is that it can basically be handled entirely from sbt. It might feel more hack-ish, but at least the hacks would be restricted to sbt projects.

alexarchambault · 2022-12-21T14:51:17Z

Another benefit of the approach I described would be that it would make it easier to sunset support for legacy paths in the future. The legacy path detection before resolution, and the duplicate detection right after, would just have to be removed.

adpi2 · 2022-12-22T16:51:08Z

Thanks @alexarchambault for sharing your feedback.

I understand your point. It is indeed unfortunate that sbt special handling is leaking into Coursier. However there are a number of related things that this work can solve, among which:

Resolution of sbt plugins in entreprise environment
mvnrepository.com starts indexing the sbt plugins
statistics (downloads) are available on Sonatype for sbt plugins
javadoc.io can find the scaladoc of sbt plugins
we can fix the linking of sbt plugin dependencies in Scaladex
we can use Maven to resolve sbt plugins

The price to pay, to fix those in sbt 1.x, is:

Double publication of poms and jars
More special handling in Coursier and librarymanagement for resolution

Why not wait for sbt 2.x to fix that issue?

Assuming sbt 2.x is released next year, which is uncertain, then we will have to move the entire plugin ecosystem to it, which will take some time, probably more than a year. We will have to deal with cross-building of plugins, in Scala 2.12 for sbt 1.x, and Scala 3 for sbt 2.x, which makes things harder and slower. Not that we don't know how to do it, but it will take time. And the users will continue using sbt 1.x for a few more months.

Here by fixing it in sbt 1.9, the transition to the new valid Maven pattern is fast and very smooth for all users. Plugin authors just need to bump the version of sbt and to release. It's all bi-directional compatible for plugin authors and plugin users.

In Coursier itself I think the added complexity is reasonable. It boils down to two main things:

the fallback mechanism in MavenRepository:
https://github.com/adpi2/coursier/blob/fe2ecf708e8a96d82be3aa1997b2083b57f9078a/modules/core/shared/src/main/scala/coursier/maven/MavenRepository.scala#L371-L384
the special handling of plugin dependencies in PomParser:
https://github.com/adpi2/coursier/blob/fe2ecf708e8a96d82be3aa1997b2083b57f9078a/modules/core/shared/src/main/scala/coursier/maven/PomParser.scala#L239-L244

That's about 20 lines of code, including comments. The rest of the changes are mostly 104 lines of tests, 45 lines of added utils in Definitions, among which a lot of comments to make things as clear as possible.

Isn't possible to handle this externally, outside of the resolver, from sbt or sbt-coursier say?

I think something along those lines could work:

when users add a plugin, make sbt or sbt-coursier detect if it uses a standard path, or the legacy one (possibly using coursier)

pass standard paths plugins as normal dependencies to coursier (that wouldn't even be aware it's an sbt plugin)

when publishing plugins, put standard paths plugin dependencies as standard dependencies in POMs (no extraDependencyAttributes for them)

to work around the diamond problem (possibly bringing both legacy and standard paths versions of a plugin), detect when such duplicates happen in the resolution output, exclude the legacy plugin, and run the resolution again.

I understand why you want to move the hack closer to sbt but I am afraid this is a lot more complex, and I am doubtful about your solution. Would it be even possible to do things properly:

there are more back and forth to do between sbt and Coursier with manipulation in between, and potential bugs.
We would also need to work around each of the possible case of failures and return good error reports to the user.
For the diamond problem resolution, would it even be possible to get good eviction warnings after excluding the legacy plugins.
What about the Ivy-style resolution, we still need the extra-attributes

In contrast, with the proposed implementation of that PR, the hack is minimal and everything is working as expected:

conflict resolution and eviction warning
error reports are the same
strictly no changes in the Ivy-style resolution
minimal performance cost

Another benefit of the approach I described would be that it would make it easier to sunset support for legacy paths in the future. The legacy path detection before resolution, and the duplicate detection right after, would just have to be removed.

I also think we can remove all the special handling in Coursier after sbt 2.x. That means sbt 1.x would have to live on a legacy version of Coursier but that looks fine to me.

alexarchambault · 2022-12-23T09:57:07Z

I understand your point. It is indeed unfortunate that sbt special handling is leaking into Coursier. However there are a number of related things that this work can solve, among which:

I know, these are great points, I don't propose to go against them, quite the opposite! My only concerns are about the implementation: I'd rather have sbt and sbt-coursier (which somehow belongs more to the sbt realm than to the coursier one) handle those concerns, rather than the core of coursier.

The price to pay, to fix those in sbt 1.x, is:

Double publication of poms and jars

More special handling in Coursier and librarymanagement for resolution

Precisely, I think we can achieve this without modifying coursier itself, but by modifying sbt and sbt-coursier.

alexarchambault · 2022-12-23T10:18:52Z

In Coursier itself I think the added complexity is reasonable. It boils down to two main things:

Still, there's already a hack for sbt plugins in coursier (to handle the soon-to-be-legacy non-standard paths). My feeling is this hack is here to stay, even though no new plugins will rely on it. As I'd like the coursier CLI to still be able to resolve legacy-paths plugins in the future, like cs resolve --sbt-plugin com.github.sbt:sbt-native-packager:1.9.11. As such hacks are here to stay, I'd rather not add new ones.

To be more specific, I'd see most changes implemented in sbt-coursier, roughly following those lines:

add scripted tests for the test plugins involving diamond dependencies under a sub-directory here (either in shared-1 or shared-2 if making the legacy sbt-coursier work for this too is straightforward, or in sbt-lm-coursier if this should only be supported by lm-coursier)
change things in lm-coursier until the new scripted tests pass. Resolution is handled around here in particular, that's where we should pre-process input plugin dependencies (check if legacy or new module format should be used), and post-process the resolution output if it has duplicate plugins (legacy and standard module formats).

alexarchambault · 2022-12-23T10:29:06Z

Would it be even possible to do things properly:

there are more back and forth to do between sbt and Coursier with manipulation in between, and potential bugs.

Yeah, but it's an sbt-specific hack. It should just live in sbt-land, not in the core of the resolver.

We would also need to work around each of the possible case of failures and return good error reports to the user.

Resolution returns detailed errors, so that's tractable.

For the diamond problem resolution, would it even be possible to get good eviction warnings after excluding the legacy plugins.

This could also probably be handled in sbt-land, if we put the excluded dependencies in the update report (I don't recall if we do or not…) That said, that would only be a problem during the transition period, and would only affect sbt plugins.

What about the Ivy-style resolution, we still need the extra-attributes

If the plugins are to be published in a standard way, you shouldn't need attributes anymore. Just like Scala libraries or Scala.js are libraries are simply published with suffixes such as _2.13 or _sjs1_3, sbt plugins should be published as Maven modules, with suffixes such as _1.0_2.12, and have no attributes. They should be seen as pure Maven modules by Ivy.

adpi2 · 2022-12-23T14:22:34Z

I am still not quite convinced by your solution because:

If I remove the extra-attributes from the new POM then I would need to implement your solution in librarymanagement as well, which double the complexity and the bugs.
There is a penalty cost of resolving things multiple times, with potentially different versions and dependencies. It's not just the sbt-plugins, it's the entire dependency tree that can change.
I don't want to implement the double publication for Ivy. The current Ivy pattern is valid and causes no trouble, there is no reason to migrate it in sbt 1.x. (It will change in sbt 2.x at no cost).

May I again say that this PR adds no more than 20 lines of hacks, localized in MavenRepository and PomParser, couting the 7 lines of comments.

As I'd like the coursier CLI to still be able to resolve legacy-paths plugins in the future, like cs resolve --sbt-plugin com.github.sbt:sbt-native-packager:1.9.11. As such hacks are here to stay, I'd rather not add new ones.

I would rather suggest to drop the support of sbt 1.x in Coursier, just after the release of sbt 2.x. That would be soft, as it would still be possible to do:

cs resolve org.example:sbt-example_2.12_1.0:1.0.0, which is the classic Maven resolution.
Or in case of legacy plugin, which should not happen anymore, cs launch cs:2.1.0 -- resolve --sbt-plugin org.example:sbt-example:1.0.0.

ScalaWilliam · 2022-12-23T19:21:45Z

Hi @alexarchambault! "My feeling is this hack is here to stay" -- it seems to me that if there already are SBT specific hacks in the resolver, any solution to move #2633 out of coursier, should bring with it together that old SBT specific-hack as well. Otherwise we have 2 sets of hacks in 2 different places, quadrupling the complexity of the problem.

The hack is definitely not here to stay, it is rather to enable a smooth transition between the old world and the new world. The sbt-vspp plugin (for publishing valid artifacts) is already used by at least a dozen SBT plugins, half of which adopted it without prompting. The migration into the new world has already begun - for plugins that have no dependencies on other plugins. Those that have dependencies on other plugins, such as sbt-scalajs, have a more tricky story in terms of dependencies. Once you bump to SBT 2.x, new versions of coursier can rid of this hack immediately, without breaking users of new SBT or old SBT.

Regarding Ivy, the Ivy publishing is fine and I'd steer far from touching it, as the issue is purely in the Maven-POM land due to the extra attributes and "inconsistent" file paths/POMs published.

sbt plugins were published to an invalid path with regard to Maven's specifictation. For instance, 'org/example_2.12_1.0/1.0.0/example-1.0.0.pom' is invalid because the file name 'example-1.0.0.pom' does not start with 'example_2.12_1.0', the name of the directory that should correspond to the artifact ID. As of sbt 1.9, we also publish sbt plugins to the valid path: example_2.12_1.0/1.0.0/example_2.12_1.0-1.0.0.pom For backward compatibility, Coursier should try to resolve sbt plugins from the new Maven path, and fallback to the deprecated Maven path.

Remove sbtAttrsStub from MavenRepository Remove extraAttributes handling from PomParser

adpi2

@alexarchambault This is ready for review.

As discussed in a meeting, I extracted all the sbt hacks in a new module coursier-sbt-maven-repository. See comments below for more details.

adpi2 · 2023-01-25T08:23:55Z

build.sc

@@ -49,6 +51,10 @@ object core extends Module {
  object jvm extends Cross[CoreJvm](ScalaVersions.all: _*)
  object js  extends Cross[CoreJs](ScalaVersions.all: _*)
 }
+object `sbt-maven-repository` extends Module {


Added new module coursier-sbt-maven-repository containing SbtMavenRepository, SbtPom and SbtPomParser

coursier-cli depends on this new artifact.

modules/core/shared/src/main/scala/coursier/core/Repository.scala

modules/core/shared/src/main/scala/coursier/maven/Pom.scala

adpi2 · 2023-01-25T08:35:41Z

modules/coursier/shared/src/main/scala/coursier/params/MavenMirror.scala

-      case m: MavenRepository =>
+      case m: MavenRepositoryBase =>


To make sure that both MavenRepository and SbtMavenRepository support mirrors.

adpi2 · 2023-01-25T08:36:04Z

modules/coursier/shared/src/test/scala/coursier/ResolveTests.scala

@@ -842,21 +842,6 @@ object ResolveTests extends TestSuite {
      }
    }

-    "config handling" - async {


Moved to SbtPluginResolveTests in tests

adpi2 · 2023-01-25T08:37:03Z

project/modules/sbt-maven-repository0.sc

+    Deps.dataClass
+  )
+}
+trait SbtMavenRepositoryJvmBase extends SbtMavenRepository with CsMima


Here I don't think we need shading, since it only depends on core which is already shaded.

In particular, emove MavenRepositoryBase, and replace it with MavenRepositoryLike (meant to be used by users) and MavenRepositoryInternal (purely internal, where MavenRepository and SbtMavenRepository shared implems)

modules/core/shared/src/main/scala/coursier/maven/MavenRepositoryLike.scala

alexarchambault · 2023-02-16T09:44:54Z

Ok, so merging this! (Going to look at https://github.com/coursier/sbt-coursier/pull/445/files next)

Thanks @adpi2!

ScalaWilliam · 2023-02-16T10:08:12Z

Thank you @adpi2 , @alexarchambault , this is really exciting!

adpi2 · 2023-02-22T09:02:45Z

Thanks @alexarchambault for taking care of this while I was away. Getting rid of the SbtPomParser and reducing the diff are really nice.

Since coursier/coursier#2633 handling for resolving sbt plugins from Maven repositories got extracted into `SbtMavenRepository` in the `coursier-sbt-maven-repository` module

This was referenced Dec 21, 2022

Add sbt-plugin-example-diamond to test coursier#2633 coursier/test-metadata#7

Merged

[1.9.x] Smooth transition to valid Maven pattern of sbt plugins sbt/sbt#7096

Merged

The maven pattern for sbt plugins is invalid based on the spec sbt/sbt#3410

Closed

adpi2 requested a review from alexarchambault December 21, 2022 10:12

adpi2 commented Dec 21, 2022

View reviewed changes

modules/core/shared/src/main/scala/coursier/maven/Pom.scala Outdated Show resolved Hide resolved

julienrf reviewed Dec 21, 2022

View reviewed changes

adpi2 force-pushed the sbt-plugins-maven-path branch 6 times, most recently from 942d9c9 to fe2ecf7 Compare December 22, 2022 15:31

adpi2 force-pushed the sbt-plugins-maven-path branch from fe2ecf7 to 0997bed Compare December 22, 2022 16:55

adpi2 mentioned this pull request Dec 23, 2022

Try resolve sbt plugin from valid Maven pattern sbt/librarymanagement#409

Merged

adpi2 force-pushed the sbt-plugins-maven-path branch 2 times, most recently from 6f270b3 to ab45519 Compare January 24, 2023 14:19

adpi2 added 3 commits January 24, 2023 17:01

Extract SbtMavenRepository from MavenRepository

8e11a6e

Remove sbtAttrsStub from MavenRepository Remove extraAttributes handling from PomParser

Introduce new coursier-sbt-maven-repository module

9c1f5d5

adpi2 force-pushed the sbt-plugins-maven-path branch from ab45519 to 9c1f5d5 Compare January 24, 2023 16:01

Fix mirror test and format

e949597

adpi2 commented Jan 25, 2023

View reviewed changes

adpi2 mentioned this pull request Jan 25, 2023

Update coursier to 2.1.0-RC6 coursier/sbt-coursier#445

Merged

alexarchambault added 6 commits February 10, 2023 13:57

Rework things a bit

32478ec

In particular, emove MavenRepositoryBase, and replace it with MavenRepositoryLike (meant to be used by users) and MavenRepositoryInternal (purely internal, where MavenRepository and SbtMavenRepository shared implems)

Minimize diff in Pom and PomParser

10261d1

fmt

f7fe0d9

Minimize diff

b9d6215

NIT

bd2f689

Minimize diff

968fc83

lefou reviewed Feb 13, 2023

View reviewed changes

modules/core/shared/src/main/scala/coursier/maven/MavenRepositoryLike.scala Show resolved Hide resolved

alexarchambault added 2 commits February 13, 2023 10:33

Add scaladoc to MavenRepositoryLike

a54d44c

fmt

0b7cb76

alexarchambault merged commit b5adafa into coursier:main Feb 16, 2023

fthomas mentioned this pull request Feb 16, 2023

Update coursier to 2.1.0-RC6 scala-steward-org/scala-steward#2977

Closed

mzuehlke mentioned this pull request Mar 24, 2023

Update to changes in coursier 2.1.0 scala-steward-org/scala-steward#3021

Merged

eed3si9n mentioned this pull request Jun 26, 2024

Resolve file validation errors blocking deployment of first version lumidion/sbt-sonatype-central#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Try resolve sbt plugin from valid Maven pattern #2633

Try resolve sbt plugin from valid Maven pattern #2633

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Try resolve sbt plugin from valid Maven pattern #2633

Try resolve sbt plugin from valid Maven pattern #2633

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!