New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Ivy 2.3.0-final. #1040

Merged
merged 2 commits into from Dec 13, 2013

Conversation

Projects
None yet
4 participants
@willb
Contributor

willb commented Dec 13, 2013

This entailed modifying ResolutionCache and the CustomPomParser
to reflect changes to the ResolutionCacheManager interface and
DefaultExtendsDescriptor class between Ivy 2.3.0-rc1 and
2.3.0-rc2. Specifically,

  1. ResolutionCacheManager now includes two additional methods
    that needed implementations in ResolutionCache:
    getResolvedModuleDescriptor(mrid: ModuleRevisionId) and
    saveResolvedModuleDescriptor(md: ModuleDescriptor). I adapted
    the implementations for these (which are expressed primarily in
    terms of other interface methods) from Ivy 2.3.0's
    DefaultResolutionCacheManager.
  2. Instead of taking a ModuleRevisionIdentifier and a resolved
    ModuleRevisionIdentifier as its first two arguments, the
    DefaultExtendsDescriptor constructor now takes a
    ModuleDescriptor.

Note that ResolutionCache.getResolvedModuleDescriptor does not
appear to be used by Ivy as sbt uses Ivy and there is thus no
test coverage for its implementation. Also note that the
DefaultResolutionCacheManager object created in
Update.configureResolutionCache now requires a reference to an
IvySettings object; DRCM expects this to be non-null.

Support Ivy 2.3.0-final.
This entailed modifying ResolutionCache and the CustomPomParser
to reflect changes to the ResolutionCacheManager interface and
DefaultExtendsDescriptor class between Ivy 2.3.0-rc1 and
2.3.0-rc2. Specifically,

1. ResolutionCacheManager now includes two additional methods
that needed implementations in ResolutionCache:
getResolvedModuleDescriptor(mrid: ModuleRevisionId) and
saveResolvedModuleDescriptor(md: ModuleDescriptor). I adapted
the implementations for these (which are expressed primarily in
terms of other interface methods) from Ivy 2.3.0's
DefaultResolutionCacheManager.

2. Instead of taking a ModuleRevisionIdentifier and a resolved
ModuleRevisionIdentifier as its first two arguments, the
DefaultExtendsDescriptor constructor now takes a
ModuleDescriptor. This was a trivial change.

Note that ResolutionCache.getResolvedModuleDescriptor does not
appear to be used by Ivy as sbt uses Ivy and there is thus no
test coverage for its implementation. Also note that the
DefaultResolutionCacheManager object created in
Update.configureResolutionCache now requires a reference to an
IvySettings object; DRCM expects this to be non-null.
@harrah

This comment has been minimized.

Show comment
Hide comment
@harrah

harrah Dec 13, 2013

Member

LGTM, thank you!

I had previously thought this should go into 0.14 due to the risk of binary incompatible changes. It looks like the sbt side is ok, but I don't know about Ivy itself. Problems would occur if a) Ivy has some incompatibilities and b) a plugin uses Ivy directly and is affected by them.

