Ivy - PR-57 need inputs

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Ivy - PR-57 need inputs

Jaikiran Pai
This PR - https://github.com/apache/ant-ivy/pull/57 does changes related
to generics usage. I reviewed it a while back and it looks fine overall
except for one change, for which I need inputs from the rest of the team.

Ivy has a DependencyResolver interface which is the central piece of
contract/interface for extending Ivy (any external usage for that
matter). This PR introduces a new method on this interface[1]. I
understand why that method (in favour of the other one that is marked as
deprecated in that same PR) makes sense. Had this been some other
relatively lesser exposed interface or had this been a change related to
introducing a new feature, I think I probably wouldn't have been worried
about it. However, given the nature of this interface, I think we
shouldn't add this method just yet in this release. Instead, what I
think we could do is add that method to the implementing class(es)
internally (like the AbstractResolver - the PR does that already). Of
course at some places within our code, if we want to use the newer
generics based method, we will probably end up doing a type check on the
resolver instance to see if it's a AbstractResolver which has that new
method, but I think that should be fine for now.

I would like some inputs on how we should go about this.

[1]
https://github.com/apache/ant-ivy/pull/57/files#diff-8078d19e36e6c0b7abb0690fca45db0fR191

-Jaikiran


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Ivy - PR-57 need inputs

Gintautas Grigelionis
Please see my comment on GitHub. It is a change of signature (from array to
collection) in order to change the return type (from array to collection,
because arrays of generics do not work), and it's based on refactoring of
AbstractOSGiResolver. It is a crucial piece to getting the generics right,
and leaving it out would mean leaving all other changes in resolvers and
SearchEngine out.

Gintas

2017-07-28 17:19 GMT+02:00 Jaikiran Pai <[hidden email]>:

> This PR - https://github.com/apache/ant-ivy/pull/57 does changes related
> to generics usage. I reviewed it a while back and it looks fine overall
> except for one change, for which I need inputs from the rest of the team.
>
> Ivy has a DependencyResolver interface which is the central piece of
> contract/interface for extending Ivy (any external usage for that matter).
> This PR introduces a new method on this interface[1]. I understand why that
> method (in favour of the other one that is marked as deprecated in that
> same PR) makes sense. Had this been some other relatively lesser exposed
> interface or had this been a change related to introducing a new feature, I
> think I probably wouldn't have been worried about it. However, given the
> nature of this interface, I think we shouldn't add this method just yet in
> this release. Instead, what I think we could do is add that method to the
> implementing class(es) internally (like the AbstractResolver - the PR does
> that already). Of course at some places within our code, if we want to use
> the newer generics based method, we will probably end up doing a type check
> on the resolver instance to see if it's a AbstractResolver which has that
> new method, but I think that should be fine for now.
>
> I would like some inputs on how we should go about this.
>
> [1] https://github.com/apache/ant-ivy/pull/57/files#diff-8078d19
> e36e6c0b7abb0690fca45db0fR191
>
> -Jaikiran
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Ivy - PR-57 need inputs

Stefan Bodewig
In reply to this post by Jaikiran Pai
On 2017-07-28, Jaikiran Pai wrote:

> Ivy has a DependencyResolver interface which is the central piece of
> contract/interface for extending Ivy (any external usage for that
> matter).

I'm not at all familiar with Ivy and the eco system that might exist
around it. How likely is it that anybody has implemented this interface
in a an extension? And how likely does such an extension not subclass
AbstractResolver?

