Add withAddedResolvers to DependencyResolutionInterface #427
Conversation
| resolvers: Seq[Resolver], | ||
| log: Logger | ||
| ): IvyDependencyResolution = { | ||
| ivySbt.withIvy(log) { ivy => |
There was a problem hiding this comment.
I have the impression here that this is just mutating the underlying ivy settings rather than creating a new copy with the modified ivy settings which is what the intent of the implementation here.
If this is the case how would you solve this properly?
There was a problem hiding this comment.
I guess currently Resolver passing is done somewhat outside of LM API - https://github.com/sbt/sbt/blob/v1.9.4/main/src/main/scala/sbt/Defaults.scala#L4022-L4038. I've been reluctant to give too much support to Resolvers because beyond the basic URL layout for HTTPS there's no consensus among the LM engine implementations on how resolvers are used. For example, someone has implemented Amazon S3 resolver for Ivy, that can't be passed into Coursier.
In general, we don't have LM API for LM engine construction beyond IvyDependencyResolution.apply function, so the best we can do is maybe start there and implement another apply that takes additional resolvers, and also make sure we can implement one for Coursier as well.
There was a problem hiding this comment.
I guess currently Resolver passing is done somewhat outside of LM API - https://github.com/sbt/sbt/blob/v1.9.4/main/src/main/scala/sbt/Defaults.scala#L4022-L4038.
Thanks for the tip, I managed to figure out a more principled way to add the extra resolvers (configuration is either an InlineIvyConfiguration or an ExternalIvyConfiguration both of these implementations provided an easy way to add extra resolvers).
I just pushed a commit with the changes
I've been reluctant to give too much support to Resolvers because beyond the basic URL layout for HTTPS there's no consensus among the LM engine implementations on how resolvers are used. For example, someone has implemented Amazon S3 resolver for Ivy, that can't be passed into Coursier.
I guess I am hitting contention here, which is treating DependencyResolution (and by proxy LM Engine) as a first class citizen vs Ivy Support which I have the view that it should eventually be deprecated but considering your response maybe this is a case of "lets not go there".
My immediate (and most likely non-educated) answer to this would be that we should extend the DependencyResolution interface to cover our use cases (just as is being done in this PR) and going forward if someone wants to, for example implement an S3 resolver (I assume you are talking about things like https://github.com/ohnosequences/ivy-s3-resolver) we would extend sbt.librarymanagement.Resolver with our own AbstractResolver to cover cases like this. Then it would be LM engine's role to turn that extra functionality to either an Ivy AbstractResolver (which is what Ivy needs) and whatever Coursier expects.
But then we circle back to your point which is I think that if we do add withAddedResolvers as this PR suggests, then this ties the DependencyResolution interface to sbt.librarymanagement.Resolver and there currently isn't a way to handle such resolvers for both Ivy/Coursier. I don't really have anything more to add to this, aside from the fact that I don't think there is a nice way to solve your original problem (i.e. using org.apache.ivy.plugins.resolver.AbstractResolver/org.apache.ivy.plugins.resolver.DependencyResolver or w/e the appropriate Ivy type should be instead of sbt.librarymanagement.Resolver for withAddedResolvers is worse in my opinion).
In general, we don't have LM API for LM engine construction beyond
IvyDependencyResolution.applyfunction, so the best we can do is maybe start there and implement anotherapplythat takes additional resolvers, and also make sure we can implement one for Coursier as well.
As said previously, I just updated the PR so the builder pattern works as I originally wanted it to, should I also add the Resolver to apply? Strictly speaking for the problem I am solving this is not needed so my initial inclination is to leave it to a future PR which may be more appropriate given that we are already discussing open design points.
To expand, the whole intent of the PR is for users to add resolvers ontop of the existing list of resolvers (which is going to be computed from sbt settings). Putting added resolvers that in apply would be a bit confusing considering that the apply would typically contain the initial set of resolvers (which is already provided by configuration) or is the only reason for wanting to add the custom resolvers into IvyDependencyResolution.apply is to avoid adding sbt.librarymanagement.Resolver to the generic DependencyResolution interface (as per my previous point)? If so I don't have a big problem with this per say, although I guess the downside is that my PR wouldn't work with coursier (and it would be nice to use coursier since its a lot faster than Ivy).
Finishing off, if you are happy with the direction of the PR let me know and I can write some tests.
There was a problem hiding this comment.
@eed3si9n Now that sbt 2.0 is going forwards, what are your thoughts on this?
There was a problem hiding this comment.
Also pinging @jtjeferreira since I think you seem to be a maintainer of this project now
There was a problem hiding this comment.
I am out of depth here, but maybe it would be worth to do this when sbt/sbt#7739 is finished, so we can do all the necessary changes in a single PR...
There was a problem hiding this comment.
I am out of depth here, but maybe it would be worth to do this when sbt/sbt#7739 is finished, so we can do all the necessary changes in a single PR...
Sure!
@adpi2 Maybe you want to chime in here as well
There was a problem hiding this comment.
I have to think a bit more on the technical details of the proposal, but one thing I can tell you is that we've removed useCoursier flag from sbt 2.x, which means that out-of-box everyone would be assumed to use Coursier. Maybe someone can write a plugin to revive Ivy as LM engine, but I'm ever more determined to make Coursier the default.
So if we're thinking about changing the LM API, my primary concern would be how it would align with the semantics of Coursier impl.
There was a problem hiding this comment.
Regarding making coursier a default, that is definitely a welcome change and I fully support it. If thats the case then actually the concept of this should be trivialized no, we just need to add the equivalent implementation methods to coursier integration in sbt and then it should work? Not sure what you had in mind regarding technical proposal, but I remember doing this locally (i.e. also modifying sbt's internal bridge to coursier for DependencyResolution) and it worked as expected. iirc when I implemented this, Coursier's resolvers was just an immutable collection and add another while returning a new instance of DependencyResolution worked as expected.
Judging from sbt/sbt#7753, it appears that librarymanagement is getting inlined into sbt proper rather than its own github repo/module, if thats the case then it should be quite easy to implement this as we don't have a chicken and egg problem.
|
@eed3si9n Do you have an opinion on this? |
mdedetrich
force-pushed
the
add-withaddedresolvers-to-dependency-resolution-interface
branch
from
cb38ccb
to
a8ce9fa
Compare
| case configuration: ExternalIvyConfiguration => | ||
| configuration.withExtraResolvers(configuration.extraResolvers ++ resolvers) | ||
| case configuration: InlineIvyConfiguration => | ||
| configuration.withResolvers(configuration.resolvers ++ resolvers) |
There was a problem hiding this comment.
InlineIvyConfiguration also has a otherResolvers field where the resolvers can be added, not sure what the intended difference is meant to be and whether it should be used instead.
adpi2
changed the title
adpi2
changed the title
mdedetrich
force-pushed
the
add-withaddedresolvers-to-dependency-resolution-interface
branch
from
a8ce9fa
to
78a6a00
Compare
|
I have just rebased and updated the PR |
The intent of this PR is to give the ability for a
DependencyResolutionInterfaceto add resolvers. The context behind this change is that there are situations where you have sbt plugins that resolve dependencies but not in the context of an sbt project/config. Examples of this areIn both these cases we have an sbt plugin that resolves dependencies from a resolver but its not because those dependencies are in the context of an sbt project (i.e. compiling your code). In MiMa's case we are resolving jar's to check for binary compatibility issues and in sbt-reproducible-builds case we are resolving jar's to confirm that a local build matches a deployed build.
The issue here is if you want to add resolvers that are only necessary for these plugins then currently this is not possible without hacky workarounds (in MiMa's case you would have to create a new sbt subproject that has these extra resolvers just for MiMa and for sbt-reproducible-builds we ended up resolving jars manually, i.e. using gigahorse to manually download the jars).
So idea here is to provide a mechanism so that such sbt plugins can add resolvers just for those plugins so you don't need to pollute the global sbt
resolverssetting scope.An interesting point is, if this feature is accepted where it should land i.e. in SBT 1.9 or SBT 2.x. Its theoretically possible to have this in SBT 1.9 with the a default implementation for the
def withAddedResolvers(resolvers: Seq[Resolver], log: Logger): DependencyResolutionInterfacemethod and sbt plugins responsible for knowing whether the method exists (or not). The problem here is thatDependencyResolutionInterfaceis open and hence coursier also provides an implementation of it so there may be an argument that such a change should land in SBT 2.x.