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

Add BOM / dependency management support #3924

Merged
merged 43 commits into from
Dec 5, 2024

Conversation

alexarchambault
Copy link
Contributor

@alexarchambault alexarchambault commented Nov 8, 2024

This PR adds support for user-specified BOMs and dependency management in Mill.

BOM support allows users to pass the coordinates of an existing Maven "Bill of Material" (BOM), such as this one, that contains versions of dependencies, meant to override those pulled during dependency resolution. (They can also add exclusions to dependencies.)

def bomDeps = Agg(
  ivy"io.quarkus:quarkus-bom:3.17.0"
)

It also allows users to specify the coordinates of a parent POM, which are taken into account just like a BOM:

def parentDep = ivy"org.apache.spark::spark-parent:3.5.3"

(in line with PublishModule#pomParentProject that's been added recently)

It allows users to specify "dependency management", which act like the dependencies listed in a BOM: versions in dependency management override those pulled transitively during dependency resolution, and exclusions in its dependencies are added to the same dependencies during dependency resolution.

def dependencyManagement = Agg(
  ivy"com.google.protobuf:protobuf-java:4.28.3",
  ivy"org.java-websocket:Java-WebSocket:_" // placeholder version - this one only adds exclusions, no version override
        .exclude(("org.slf4j", "slf4j-api"))
)

BOM and dependency management also allow for "placeholder" versions: users can use _ as version in their ivyDeps, and the version of that dependency will be picked either in dependency management or in BOMs:

def bomDeps = Agg(
  ivy"com.google.cloud:libraries-bom:26.50.0"
)
def ivyDeps = Agg(
  ivy"com.google.protobuf:protobuf-java:_"
)

A tricky aspect of that PR is that details about BOMs and dependency management have to be passed around via several paths:

  • in the current module: BOMs and dependency management have to be taken into account during dependency resolution of the module they're added to
  • via moduleDeps: BOMs and dependency management of module dependencies have to be applied to the dependencies of the module they come from
  • to transitive modules pulled via moduleDeps: BOMs and dependency management of a module dependency have to be applied to the dependencies of modules they pull transitively (if A depends on B and B depends on C, from A, the BOMs and dep mgmt of B apply to C's dependencies too) (worked out-of-the-box with the previous point, via transitiveIvyDeps)
  • via ivy.xml: when publishing to Ivy repositories (like during pubishLocal), BOMs and dep mgmt details need to be written in the ivy.xml file, so that they're taken into account when resolving that module from the Ivy repo
  • via POM files: when publishing to Maven repositories, BOMs and dep mgmt details need to be written to POMs, so that they're taken into account when resolving that module from the Maven repo

Fixes #1975

@himanshumahajan138
Copy link
Contributor

Sir @alexarchambault, will this add bom support for jetpack compose libs also?

@alexarchambault
Copy link
Contributor Author

If it only relies on a "BOM", that should yes. But this PR isn't about the Gradle module metadata stuff, that was mentioned too in one of your PRs.

@himanshumahajan138
Copy link
Contributor

Actually i think this will work(not sure) lets see after it gets merged then i will try this once

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Don't we need to take transitive bomDeps into account? In it's current draft, bomDeps is only used intransitively.

As a rule of thumb, transitive moduleDeps should behave the same as transitive ivyDeps. If an upstream dependency import a BOM and that BOM is considered by coursier when resolving dependencies, it should also work, if that upstream dependency is a local module.

@alexarchambault
Copy link
Contributor Author

@lefou Yes, that's what I meant by "make things working for multi-module projects". In the ITs, I intend to publish locally things in a directory, and check that resolution by coursier of stuff published locally gives the same class path as Mill computes internally.

 Conflicts:
	scalalib/src/mill/scalalib/PublishModule.scala
	scalalib/src/mill/scalalib/publish/Pom.scala
@alexarchambault

This comment was marked as outdated.

 Conflicts:
	build.mill
	main/util/src/mill/util/CoursierSupport.scala
	scalalib/src/mill/scalalib/CoursierModule.scala
	scalalib/src/mill/scalalib/JavaModule.scala
	scalalib/src/mill/scalalib/Lib.scala
@alexarchambault alexarchambault force-pushed the bom-support branch 2 times, most recently from 2843192 to 067d84d Compare November 20, 2024 18:33
@alexarchambault

This comment was marked as outdated.

@lihaoyi
Copy link
Member

lihaoyi commented Dec 4, 2024

But I just removed parentDep given that it was confusing and didn't bring much on its own

If this is gone, do we still support marking the BOM as "parent" in the POM of the project? Like by taking the first of the bomDeps or something? Or is it now unsupported?

//
// Also, by default, grpc-protobuf `1.67.1` pulls version `3.25.3` of protobuf-java (`com.google.protobuf:protobuf-java`) .
// But the BOM specifies another version for that dependency, `4.28.3`, so
// protobuf-java `4.28.3` ends up being pulled here.
Copy link
Member

Choose a reason for hiding this comment

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

We should mention what happens if there are multiple bomDeps and how overlaps and resolved. You mentioned it in the PR discussion, but it should be included in the docs since people will want to know

Copy link
Member

@lihaoyi lihaoyi Dec 4, 2024

Choose a reason for hiding this comment

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

Also we should include

If users specify both bomDeps and dependencyManagement, versions from the latter ought to be picked, but I'd like to check that and add non-regression tests for it.

Versions from dependencyManagement should be picked IMO, so that things are consistent with ivyDeps: if a version is in ivyDeps, we use this one. Else the one from dependencyManagement. Else the one obtained via bomDeps. The version "specified the closest" to the ivyDeps wins.

Copy link
Member

Choose a reason for hiding this comment

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

For exclusions, all exclusions from all BOMs added to a dependency are added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For exclusions, all exclusions from all BOMs added to a dependency are added.

Adding that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also we should include

If users specify both bomDeps and dependencyManagement, versions from the latter ought to be picked, but I'd like to check that and add non-regression tests for it.

Versions from dependencyManagement should be picked IMO, so that things are consistent with ivyDeps: if a version is in ivyDeps, we use this one. Else the one from dependencyManagement. Else the one obtained via bomDeps. The version "specified the closest" to the ivyDeps wins.

For those, I'm going to add them in a subsequent PR. I wrote tests locally to assert that, and they don't pass (yet). That might need changes in coursier. I'd merge the PR here and leave that behavior unspecified for a (short) amount of time.

@lihaoyi
Copy link
Member

lihaoyi commented Dec 4, 2024

Some minor comments. I didn't review the code changes in detail, but I expect the existing and new test suites should give us confidence that things work

@lihaoyi
Copy link
Member

lihaoyi commented Dec 4, 2024

@lefou I'm planning on merging this once the next round of review updates is done, so take a look if you have time

* )
* }}}
*/
def dependencyManagement: T[Agg[Dep]] = Task { Agg.empty[Dep] }
Copy link
Member