@jsuereth @eed3si9n (and anyone watching #847) any thoughts?

As a follow up commit, perhaps do a similar doc refresh as was done in xuwei-k@d5d4d83 ?

Member

harrah commented Dec 13, 2013

LGTM, thank you!

I had previously thought this should go into 0.14 due to the risk of binary incompatible changes. It looks like the sbt side is ok, but I don't know about Ivy itself. Problems would occur if a) Ivy has some incompatibilities and b) a plugin uses Ivy directly and is affected by them.

@jsuereth @eed3si9n (and anyone watching #847) any thoughts?

As a follow up commit, perhaps do a similar doc refresh as was done in xuwei-k@d5d4d83 ?

@jsuereth

This comment has been minimized.

Show comment
Hide comment
@jsuereth

jsuereth Dec 13, 2013

Member

Wow nice! I think we may need to push a milestone of 0.13.2 with this and get teh community/plugins to experiment and see what happens....

Still, Really looking forward to reviewing this.

Member

jsuereth commented Dec 13, 2013

Wow nice! I think we may need to push a milestone of 0.13.2 with this and get teh community/plugins to experiment and see what happens....

Still, Really looking forward to reviewing this.

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Dec 13, 2013

Member

There's no signature changes to public classes or methods, right?
I can't speak for all the plugins, but I personally can't think of any situation I had to drive Ivy directly.

A minor point but the code seems to be using whitespace as opposed to tab. (I wish this was automated)

Member

eed3si9n commented Dec 13, 2013

There's no signature changes to public classes or methods, right?
I can't speak for all the plugins, but I personally can't think of any situation I had to drive Ivy directly.

A minor point but the code seems to be using whitespace as opposed to tab. (I wish this was automated)

@jsuereth

This comment has been minimized.

Show comment
Hide comment
@jsuereth

jsuereth Dec 13, 2013

Member

There's only one using internal Ivy I know of - sbt-license-report. IT's fine to break it though, I own that one.

Member

jsuereth commented Dec 13, 2013

There's only one using internal Ivy I know of - sbt-license-report. IT's fine to break it though, I own that one.

@harrah

This comment has been minimized.

Show comment
Hide comment
@harrah

harrah Dec 13, 2013

Member

@eed3si9n No signature changes to sbt side. I don't know about Ivy.

@jsuereth That means that sbt-license-report users have to know to bump the sbt-license-report version if they go to 0.13.2, though.

Member

harrah commented Dec 13, 2013

@eed3si9n No signature changes to sbt side. I don't know about Ivy.

@jsuereth That means that sbt-license-report users have to know to bump the sbt-license-report version if they go to 0.13.2, though.

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Dec 13, 2013

Member

There's like one project using sbt-license-report on github, and it's activator. Not saying it's not widely popular among the closed source community, but still.

Member

eed3si9n commented Dec 13, 2013

There's like one project using sbt-license-report on github, and it's activator. Not saying it's not widely popular among the closed source community, but still.

@harrah

This comment has been minimized.

Show comment
Hide comment
@harrah

harrah Dec 13, 2013

Member

@eed3si9n Compatibility changes need to take into account plugins that we may not know about. In this case it is indeed @jsuereth's plugin used at Typesafe, but that actually makes it less problematic as he indicates. I don't have a strong opinion either way, but if there are real plugins using Ivy directly that would be affected, we should consider them. From the diff, it looks like a rather central class in Ivy was changed. However, perhaps no one constructs it and they only consume it.

I agree we should publish a milestone and get people to try it out. Plugins may use Ivy, but it may be that they aren't generally affected.

Member

harrah commented Dec 13, 2013

@eed3si9n Compatibility changes need to take into account plugins that we may not know about. In this case it is indeed @jsuereth's plugin used at Typesafe, but that actually makes it less problematic as he indicates. I don't have a strong opinion either way, but if there are real plugins using Ivy directly that would be affected, we should consider them. From the diff, it looks like a rather central class in Ivy was changed. However, perhaps no one constructs it and they only consume it.

I agree we should publish a milestone and get people to try it out. Plugins may use Ivy, but it may be that they aren't generally affected.

@harrah

This comment has been minimized.

Show comment
Hide comment
@harrah

harrah Dec 13, 2013

Member

So, I guess that means I'll merge to the 0.13 branch.

Member

harrah commented Dec 13, 2013

So, I guess that means I'll merge to the 0.13 branch.

harrah added a commit that referenced this pull request Dec 13, 2013

@harrah harrah merged commit 70b6f2d into sbt:0.13.1 Dec 13, 2013

harrah added a commit that referenced this pull request Dec 13, 2013

Revert "Merge pull request #1040 from willb/FEATURE-ivy-2.3.0-final"
This reverts commit 70b6f2d, reversing
changes made to 7fefdb7.

Wrong branch: should have gone on 0.13
@harrah

This comment has been minimized.

Show comment
Hide comment
@harrah

harrah Dec 13, 2013

Member

Sorry, I didn't realize this targeted 0.13.1, which has been released as final. Can you target the 0.13 branch instead?

Member

harrah commented Dec 13, 2013

Sorry, I didn't realize this targeted 0.13.1, which has been released as final. Can you target the 0.13 branch instead?

@willb

This comment has been minimized.

Show comment
Hide comment
@willb

willb Dec 13, 2013

Contributor

Sorry about that. I've rebased my patches atop 0.13 and force-updated my branch. I re-ran the test suite as a sanity check and everything still passes.

Contributor

willb commented Dec 13, 2013

Sorry about that. I've rebased my patches atop 0.13 and force-updated my branch. I re-ran the test suite as a sanity check and everything still passes.

@harrah

This comment has been minimized.

Show comment
Hide comment
@harrah

harrah Dec 13, 2013

Member

No problem, thanks for the quick response. Merged to 0.13 in 5df24e5.

Member

harrah commented Dec 13, 2013

No problem, thanks for the quick response. Merged to 0.13 in 5df24e5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment