Break the dependency between repositoriesTask and ivyDeps #4465
Conversation
mill-scalablytyped (but possibly other plugins or user setups too) adds a dependency on `repositoriesTask` from `ivyDeps`. Since the introduction of `JavaModule#coursierProject`, Mill had a dependency the other way around, `repositoriesTask` depends on `ivyDeps`. This creates a cycle, leading to StackOverflowException-s. In order to work around that, this PR splits both `repositoriesTask` and `defaultResolver`, adding: - `allRepositoriesTask`: basically `repositoriesTask`, with the Mill internal repository added - `internalResolver`: same as `defaultResolver`, with the Mill internal repository added (via `allRepositoriesTask`) If users need to resolve purely external modules (most common case it seems), they can keep using `repositoriesTask` or `defaultResolver`. If they need to resolve some Mill internal modules (usually brought in via `JavaModule#coursierDependency`), they now need to use `allRepositoriesTask` and `internalResolver` instead of `repositoriesTask` and `defaultResolver`. That way, no cycle is introduced when users only need to resolve external modules. Fixes com-lihaoyi#4457
| // bin-compat shim | ||
| def resolveDeps( | ||
| deps: Task[Agg[BoundDep]], | ||
| sources: Boolean, | ||
| artifactTypes: Option[Set[Type]] | ||
| ): Task[Agg[PathRef]] = | ||
| resolveDeps( | ||
| deps, | ||
| sources, | ||
| artifactTypes, | ||
| enableMillInternalDependencies = false | ||
| ) | ||
|
|
There was a problem hiding this comment.
Do you meant to open this PR against 0.12.x branch. Otherwise the bin-compat shim isn't needed.
There was a problem hiding this comment.
Do you meant to open this PR against
0.12.xbranch?
I wish, but I couldn't find it
There was a problem hiding this comment.
I added the shim to make the CI happy (seems it requires it), but it might not be needed, yes
There was a problem hiding this comment.
Huh, I restored the branch, but it seems to be removed again. What's going on here?
There was a problem hiding this comment.
Isn't it here? https://github.com/com-lihaoyi/mill/tree/0.12.x
There was a problem hiding this comment.
I restored it again. @lolgab removed it this time. haha.
There was a problem hiding this comment.
I think the big green Merge button removes branches sometimes. I know that's what happened when I removed it on that PR, not sure whether the same thing happened to @lolgab
There was a problem hiding this comment.
I enabled the pr branch delete option in the project settings long time ago, as it is tedious to remove them manually. It's unusual to open a PR from some maintenance branch, that's why it got removed the first time and I restored it, once I recognized it. I think the second removal by @lolgab was in good intent to clean up after a merge. It's easy to overlook that the PR branch name is actually a maintenance branch. Fortunately, the restore feature worked. The 0.12.x branch should now be there again and ready as merge target.
There was a problem hiding this comment.
I opened #4472 against the 0.12.x branch. Waiting for it to be merged before reporting its changes here.
| * | ||
| * @return `CoursierModule.Resolver` instance | ||
| */ | ||
| def internalResolver: Task[CoursierModule.Resolver] = Task.Anon { |
There was a problem hiding this comment.
How about coursierModuleResolver. It's longish, but better tells what it is. Also, it's prefixed with the originating trait name, which make it easy to associate with.
There was a problem hiding this comment.
Or internalModuleResolver, if you want to keep it close to internalRepositories.
There was a problem hiding this comment.
I named it this way as it goes alongside defaultResolver. Maybe defaultResolver could be renamed. The one here is meant to be equivalent, but also handles Mill modules, hence the "internal".
There was a problem hiding this comment.
I'm not sure about coursierModuleResolver, as in Mill, "module" is associated with Mill modules, while the resolver resolves mainly external dependencies. coursierDependencyResolver or just dependencyResolver / depResolver might work too.
There was a problem hiding this comment.
Renamed it to internalCoursierResolver here: #4472
There was a problem hiding this comment.
I'll report it here once the state of #4472 is ok
mill-scalablytyped (but possibly other plugins or user setups too) adds a dependency on
repositoriesTaskfromivyDeps. Since the introduction ofJavaModule#coursierProject, Mill had a dependency the other way around,repositoriesTaskdepends onivyDeps.This creates a cycle, leading to StackOverflowException-s.
In order to work around that, this PR splits both
repositoriesTaskanddefaultResolver, adding:allRepositoriesTask: basicallyrepositoriesTask, with the Mill internal repository addedinternalResolver: same asdefaultResolver, with the Mill internal repository added (viaallRepositoriesTask)If users need to resolve purely external modules (most common case it seems), they can keep using
repositoriesTaskordefaultResolver.If they need to resolve some Mill internal modules (usually brought in via
JavaModule#coursierDependency), they now need to useallRepositoriesTaskandinternalResolverinstead ofrepositoriesTaskanddefaultResolver.That way, no cycle is introduced when users only need to resolve external modules.
Fixes #4457