Adding methods to an interface usually means a backwards incompatible
change that requires a new major version when using semantic versioning
(not sure whether Ivy uses semver, Ant itself doesn't).

> Instead, what I think we could do is add that method to the
> implementing class(es) internally (like the AbstractResolver - the PR
> does that already). Of course at some places within our code, if we
> want to use the newer generics based method, we will probably end up
> doing a type check on the resolver instance to see if it's a
> AbstractResolver which has that new method, but I think that should be
> fine for now.

Alternatively add a new interface with that method that extends
DependencyResolver and is implemented by AbstractResolver and check for
the new interface rather than AbstractResolver?

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Ivy - PR-57 need inputs

Gintautas Grigelionis
It's not a new method, it's a change of signature. The old method is still
there. If an implementation disregards the abstract class, which contains
the replacement, it would still work, right?

Gintas

2017-07-28 18:55 GMT+02:00 Stefan Bodewig <[hidden email]>:

> On 2017-07-28, Jaikiran Pai wrote:
>
> > Ivy has a DependencyResolver interface which is the central piece of
> > contract/interface for extending Ivy (any external usage for that
> > matter).
>
> I'm not at all familiar with Ivy and the eco system that might exist
> around it. How likely is it that anybody has implemented this interface
> in a an extension? And how likely does such an extension not subclass
> AbstractResolver?
>
> Adding methods to an interface usually means a backwards incompatible
> change that requires a new major version when using semantic versioning
> (not sure whether Ivy uses semver, Ant itself doesn't).
>
> > Instead, what I think we could do is add that method to the
> > implementing class(es) internally (like the AbstractResolver - the PR
> > does that already). Of course at some places within our code, if we
> > want to use the newer generics based method, we will probably end up
> > doing a type check on the resolver instance to see if it's a
> > AbstractResolver which has that new method, but I think that should be
> > fine for now.
>
> Alternatively add a new interface with that method that extends
> DependencyResolver and is implemented by AbstractResolver and check for
> the new interface rather than AbstractResolver?
>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Ivy - PR-57 need inputs

Gintautas Grigelionis
Here's a japicmp report on binary incompatibilities between Ivy 2.4.0 and
the snapshot build from the PR branch (which is rebased on latest master):
none of them break binary compatibility. Would anybody comment the rest of
incompatibilities? I am guilty of breaking ModuleRules, but that was for
the same reason: arrays of generics do not work in Java.

Gintas

***! MODIFIED CLASS: PUBLIC org.apache.ivy.core.module.id.ModuleRules  (not
serializable)
    ***! MODIFIED METHOD: PUBLIC java.util.List (<-java.lang.Object[])
getRules(org.apache.ivy.core.module.id.ModuleId)
    ***! MODIFIED METHOD: PUBLIC java.util.List (<-java.lang.Object[])
getRules(org.apache.ivy.core.module.id.ModuleRevisionId,
org.apache.ivy.util.filter.Filter)
***! MODIFIED CLASS: PUBLIC
org.apache.ivy.osgi.updatesite.UpdateSiteLoader  (not serializable)
    ---! REMOVED CONSTRUCTOR: PUBLIC(-)
UpdateSiteLoader(org.apache.ivy.core.cache.RepositoryCacheManager,
org.apache.ivy.core.event.EventManager,
org.apache.ivy.core.cache.CacheResourceOptions)
***! MODIFIED CLASS: PUBLIC FINAL
org.apache.ivy.plugins.circular.CircularDependencyHelper  (not serializable)
    ---! REMOVED METHOD: PUBLIC(-) STATIC(-)
org.apache.ivy.core.module.id.ModuleRevisionId[]
toMrids(org.apache.ivy.core.module.descriptor.ModuleDescriptor[])
***! MODIFIED CLASS: PUBLIC FINAL org.apache.ivy.util.FileUtil  (not
serializable)
    ---! REMOVED METHOD: PUBLIC(-) STATIC(-) void copy(java.net.URL,
java.io.File, org.apache.ivy.util.CopyProgressListener)
        ---  REMOVED EXCEPTION: java.io.IOException
    ---! REMOVED METHOD: PUBLIC(-) STATIC(-) void copy(java.io.File,
java.net.URL, org.apache.ivy.util.CopyProgressListener)
        ---  REMOVED EXCEPTION: java.io.IOException
    ---! REMOVED METHOD: PUBLIC(-) STATIC(-) void symlink(java.io.File,
java.io.File, org.apache.ivy.util.CopyProgressListener, boolean)
        ---  REMOVED EXCEPTION: java.io.IOException
    ---! REMOVED METHOD: PUBLIC(-) STATIC(-) void
symlinkInMass(java.util.Map, boolean)
        ---  REMOVED EXCEPTION: java.io.IOException
***! MODIFIED CLASS: PACKAGE_PROTECTED (<- PUBLIC)
org.apache.ivy.util.url.HttpClientHandler  (not serializable)
    ***! MODIFIED CONSTRUCTOR: PACKAGE_PROTECTED (<- PUBLIC)
HttpClientHandler()
    ---! REMOVED METHOD: PUBLIC(-) int getHttpClientMajorVersion()
---! REMOVED INTERFACE: PUBLIC(-) ABSTRACT(-) STATIC(-)
org.apache.ivy.util.url.HttpClientHandler$HttpClientHelper  (not
serializable)
    ---! REMOVED METHOD: PUBLIC(-) ABSTRACT(-) int
getHttpClientMajorVersion()
    ---! REMOVED METHOD: PUBLIC(-) ABSTRACT(-) long
getResponseContentLength(org.apache.commons.httpclient.HttpMethodBase)



2017-07-28 20:34 GMT+02:00 Gintautas Grigelionis <[hidden email]>:

> It's not a new method, it's a change of signature. The old method is still
> there. If an implementation disregards the abstract class, which contains
> the replacement, it would still work, right?
>
> Gintas
>
> 2017-07-28 18:55 GMT+02:00 Stefan Bodewig <[hidden email]>:
>
>> On 2017-07-28, Jaikiran Pai wrote:
>>
>> > Ivy has a DependencyResolver interface which is the central piece of
>> > contract/interface for extending Ivy (any external usage for that
>> > matter).
>>
>> I'm not at all familiar with Ivy and the eco system that might exist
>> around it. How likely is it that anybody has implemented this interface
>> in a an extension? And how likely does such an extension not subclass
>> AbstractResolver?
>>
>> Adding methods to an interface usually means a backwards incompatible
>> change that requires a new major version when using semantic versioning
>> (not sure whether Ivy uses semver, Ant itself doesn't).
>>
>> > Instead, what I think we could do is add that method to the
>> > implementing class(es) internally (like the AbstractResolver - the PR
>> > does that already). Of course at some places within our code, if we
>> > want to use the newer generics based method, we will probably end up
>> > doing a type check on the resolver instance to see if it's a
>> > AbstractResolver which has that new method, but I think that should be
>> > fine for now.
>>
>> Alternatively add a new interface with that method that extends
>> DependencyResolver and is implemented by AbstractResolver and check for
>> the new interface rather than AbstractResolver?
>>
>> Stefan
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Ivy - PR-57 need inputs

Gintautas Grigelionis
In reply to this post by Stefan Bodewig
Sorry all, I misunderstood the problem, and thanks to Jaikiran and Stefan
for the discussion.

Third party implementations will be broken because they lack the method
with the new signature unless they extend the AbstractResolver. And the
AbstractResolver must work the other way around, provide a default
implementation that defines the method with new signature in terms of the
method with the old signature. The SearchEngine should work, though,
because the loops are refactored so that they take whatever iterable is
thrown at them. And AFAICS there are no other uses of listTokenValues()
implemented by resolvers (which are possible extensions of Ivy).

Gintas

2017-07-28 18:55 GMT+02:00 Stefan Bodewig <[hidden email]>:

> On 2017-07-28, Jaikiran Pai wrote:
>
> > Ivy has a DependencyResolver interface which is the central piece of
> > contract/interface for extending Ivy (any external usage for that
> > matter).
>
> I'm not at all familiar with Ivy and the eco system that might exist
> around it. How likely is it that anybody has implemented this interface
> in a an extension? And how likely does such an extension not subclass
> AbstractResolver?
>
> Adding methods to an interface usually means a backwards incompatible
> change that requires a new major version when using semantic versioning
> (not sure whether Ivy uses semver, Ant itself doesn't).
>
> > Instead, what I think we could do is add that method to the
> > implementing class(es) internally (like the AbstractResolver - the PR
> > does that already). Of course at some places within our code, if we
> > want to use the newer generics based method, we will probably end up
> > doing a type check on the resolver instance to see if it's a
> > AbstractResolver which has that new method, but I think that should be
> > fine for now.
>
> Alternatively add a new interface with that method that extends
> DependencyResolver and is implemented by AbstractResolver and check for
> the new interface rather than AbstractResolver?
>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Ivy - PR-57 need inputs

Gintautas Grigelionis
Having mistaken binary compatibility for backwards compatibility, I tried
to look at the ecosystem. Sure enough, there is a bunch of custom resolvers
around, but most extend at least RepositoryResolver; here's a list of those
that do it (might be useful for Ivy documentation as well)

https://github.com/fdrueke/ivyp4
https://github.com/massdosage/ivysvn
https://github.com/ohnosequences/ivy-s3-resolver
https://github.com/rajeshja/atg-ivy-resolver
https://github.com/angrycamel/ivydav

There are others that extend URLResover or IBiblioResolver; only SBT
appears to dig deeper, but apparently only because of the lack of proper
typing. In a nutshell, the mitigation I proposed looks adequate (the check
in ChainResolver looks like almost an overkill).

Gintas

2017-07-28 21:26 GMT+02:00 Gintautas Grigelionis <[hidden email]>:

> Sorry all, I misunderstood the problem, and thanks to Jaikiran and Stefan
> for the discussion.
>
> Third party implementations will be broken because they lack the method
> with the new signature unless they extend the AbstractResolver. And the
> AbstractResolver must work the other way around, provide a default
> implementation that defines the method with new signature in terms of the
> method with the old signature. The SearchEngine should work, though,
> because the loops are refactored so that they take whatever iterable is
> thrown at them. And AFAICS there are no other uses of listTokenValues()
> implemented by resolvers (which are possible extensions of Ivy).
>
> Gintas
>
> 2017-07-28 18:55 GMT+02:00 Stefan Bodewig <[hidden email]>:
>
>> On 2017-07-28, Jaikiran Pai wrote:
>>
>> > Ivy has a DependencyResolver interface which is the central piece of
>> > contract/interface for extending Ivy (any external usage for that
>> > matter).
>>
>> I'm not at all familiar with Ivy and the eco system that might exist
>> around it. How likely is it that anybody has implemented this interface
>> in a an extension? And how likely does such an extension not subclass
>> AbstractResolver?
>>
>> Adding methods to an interface usually means a backwards incompatible
>> change that requires a new major version when using semantic versioning
>> (not sure whether Ivy uses semver, Ant itself doesn't).
>>
>> > Instead, what I think we could do is add that method to the
>> > implementing class(es) internally (like the AbstractResolver - the PR
>> > does that already). Of course at some places within our code, if we
>> > want to use the newer generics based method, we will probably end up
>> > doing a type check on the resolver instance to see if it's a
>> > AbstractResolver which has that new method, but I think that should be
>> > fine for now.
>>
>> Alternatively add a new interface with that method that extends
>> DependencyResolver and is implemented by AbstractResolver and check for
>> the new interface rather than AbstractResolver?
>>
>> Stefan
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>
>
Loading...