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

Use bomDeps when importing Maven projects #4067

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

alexarchambault
Copy link
Collaborator

@alexarchambault alexarchambault commented Dec 3, 2024

This tries to take into account "import" dependencies when importing Maven projects (which correspond to bomModulesDeps and bomIvyDeps in Mill), and adds parent modules there too (parent modules are basically BOMs).

Checking if this breaks the current tests. Extra tests might need to be added, to test these changes.

@alexarchambault alexarchambault marked this pull request as ready for review December 5, 2024 23:27
@alexarchambault
Copy link
Collaborator Author

CI error looks like a flake

@lihaoyi
Copy link
Member

lihaoyi commented Dec 6, 2024

Lemme kick the CI and see if it turns green

@lihaoyi
Copy link
Member

lihaoyi commented Dec 6, 2024

@alexarchambault seems the test is failing reliably on CI, does it pass on your machine?

@alexarchambault
Copy link
Collaborator Author

@alexarchambault seems the test is failing reliably on CI, does it pass on your machine?

My bad, I misread the CI output. It fails on my machine too, yes.

@alexarchambault alexarchambault marked this pull request as draft December 6, 2024 11:37
@alexarchambault
Copy link
Collaborator Author

So adding the parent POM to bomDeps during Maven imports is a problem. bomDeps can only be external BOMs, fetched from a Maven repository. If the parent is a module of the project being imported, then it's not found on Maven repos, and the generated Mill build fails.

This needs a missing part of BOM support, something like BomModule, where the dependency management defined in the Maven parent module would be written to (in JavaModule#depManagement), that other modules could refer to via a new bomModules say (like bomDeps, but for local modules), and that would get published as a BOM (no sources / no JAR, pom artifact type with dependencyManagement section).

@alexarchambault
Copy link
Collaborator Author

The failing example is this one. It runs into the parent POM issue above if we add bomDeps for parent POMs to it.

@lihaoyi
Copy link
Member

lihaoyi commented Dec 6, 2024

@alexarchambault perhaps bomDeps should be named bomIvyDeps, and then have a separate bomModuleDeps to refer to local modules? That would follow the naming convention we use elsewhere in JavaModule

lihaoyi pushed a commit that referenced this pull request Dec 11, 2024
I had to update the test data of `mill.main.maven.BuildGenTests` for
#4067. The output of those tests
didn't really help.

This PR changes the helper method checking if generated files match test
data in resources, so that it prints more helpful output (removed /
added files, diff of modified files), and allows to update the test data
on disk by setting `BuildGenTests.updateSnapshots` to true.
 Conflicts:
	main/init/maven/test/resources/expected/maven-samples/multi-module/server/package.mill
	main/init/maven/test/resources/expected/maven-samples/multi-module/webapp/package.mill
	main/maven/src/mill/main/maven/BuildGen.scala
	main/maven/test/resources/expected/config/all/build.mill
	main/maven/test/resources/expected/config/base-module/server/package.mill
	main/maven/test/resources/expected/config/base-module/webapp/package.mill
	main/maven/test/resources/expected/config/deps-object/server/package.mill
	main/maven/test/resources/expected/config/deps-object/webapp/package.mill
	main/maven/test/resources/expected/config/merge/build.mill
@alexarchambault
Copy link
Collaborator Author

About this PR: it tries to set JavaModule#{bomModuleDeps,bomIvyDeps} in a sensible way when importing Maven projects. It seems it doesn't break the current tests, but I didn't try it on actual Maven projects that need those tasks. So I'm not sure if it should be merged right now.

But if any one ran into issues with empty versions or faulty class paths, when importing Maven projects, this might help.

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