Add overloads to try and simplify resolveDeps workflows

[lihaoyi]

Still a bit awkward, but less than before.

Semantics should be exactly the same, just moved a bunch of boilerplate into helper overloads.

Pull request: #3330

[lefou]

The issue with these overloads is, that they hide the fact dependencies need to bind to a context. API users now are encouraged to collect dependencies from various contexts (e.g. different modules). This led to various issues in the past, where modules with different contexts (Scala version, platform, ...) were resolved with the wrong (local) context.

By introducing the BoundDep, we made the context explicit and all targets returning dependencies originating in non-local modules return collections of BoundDeps instead of Deps.

Maybe, these overloads should have the word "local" in their names? Although the type alone already has that semantic, its name Dep is way to generic to make that obvious.

[lihaoyi]

Sure, I think resolveLocalDep sounds great.

[lihaoyi]

TBH looking at all the callsites, resolving to "this module" by default definitely seems like the right thing to do. I do think that for the bulk of users, resolving "this module's" dependencies is what they want. And it's true that occasionally people will need to aggregate across multiple modules and feed it through the same context, but that seems like a pretty advanced use case that doesn't happen that often

[lihaoyi]

I have some ideas of how to make this API even better, will update the PR

[lihaoyi]

This needs to be adjusted to throw a proper exception

[lihaoyi]

@lefou took another pass at it. I think the code looks a lot nicer now; no more confusing nested T.task things, and it looks like a normal method call chain.

One downside is the normal method call now needs to throw an exception if it fails, but that's what normal method calls do, and it shouldn't be too hard to wire up try/throw/catch to report a proper error

[lihaoyi]

Using a typeclass here is a bit annoying, but it's necessary to work around the "multiple overloads cant have default params" problem that would otherwise make the user-facing API a lot worse

[lefou]

One downside is the normal method call now needs to throw an exception if it fails, but that's what normal method calls do, and it shouldn't be too hard to wire up try/throw/catch to report a proper error

Couldn't we provide a version returning a Task, so we just add another () and get error handling for free?

[lefou]

One downside is the normal method call now needs to throw an exception if it fails, but that's what normal method calls do, and it shouldn't be too hard to wire up try/throw/catch to report a proper error

Couldn't we provide a version returning a Task, so we just add another () and get error handling for free?

Nevermind. The issue is that the input-parameter(s) are dynamic.

[lefou]

I had some idea in mind for quite a while, that we split the coursier resolution into two steps, resolution and fetching. Lots of meta-operations like finding updates, inspecting licenses, ... only require the resolution without the fetched artifacts. The resolution could also be locked and persisted for later reuse in a more reproducible setup. Since this affects resolveDeps as well, we might think it through as s whole for the next breaking Mill. We lived so long with resolveDeps, it's not time critical to change it now.

[lihaoyi]

Making defaultResolver().resolveDeps(...)() return a task that you call with additional () doesn't really work, because then you cannot pass stuff to the ... that comes from within the enclosing T{...} block. To make that work, we'd need to make resolveDeps itself take a T.task{...}, then we're back to where we started in messiness.

This is pretty fundamental to Mill's multi-stage evaluation model here. defaultResolver().resolveDeps(...) is actually less flexible than the old resolveDeps(T.task{...})(), because we are moving the decision of what gets passed to resolveDeps from resolve-time to evaluation-time. In theory, with the old API you could make decisions affecting the task graph structure based on resolve-time values, while the new API the task graph structure is fixed.

Given all the callsites within the Mill codebase work just fine with the more restricted callsites, I think it's probably the right move to mostly use the newer restricted API, and the older more flexible API is still available for whoever wants it. I'd like to get it in sooner rather than waiting for a complete re-think of the API, since it's a backwards compatible addition that fixes a pretty warty part of the Mill experience, and it doesn't block us from making more drastic cleanups later

[lihaoyi]

Updated a bunch of the examples to take advantage of this, which makes them a lot simpler