@lihaoyi lihaoyi Dec 4, 2024

Choose a reason for hiding this comment

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

Should we name this def depManagement? We use deps/Deps throughout the rest of the codebase, so this would make the API a bit more consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

depMgmt could work too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went for depManagement for now

@alexarchambault
Copy link
Contributor Author

But I just removed parentDep given that it was confusing and didn't bring much on its own

If this is gone, do we still support marking the BOM as "parent" in the POM of the project? Like by taking the first of the bomDeps or something? Or is it now unsupported?

That's unsupported. Users have to explicitly override PublishModule#pomParentProject themselves, with the right BOM coordinates.

@lefou
Copy link
Member

lefou commented Dec 4, 2024

But I just removed parentDep given that it was confusing and didn't bring much on its own

If this is gone, do we still support marking the BOM as "parent" in the POM of the project? Like by taking the first of the bomDeps or something? Or is it now unsupported?

That's unsupported.

Users have to explicitly override PublishModule#pomParentProject themselves, with the right BOM coordinates.

I don't think there is any (valid) use case for that. The only I could imagine is a user trying to produce poms with Mill (as it's easy) without actually building the artifacts with Mill. Or some strange corporate setup, where using a specific parent pom is mandatory. But in such a scenario, Mill is most likely not allowed anyways. It would lead to inconsistencies in Mill vs. Maven usage too, if the parent pom has any significant content.

@alexarchambault
Copy link
Contributor Author

alexarchambault commented Dec 4, 2024

Not really sure what the exact use case would be either, TBH

Should help in subsequent PRs
@lihaoyi lihaoyi merged commit 60e6ce2 into com-lihaoyi:main Dec 5, 2024
27 checks passed
@lihaoyi
Copy link
Member

lihaoyi commented Dec 5, 2024

This looks great, thanks @alexarchambault! Looking forward to the follow ups. I also chatted with the Bazel folks and they seem pretty interested in making use of the new BOM support in Coursier!

@lefou lefou added this to the 0.12.4 milestone Dec 5, 2024
@lefou
Copy link
Member

lefou commented Dec 5, 2024

@lefou I'm planning on merging this once the next round of review updates is done, so take a look if you have time

So, I did take a look, I also commented and there were open unresolved remarks but this was merged anyways. I don't think we should compromise consistency for quickly merging features. It looks like the whole Scala build tool community seem to be interested in BOM support, so we should also try to find a nice user experience. It's technically complicated enough. The inconsistent empty version vs. _ wildcard version isn't great to start with.

@lihaoyi
Copy link
Member

lihaoyi commented Dec 5, 2024

I thought the resolution was that _ wildcard was just going to be an implementation detail and not a supported feature? That was my understanding but apologies if I'm mistaken.

@lefou
Copy link
Member

lefou commented Dec 5, 2024

Thanks. This is a complex issue after all.

The _ is used in coursier's Dependency class, which is a fully leaking abstraction. So Mill users will see it all over the places. See this comment: #3924 (comment)

Although we don't accept a _ in our ivy String context, we will see it when we use ivy"a:b".version or print a ivyDepsTree for example. Edit: Although I'm not sure about the tree, as it should only contain resolved versions. But error messages for coursier will most likely contain the _.

@lihaoyi
Copy link
Member

lihaoyi commented Dec 5, 2024

@alexarchambault how hard would it be to tweak the coursier parser upstream to allow foo:bar without the trailing :_? Seems to me like it shouldn't be too much of a burden right?

@alexarchambault
Copy link
Contributor Author

Having a look, to me the main point of _ as a version was to mark missing versions in the inputs of coursier. While empty versions originate from versions not written down in POMs (whose value should be somewhere in a BOM). So having a _ in error messages means something went wrong in the upfront processing of inputs, while an empty version means something went wrong in the resolver internals.

Admittedly, the _ can always be dropped… It shouldn't be that much of a big deal to tweak the coursier parser, I'm having a look

lihaoyi pushed a commit that referenced this pull request Dec 6, 2024
This adds tests to ensure the versions in `JavaModule#depManagement`
have precedence over those coming via `JavaModule#bomDeps`. The tests
fail for now (might need changes in coursier…)

Includes #3924 for now
lihaoyi pushed a commit that referenced this pull request Dec 14, 2024
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.

Support for (transitive) dependency management / override
4 participants