Deprecate resolveFilter stuff #4162
Conversation
So that we don't need to resort to "resolveFilter" stuff to keep only their main JAR
|
The CI errors seem unrelated to these changes (I can't reproduce them locally) |
|
Yes we've been seeing flakiness in ci, i'll rerun them |
|
@lihaoyi @lefou Can any of you re-run the Also, I'm about to undraft the 3 other PRs that depend on this one (that actually depend on each other successively). Namely #4145, #4154, and #4155. Each adds something to the previous one. #4145 should be the one with the most important changes: it changes the way Mill interfaces with coursier basically, which impacts many other Mill parts. The PR here should be the most straightforward to review I guess. The other two are what motivated the first two: they consist in BOM-related changes. If you'd like me to… split them further to ease review, or re-open them with a cleaner git history to spot more easily what each adds to the previous ones, just tell me. |
|
@alexarchambault kicked the jobs, also gave you write access to the repo so you can do this yourself as necessary in future |
|
I've been holding off taking a look at these PRs since it looked like they were still in flux; should we look at all of them? Or just this one first? |
As you wish, I'd say. One after the other should be easier (once the first is merged, the second one, etc.), keeping in mind that the first two are not just nice-to-have refactorings / clean-ups, but are motivated by the other two, that they make easier to implement (first one is actually motivated by the second one, itself motivated by the other two). |
There was a problem hiding this comment.
I can't spot a single @deprecated anywhere. Shouldn't we provide some API that is free of deprecated stuff and have a message to an deprecated API user?
The change itself makes sense to me. But due to the informal deprecation, it'S rather hard to spot, where (else) we still rely on the filtering.
I was hesitating about this. Managing deprecations while removing a parameter is more of a burden than when adding one. Basically, in a signature like def resolveDependencies(
repositories: Seq[Repository],
deps: IterableOnce[Dependency],
force: IterableOnce[Dependency],
sources: Boolean = false,
mapDependencies: Option[Dependency => Dependency] = None,
customizer: Option[Resolution => Resolution] = None,
ctx: Option[mill.api.Ctx.Log] = None,
coursierCacheCustomizer: Option[FileCache[Task] => FileCache[Task]] = None,
resolveFilter: os.Path => Boolean = _ => true,
artifactTypes: Option[Set[Type]] = None,
resolutionParams: ResolutionParams = ResolutionParams()
): Result[Agg[PathRef]]we'd like to deprecate We can always remove the parameter, and add a shim of the former method with no default params for bin compat, but that means current users of the parameter are very likely to experience compilation errors when bumping Mill (if they rely on a default parameter), while deprecations usually only create a warning. That said, I'm not too opiniated about doing this or what's currently in the PR (renaming of the parameter), it seems both have caveats. |
|
I see. You mean, the synthetic providers for the default values may change in an binary incompatible way. It's a real pita, that this is still an issue in Scala. |
|
But if the |
|
I've been trying to get |
To my understanding, |
|
ah yes, it only helps for adding parameters |
|
Just added |
| def compileModuleDeps = Seq( | ||
| build.main.api, | ||
| scoverage.api | ||
| ) | ||
| def compileIvyDeps = Task { | ||
| Agg( | ||
| super.mandatoryIvyDeps() ++ Agg( | ||
| // compile-time only, need to provide the correct scoverage version at runtime | ||
| build.Deps.scalacScoverage2Plugin, | ||
| build.Deps.scalacScoverage2Reporter, | ||
| build.Deps.scalacScoverage2Domain, | ||
| build.Deps.scalacScoverage2Serializer | ||
| ) | ||
| } | ||
| def mandatoryIvyDeps = Agg.empty[Dep] |
There was a problem hiding this comment.
What's going on with these changes? Did the semantics of the underlying resolution change in some way that we need to change this downstream code?
This PR makes Mill stop relying on the
resolveFilterparameter ofCoursierSupport#resolveDependencies.This parameter manually filters the artifacts provided by coursier, but not the ones originating from "local dependencies" (Mill's hack to use freshly built workers and plugins during tests), where it seems such a filtering was done in the build (by customizing
testDepPaths). "Local dependencies" cannot be kept as-is in #4145 (where such dependencies aren't necessarily root dependencies anymore, making it harder to handle them in a different fashion). Hence the need to stop usingresolveFilter.Rather than filtering artifacts after dependency resolution, this PR marks unwanted dependencies of workers as compile-time only. That way, they're not pulled by dependency resolution, and we don't need to manually filter artifacts.