Skip to content

Remove CoursierModule#mapDependencies #5070

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

alexarchambault
Copy link
Collaborator

This PR removes CoursierModule#mapDependencies altogether, and uses CoursierModule#resolutionParams instead. mapDependencies is hack that the coursier API allows, but it's too powerful IMO. A benefit of using resolution params instead is that almost anything done via resolution params can be done via the coursier command-line, which is handy to debug things.

Copy link
Member

@lihaoyi lihaoyi left a comment

Choose a reason for hiding this comment

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

Looks fine to me I guess, but would be would be good to have a slightly longer explanation in the PR description of what the change means. What kind of code would be going away? What does the replacement look like?

@lefou
Copy link
Member

lefou commented May 6, 2025

As we have users of this API, a discussion how to replace it, would be nice. E.g. my main usage is:

  • Replace some transitive dependencies (change coordinates, e.g. replace some logging impl with a different impl; switch from non-osgi to separately released osgi artifact)
  • Bump or enforce transitive versions

lihaoyi pushed a commit that referenced this pull request May 15, 2025
## Summary

Following @alexarchambault work on
#4626 I've done some dependency
cleanup + others . The aim is to remove as many dependency management
workarounds from the past android attempts as possible and rely solely
on coursier

## Dependency cleanup

I have managed to cleanup most of the examples but I've introduced some
small workarounds in two places:
1. Screenshot testing: This is kind of a special case, first even in AGP
it's super experimental, and I had to use the defaultResolver to get the
runtime dependencies in the tree and put them in the screenshot
classpath
2. In the hilt example, I had to add a few dependencies by hand that are
marked as runtime only by hand (see below the runtime section). On the
good news I've removed the mapDependencies workaround.

## Runtime management

- I have introduced some scaffolding to later separate the runtime
classpath from the compile time in an effort to have a more flexible way
of choosing what to add in the APK. I'm not entirely convinced this is
the way to go, it only kind of worked in the screenshot testing and unit
testing. I'd like to have it merged for now for later experimentation
and if it doesn't get me anywhere I'll remove it.
- I've introduced 2 new methods for managing android dependencies on top
of the resolvedMvnDeps and resolvedRunMvnDeps for further flexibility,
my plan is to allow for further experimentation with coursier (I've
already tried locally some configurations by overriding resolvedMvnDeps
for example) but haven't found the ideal way for reducing the need to
(re)declare some runtime dependencies.

## Android resources

I've separated the /resources from /res using `androidResources` and did
some housekeeping around classpath resolution.

## Things left to do

I think I'll need some help from @alexarchambault , I saw this PR
#5070, and I'd like some advice
on an alternative approach to use endorse strict versions

## Test hardening

- Hilt example now installs the app and runs the activity, to make sure
that things are working further than dex packaging
- Kotlin compose example also has the same test arrangement to make sure
the apk is installable and runnable

## R8 fixes

On experimenting with 2-compose example, I've found that main dex args
are not available for compiling against Api level 21 (so building the
compose example with R8 breaks). I removed it for now, it's not a good
default, might revisit later on on adding it for older versions of
android
 Conflicts:
	build.mill
	core/util/src/mill/util/Jvm.scala
	example/androidlib/kotlin/2-compose/build.mill
	example/androidlib/kotlin/3-compose-screenshot-tests/build.mill
	example/thirdparty/androidtodo/build.mill
	integration/ide/gen-idea/resources/extended/idea/mill_modules/mill-build.iml
	integration/ide/gen-idea/resources/extended/idea/mill_modules/mill-build.mill-build.iml
	integration/ide/gen-idea/resources/hello-idea/idea/mill_modules/mill-build.iml
	libs/scalalib/src/mill/scalalib/CoursierModule.scala
	libs/scalalib/src/mill/scalalib/JavaModule.scala
	libs/scalalib/src/mill/scalalib/Lib.scala
@alexarchambault
Copy link
Collaborator Author

Bump or enforce transitive versions

You should use CoursierModule#resolutionParams for that. eb223b6 illustrates that.

@alexarchambault
Copy link
Collaborator Author

Replace some transitive dependencies (change coordinates, e.g. replace some logging impl with a different impl; switch from non-osgi to separately released osgi artifact)

Is that use case that widespread? AFAIR, no one asked for this in coursier, or in the context of its use in sbt.

As of now, there's no out-of-the-box replacement for that. If you know the dependency you want to replace is pulled, you can exclude it from the other dependencies, and add the one you're interested in alongside those.

If you're unsure if it's pulled, it might be more complicated. You may want to resolve the dependencies pulling the wrong one in a dummy module, and depending on whether these pull the wrong dependency, do like just above.

@lefou
Copy link
Member

lefou commented May 22, 2025

Replace some transitive dependencies (change coordinates, e.g. replace some logging impl with a different impl; switch from non-osgi to separately released osgi artifact)

Is that use case that widespread? AFAIR, no one asked for this in coursier, or in the context of its use in sbt.

Probably not so widely required, but more often needed than anticipated. (The list of examples was not complete.) There is a reason why I choose Mill over other build tools. When I had to manage such scenarios with Maven, I used a tool (cmvn) that generated Maven projects, so I was able to configure module-wide exclusions, which were rolled out to every dependency. Exclusions aren't a nice fit, as you can't express them module-wide. You can roll them out programmatically, but that still gives you a large unmanageable tree.

As of now, there's no out-of-the-box replacement for that. If you know the dependency you want to replace is pulled, you can exclude it from the other dependencies, and add the one you're interested in alongside those.

This is costly work and more importantly, it produced hard to maintain setup, since you fix symptoms and don't express the intent.

Thing is, mapDependencies might be a hack, but it was added for a reason and it is there and works. You "think" it is too powerful, ok, but should we remove it? We could document it as advanced and discourage use, ideally list the issues it produces. Currently, the downsides are totally unclear to me.

@lefou
Copy link
Member

lefou commented May 22, 2025

When "enterprise" software with hundreds of modules and dependencies gets maintained/audited, you sometimes have to mangle some specific dependencies. Be it the exclusion of some vulnerable but unused classes or any other unthinkable task, and you can't always use a latest version or whatever. You also can't publish every hack into some repository to consume it, and falling back to unmanaged dependency is a death sentence. Powerful ways to mangle the dependency tree without loosing all the other standards is what makes Mill the greater alternative to Maven and Gradle.

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