-
-
Notifications
You must be signed in to change notification settings - Fork 367
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
Support customizing coursier dependency resolution (make OS specific classifiers work) #775
Conversation
If we decide, to make resolution OS specific, I could imagine some dedicated target in |
Having a dedicated target on It could default to |
I'm going to update this PR. This is the change I have in mind: I'd like to add a The idea is to override it in cases you want to tweak the coursier resolution process in your module. For example, to make JavaFX compilation work, you need to override it like this: import mill._
import mill.scalalib._
object javafx extends JavaModule {
override def resolutionCustomizer = T.task {
Some((_: coursier.core.Resolution).withOsInfo(coursier.core.Activation.Os.fromProperties(sys.props.toMap)))
}
def ivyDeps = Agg(ivy"org.openjfx:javafx-controls:13.0.2")
} |
This helps in customizing coursier resolution process, e.g. to add OS or JDK specific resolution properties sometimes used by Maven
I've just tested this PR out and it works correctly. Much nicer. Thanks. |
Thanks for the feedback. I hesitate to merge this PR right now as I have no test set up for it. But it seems there is quiet a demand for this feature and (besides all other tests succeeding) your comment suggest that it works as intended. If @lihaoyi or @alexarchambault have no other objections regarding the proposed new |
It's fine to allow users to customize the |
@alexarchambault Thank you for your input. I think I'm going to merge this PR. As a matter of fact, we already depend on Coursier API, and I think it makes not much sense to introduce some abstraction for this concrete configuration point. |
This should fix #767.
TBH, I'm not sure we want this in mill. So I encourage a review and/or discussion.
On one side this fixes an issue with resolving transitive dependencies with os-specific classifiers (#767). But on the other side it makes dependency resolution less reproducible.
So a perfect outcome of this PR for me is also a rejection paired with some documentation how to deal with such situations.