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 Coursier plugins via customizing FileCache used (with classloaders) #1760

Merged
merged 4 commits into from
Mar 4, 2022

Conversation

nitay
Copy link
Contributor

@nitay nitay commented Mar 3, 2022

Currently loading a Coursier plugin is not possible, because the ClassLoaders that are used by Mill's Coursier cannot be changed. I ran into this when trying to use my plugin which adds support for a new protocol. Loading via $ivy. or anything else doesn't work because Mill uses the default Coursier cache

This PR adds a hook to allow altering the FileCache used. With this, I can put the following in my build module:

  override def cacheCustomizer: Task[Option[FileCache[coursier.util.Task] => FileCache[coursier.util.Task]]] = T.task {
    Some( (fc: FileCache[coursier.util.Task]) => {
      fc.withClassLoaders(Seq(classOf[coursier.cache.protocol.S3Handler].getClassLoader)) }
    )
  }

Which then fixes the resolution so that when it looks for plugins it can find my class.

This PR borrows ideas from #775

If there's better way to achieve this lmk

@nitay nitay changed the title Support customizing Coursier file cache Support Coursier plugins via customizing FileCache used (with classloaders) Mar 3, 2022
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Thanks @nitay for this contribition! I think, this is another motivation for

As this fixes a real world scenario, I think it is worth to have this feature in Mill now. Unfortunately, your change results in binary incompatible API changes. Before we merge it, a bit more work is needed. We need to provide the previous API, mark it @deprecated and delegate to the new one. Good @deprecated messages are key.

The term "cache" is pretty general and there is a chance a user, who sees the cacheCustomizer task, associates it with "Mill cache" or "target cache" or "download cache". I propose to rename it to coursierCacheCustomizer.

@nitay
Copy link
Contributor Author

nitay commented Mar 3, 2022

@lefou sg will change the name and fix the API breaking failures

agreed it'd be nice to redo how coursier is used, centralize it and make it easy pluggable. not sure if it makes sense but perhaps enabling some import $coursier.plugin-foo type of thing?

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Almost there. We also need to keep the order of the parameters of newly deprecated methods, which had default parameters, to keep to encoding of default params in scala working. Meaning: new params need to come last. See below.

main/src/mill/modules/Jvm.scala Outdated Show resolved Hide resolved
scalalib/src/Lib.scala Outdated Show resolved Hide resolved
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you!

@lefou lefou merged commit 3d160c0 into com-lihaoyi:main Mar 4, 2022
@lefou lefou added this to the after 0.10.0 milestone Mar 4, 2022
@nitay
Copy link
Contributor Author

nitay commented Mar 4, 2022

thx @lefou - how do I find a build with this that I can try using with .mill-version?

@nitay nitay deleted the classloaders branch March 4, 2022 16:03
@lefou
Copy link
Member

lefou commented Mar 5, 2022

@nitay Our release pipeline is currently broken. Once, we fixed it, I'll release a 0.10.1.

@lefou
Copy link
Member

lefou commented Mar 7, 2022

Latest snapshot release made it. 🥳 You can try one of the latest releases now, e.g. 0.10.0-82-30f1bf.

@alexarchambault
Copy link
Contributor

alexarchambault commented Sep 12, 2024

Coming back at this PR 2+ years later. I'm seeing that it's the one that added a call to noCredentials on FileCache. Does anyone recall if there was there a specific motivation for that? Or was it… something tried while debugging, and that made it anyway, say?

@alexarchambault
Copy link
Contributor

I would blame that for #3408

@lefou
Copy link
Member

lefou commented Sep 12, 2024

@alexarchambault Can't tell. It probably slipped through my radar, since the term credentials isn't anywhere in this changeset.

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