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

Support customizing coursier dependency resolution (make OS specific classifiers work) #775

Merged
merged 3 commits into from
Apr 26, 2021

Conversation

lefou
Copy link
Member

@lefou lefou commented Feb 14, 2020

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.

@lefou lefou requested a review from lihaoyi February 14, 2020 13:59
@lefou
Copy link
Member Author

lefou commented Feb 14, 2020

If we decide, to make resolution OS specific, I could imagine some dedicated target in CoursierModule to fine control OS specific resolution, e.g. to still allow cross platform projects.

@lihaoyi
Copy link
Member

lihaoyi commented Feb 20, 2020

Having a dedicated target on CoursierModule you can override seems like the right thing to do.

It could default to coursier.core.Activation.Os.fromProperties(sys.props.toMap) by default, but you could override it if you want it to have some other value (e.g. you are on OSX but deploying to Linux) and you could override it with a T.input(T.ctx.env("...")) if you want it to be configurable from the command line as an environment variable

@lefou
Copy link
Member Author

lefou commented Apr 24, 2021

I'm going to update this PR. This is the change I have in mind: I'd like to add a resolutionCustomizer task to CoursierModule.

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
@ajrnz
Copy link
Contributor

ajrnz commented Apr 25, 2021

I've just tested this PR out and it works correctly. Much nicer. Thanks.

@lefou
Copy link
Member Author

lefou commented Apr 25, 2021

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 resolutionCustomizer task in CoursierModule, I think we can merge as-is.

@lefou lefou changed the title Support some magic OS specific variables, e.g. to resolve classifiers Support customizing coursier dependency resolution (make OS specific classifiers work) Apr 25, 2021
@lefou lefou marked this pull request as ready for review April 25, 2021 19:28
@alexarchambault
Copy link
Collaborator

It's fine to allow users to customize the Resolution, IMO. The only possible issue is it ties the mill API to the one of coursier even more, which could be an issue if the API of coursier were to change. But it's less likely to happen since coursier switched to data-class.

@lefou
Copy link
Member Author

lefou commented Apr 26, 2021

@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.

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.

Can't compile JavaFX projects (regression?)
4 participants