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

Allow users to define and publish their own BOM in Mill #4155

Merged
merged 48 commits into from
Jan 24, 2025

Conversation

alexarchambault
Copy link
Collaborator

@alexarchambault alexarchambault commented Dec 17, 2024

This PR allows users to define their own BOMs in Mill, as Mill modules, via a new BomModule type.

BomModule inherits JavaModule, but has no sources and creates no JARs, only a POM. This PR also ensures one can publish JavaModules that have no JAR, like is the case for BomModule.

It adds non-regression tests illustrating various precedence rules of BomModule versus external BOMs (bomIvyDeps) and manual dependency management (via depManagement). Note that some cases might feel ambiguous - precedence rules could be changed later on if needed.

This changes the way transitive dependencies, but also transitive BOM
dependencies and dependency management, are passed to coursier.

Before this commit, Mill was walking its own module graph, and
dependencies / BOM dependencies / dep management were manually aggregated
beforehand. During dependency resolution, coursier also does such
aggregations.

Having both Mill and coursier handle that concern could lead to discrepancies.
So this commit passes non-processed dependencies / BOM dependencies /
dep management as-is to coursier, and lets coursier handle / compose
those.
@alexarchambault

This comment was marked as resolved.

@alexarchambault alexarchambault marked this pull request as ready for review January 9, 2025 10:42
 Conflicts:
	build.mill
	main/package.mill
	scalalib/src/mill/scalalib/JavaModule.scala
	scalalib/src/mill/scalalib/PublishModule.scala
@lihaoyi
Copy link
Member

lihaoyi commented Jan 23, 2025

I think going with this PR as is is fine, we already have a ticket to split out PublishModule from JavaModule #4338 so that will happen when we break compat in 0.13.0

@lihaoyi
Copy link
Member

lihaoyi commented Jan 23, 2025

The test failure looks pretty stubborn and isnt passing on retries; @alexarchambault do you think it could be caused by the PR?

@alexarchambault
Copy link
Collaborator Author

The test failure looks pretty stubborn and isnt passing on retries; @alexarchambault do you think it could be caused by the PR?

Let me check…

scalalib/src/mill/scalalib/JavaModule.scala Show resolved Hide resolved
Comment on lines 8 to 25
jar: os.Path,
sourcesJar: os.Path,
docJar: os.Path,
jar: Option[os.Path],
sourcesJar: Option[os.Path],
docJar: Option[os.Path],
pom: os.Path,
artifact: Artifact,
extras: Seq[PublishInfo]
Copy link
Member

Choose a reason for hiding this comment

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

I can't see it yet.

Comment on lines -19 to -21
jar: os.Path,
sourcesJar: os.Path,
docJar: os.Path,
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 rename extras to publishInfos or publishData, since this is no longer "extra" but "the" data.

@alexarchambault
Copy link
Collaborator Author

alexarchambault commented Jan 23, 2025

I'm still investigating, but I can't manage to reproduce the failure locally. Running the test succeeds:

$ ./mill 'example.thirdparty[mockito].local.server.test'

And manually cloning the mockito repository, checking out the commit of the tests, and running this succeeds too:

$ ./mill -i dist.run /path/to/mockito/ -i -j5 __.compile &&
    ./mill -i dist.run /path/to/mockito/ -i __.test

(these are the two commands ran by the example of the test)

@alexarchambault
Copy link
Collaborator Author

Seems the mockito test that doesn't pass on CI is this one (this line throws). The same mockito test failed on all 5 runs (and threw at the exact same line).

@alexarchambault alexarchambault marked this pull request as draft January 23, 2025 17:52
@lihaoyi
Copy link
Member

lihaoyi commented Jan 24, 2025

If the test passes locally I would be fine disabling it in CI

@lihaoyi
Copy link
Member

lihaoyi commented Jan 24, 2025

e.g.

> sed -i.bak 's/assertEquals(50, settings.getStubbingLookupListeners().size())//g' src/test/java/org/mockitousage/debugging/StubbingLookupListenerCallbackTest.java # disable flaky test

@lihaoyi
Copy link
Member

lihaoyi commented Jan 24, 2025

Seems the same error should_stubbing_be_treated_as_interaction as in this other PR https://github.com/com-lihaoyi/mill/actions/runs/12941764884/job/36099745683?pr=4392, so probably unrelated to your changes

@lihaoyi
Copy link
Member

lihaoyi commented Jan 24, 2025

Put up a PR to disable that test #4393

@alexarchambault alexarchambault marked this pull request as ready for review January 24, 2025 12:00
@lefou
Copy link
Member

lefou commented Jan 24, 2025

I took the opportunity to simplify the jar handling further.

@lihaoyi
Copy link
Member

lihaoyi commented Jan 24, 2025

Looks good to me, will merge it once green

@lihaoyi lihaoyi merged commit 22a2bd7 into com-lihaoyi:main Jan 24, 2025
31 checks passed
@lefou lefou added this to the 0.12.6 milestone Jan 24, 2025
@alexarchambault alexarchambault deleted the bom-modules-cs-project branch January 24, 2025 14:23
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.

3 participants