mill icon indicating copy to clipboard operation
mill copied to clipboard

Make build.sc dependency resolution independent of Mill assembly

Open lihaoyi opened this issue 1 year ago • 8 comments

Should fix https://github.com/com-lihaoyi/mill/issues/2985 and https://github.com/com-lihaoyi/mill/issues/2703, at least as a first pass.

  • I did not change the actual classpath contents to try and minimize it; that can come in a follow up PR
  • I did not try to cache or bundle the dependencies locally; that can also come in a follow up

At least with this change, all the Mill build.sc dependencies go through the same code path (Coursier resolution) and behave similarly (e.g. you should be able to update upickle via import $ivy. Further improvements to hygiene or performance can come in follow ups (e.g. resolving dependencies bundled in the assembly)

Notes:

  • Changed MillBuildRootModule#ivyDeps to mandatoryIvyDeps to prevent accidental overriding (not sure why this didn't cause problems before??? Maybe people just didn't override ivyDeps much in the meta-build)

  • Removed resolveDepsExclusions, since now we are no longer hard-coding a dependency on the Mill assembly, we also no longer need to treat these dependencies specially and they can be upgraded or evicted as the user sees fit

  • Removed Lib.resolveMillBuildDeps and its use sites, since now the build dependencies go through the same code path as any other ivyDeps

Tested the mill.idea.GenIdea/idea workflow manually in example/basic/1-simple-scala and example/misc/4-mill-build-folder manually, the intellij jump-to-definition in build.sc and mill-build/build.sc seem to work

lihaoyi avatar Jul 13 '24 15:07 lihaoyi

Added a test, which fails, demonstrating that the current code changes are not enough

The issue as of now is that even without the mill jar added to the classpath as part of MillBuildRootModule, it still ends up on the classpath as part of the classloader hierarchy when we spawn sub-classloaders to evaluate each build.sc file.

Removing that connection to make each spawned classloader independent of the parent will require some more work, because as of now the sub-classloaders and parent classloader share a the RootModule/Module/Task interfaces along with everything they depend on (os.Path, mill.define.Discover, upickle, etc.). This will need to be narrowed down significantly to make it possible to make the spawned classloaders independent, and thus the build.sc classpath independent of the Mill binary

lihaoyi avatar Jul 17 '24 01:07 lihaoyi

Maybe, we can implement this specific test as an server integration test, which should already run in a spawned, thus isolated, Java process?

lefou avatar Jul 17 '24 08:07 lefou

In this case its not the added test that is a problem, but the festure itself. It seems like it would be a bunch more work to actually isolate the build.sc classpath

lihaoyi avatar Jul 17 '24 09:07 lihaoyi

Even if we don't support bumping of "provided" dependencies, this PR is still the right change, since it makes dependency resolution more consistent and allows IDEs to easily pull sources.

@lihaoyi Why did you mark this PR with the compat-breaker label? Shouldn't this change only be implementation details and transparent to the user?

lefou avatar Jul 17 '24 15:07 lefou

The issue with using this PR as is is that because the dependencies may have different versions, your IDE may pull the wrong sources (you'd get the resolved version sources rsther than the bundled version sources)

As I imagined it, if we fully supported resolving Mill's own dependencies via coursier rather than using the bundled versions, people import $ivy stuff may end up with arbitrarily different versions of stuff on the classpath vs what they have now depending on how dependency resolution goes

lihaoyi avatar Jul 17 '24 20:07 lihaoyi

All classloaders I know repect the order of the entries, so we could try to put our "provided" dependencies first on the classpath. Another option would be to use version ranges or forced versions like [0.10.2] in the ivyDeps. That way, we would at least detect (and fail) when the versions have a conflict. Unfortunately, the old coursier API we're using doesn't properly report mismatches, and my attempts to replace it with the newer API have other issues (see https://github.com/com-lihaoyi/mill/pull/3185).

lefou avatar Jul 17 '24 21:07 lefou

The issue here is that the build.sc classloader has to share classes with the Mill binary classloader, because they interact via Scala class instances. They have to be using the same classes

So although setting parent = null to fully isolate them is trivial, we would then also need to completely rewrite the interactions to be through thin interfaces, rather than the fat Module/RootModule/Task classes we have today.

It's possible, but would take more work, and is not as simple as just changing the classpath on the MillBuildRootModule

Forcing the ivy resolution to match the provided versions could help land this without the more aggressive changes, just as a simplification without actually allowing those deps to be upgraded. Does coursier allow such a thing?

lihaoyi avatar Jul 17 '24 21:07 lihaoyi

If you use forced versions (Maven term, I think, like [0.11.8]) or intervals ([0.11.8,0.11.8]) to pin a version, coursier will fail, if transitive selected versions don't match that range.

Documentation: https://get-coursier.io/docs/other-version-selection

Example: https://github.com/com-lihaoyi/mill/issues/3075#issuecomment-1998525943

But it does not work in our current implemntation, due to Mill using the "low level" coursier API.

In PR https://github.com/com-lihaoyi/mill/pull/3185 I therefore replaced that API with the "high level" API, but I struggle to find a proper way to forward mapped dependencies to that API.

lefou avatar Jul 17 '24 22:07 lefou