Resolve versions using aether, and uncouple existing Resolver from code base.

[petroseskinder]

Context

This is a part of an ongoing effort to isolate and then, replace the existing Resolver with Aether.

Alongside actually resolving artifacts, the existing Resolver also: (1) builds Artifacts from coordinates, (2) parses versions from version specifications and decides on a version to use.

Both are orthogonal to the role of the Resolver, and make the Resolver unnecessarily coupled to various classes. This PR isolates (and also corrects) this functionality. Note: as this PR is not intended to lead to regressions, it should be merged to master.

Detailed Design

Building Artifacts Currently, artifacts are built using Resolver.getArtifact(). The Resolver also defines a class of exceptions for when this fails. These are both orthogonal to the role of the Resolver. However, since the function/exception is called from the DefaultModelResolver and the GenerateWorkspaceOptions, it unnecessarily couples the Resolver to these two classes. I have removed these and placed them in the ArtifactBuilder.

Version Resolution Version resolution selects a version given a maven coordinate and a version specification. This is also called from the DefaultModelResolver and GenerateWorkspaceOptions.

I have also elected to separate that and also provide a corrected implementation. The current implementation of resolving a version from a version spec is adhoc and idiosyncratic. This CL isolates this functionality, and provides a canonical/correct version resolution mechanism.

Maven's version resolution has two components: (1) it selects the pinned version from both hard and soft pins, e.g. "[3.0]" and "3.0" should evaluate to "3.0" (2) it selects the highest available version for version ranges e.g. "[3.0, )" should select 3.9 if its highest available

This functionality is easily implemented through aether using a version range request.

Unit Tests Tests are implemented for when there is invalid behavior, as well as for pinned version ranges, i.e. "3.2". However, due to limitations of Mockito and Aether (mockito cannot mock final classes), tests are not included for correct behavior of version ranges. In particular, there are no tests for ensuring that the highest version is selected.

Reviewer: @kchodorow Related Issues #16

[bazel-io]

Can one of the admins verify this patch?

[googlebot]

So there's good news and bad news.

:thumbsup: The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

:confused: The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[googlebot]

CLAs look good, thanks!

[kchodorow]

Jenkins, test this please.

[kchodorow]

I think it does merge it with HEAD.

On Thu, Jul 27, 2017 at 3:02 PM, Petros Eskinder notifications@github.com wrote:

I am unsure how to interpret the Jenkins results. All tests pass locally. Moreover, the specific failure is also peculiar to me. It states that it fails the nonConflictingDepManagementRange. However, I haven't merged the changes from your dependency management PR. Unless, of course, it's merging these and then running the tests.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/migration-tooling/pull/45#issuecomment-318456288, or mute the thread https://github.com/notifications/unsubscribe-auth/AABCkgl16TKXioUBxgVUPpduze7WgSiYks5sSN6pgaJpZM4OcHZ0 .

[petroseskinder]

Jenkins, test this please.

[petroseskinder]

I resolved the issue. I removed the Resolver.resolveVersion function from the codebase and made sure all version resolution was conducted by the VersionResolver I introduce here. Also used it to regenerate the generate_workspace.bzl file and things build with no issue.

I also resolved the issue with the soft pins. If you want I can add the commit to this PR or another one. The relevant changes can be found here. It was a simple enough solution. Transform "3.0" to "[3.0, )" and return the first element in the list of potential versions.

[petroseskinder]

@kchodorow can you retract the approval, and rereview? I decided on simply including the soft pin fix in this pull request. Since the change was simple and very closely related to this, I felt it was best to keep it in the same PR. Here is the relevant commit message:

Resolve soft pinned version to nearest version if it does not exist.

Currently, even if a soft pinned version, e.g. "3.0", does not exist aether returns that soft pinned version. Instead, it should provide another valid version. This change solves this issue by transforming the soft pin into a version range, and making the aether request using that version range.

For example, instead of requesting for "3.0", we transform it to "[3.0,)" providing us all versions between 3.0 and the latest version. If the version exists, we simply take it. If it doesn't, then we select the next closest valid version.

If you prefer I do this in a separate pull request, I can also do that.

[googlebot]

So there's good news and bad news.

:thumbsup: The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

:confused: The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[googlebot]

CLAs look good, thanks!

[kchodorow]

Interesting, I guess Jenkins doesn't see the code review comment?

Test this please.