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

petroseskinder · 2017-07-19T01:24:40Z

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 · 2017-07-19T01:24:42Z

Can one of the admins verify this patch?

kchodorow · 2017-07-19T18:44:58Z

generate_workspace/src/main/java/com/google/devtools/build/workspace/maven/VersionResolver.java

+    if (isInvalidRangeResult(rangeResult)) {
+      throw new InvalidArtifactCoordinateException(messageForInvalidArtifact(groupId, artifactId, versionSpec));
+    }
+    return rangeResult.getHighestVersion().toString();


I think this should be getRecommendedVersion() (if not null) or getSelectedVersion(artifact).

VersionRangeResult actually does not support either method. [source]

Those methods are associated with org.apache.maven.artifact.versioning.VersionRange. They are independent of aether.

Let me play around with this.

This is still unresolved.

kchodorow · 2017-07-19T18:46:17Z

generate_workspace/src/main/java/com/google/devtools/build/workspace/maven/VersionResolver.java

+    try {
+      rangeResult = repositorySystem.resolveVersionRange(repositorySystemSession, rangeRequest);
+    } catch (VersionRangeResolutionException e) {
+      throw new InvalidArtifactCoordinateException(messageForInvalidArtifact(groupId, artifactId, versionSpec));


Add e.getMessage() to the error.

kchodorow · 2017-07-19T18:56:26Z

generate_workspace/src/main/java/com/google/devtools/build/workspace/maven/VersionResolver.java

+
+    VersionRangeResult rangeResult;
+    try {
+      rangeResult = repositorySystem.resolveVersionRange(repositorySystemSession, rangeRequest);


If I say 3.0, no one conflicts with 3.0, 3.0 exists... we should just use 3.0, not make a request to Maven Central. How about using some of the existing "find a version" code and, if that can't find anything, we do a network request?

If I say 3.0, no one conflicts with 3.0, 3.0 exists...

How do we determine that 3.0 exists without checking the Maven Central?

Suppose there is some artifact a:b:3.12 we want. Now, suppose we create a mirror of maven central. This a:b:3.12 exists within Maven Central. However, there is no a:b:3.12 in the mirror, but there is a a:b:3.13. Using the the above heuristic in lieu of a network request, we would select a:b:3.12. Later, when we try to fetch the artifact, we would find that there is no a:b:3.12, and then skip resolving its transitive dependencies.

This is incorrect behavior. "3.10" is a soft pin, rather than a hard pin. In the situation, there is no 3.10, it should still be able to use 3.11, 3.12, 3.13, etc.

How about using some of the existing "find a version" code and, if that can't find anything, we do a network request?

This is very much a tradeoff between correctness and performance. IMO, until we can match maven/aether one to one, I think we should err on the side of over correctness. We can freely tune things afterwards.

kchodorow · 2017-07-19T19:03:22Z

generate_workspace/src/main/java/com/google/devtools/build/workspace/maven/Resolver.java

@@ -14,13 +14,25 @@

 package com.google.devtools.build.workspace.maven;

+import static com.google.devtools.build.workspace.maven.ArtifactBuilder.InvalidArtifactCoordinateException;
+import static com.google.devtools.build.workspace.maven.ArtifactBuilder.fromMavenDependency;


Don't use static imports for methods unless it's a test.

kchodorow · 2017-07-19T19:09:38Z

generate_workspace/src/main/java/com/google/devtools/build/workspace/maven/VersionResolver.java

+  private final RepositorySystem repositorySystem;
+  private final RepositorySystemSession repositorySystemSession;
+
+  private VersionResolver(RepositorySystem repositorySystem,


Make AetherUtils non-static, pass it in here, and add helper methods when you need to contact aether (aetherUtils.requestVersionRange()). Then you can customize repos & test.

I like this. It's basically a facade around aether. I made the necessary changes. I have also made a few hacks to ensure it works with the Resolver and DefaultMoldelResolver, i.e. defaultResolver

kchodorow · 2017-07-19T19:11:25Z

generate_workspace/src/main/java/com/google/devtools/build/workspace/maven/ArtifactBuilder.java

@@ -0,0 +1,47 @@
+package com.google.devtools.build.workspace.maven;


Nit: add licenses at the tops of new files.

kchodorow · 2017-07-19T19:11:42Z

...rate_workspace/src/main/java/com/google/devtools/build/workspace/maven/ArtifactResolver.java

@@ -1,7 +1,16 @@
 package com.google.devtools.build.workspace.maven;


+import static com.google.devtools.build.workspace.maven.AetherUtils.mavenCentralRepository;


kchodorow

Bah, I always mix up the Aether and Maven versions of classes.

kchodorow · 2017-07-20T15:16:53Z

...e_workspace/src/test/java/com/google/devtools/build/workspace/maven/VersionResolverTest.java

+    try {
+      Mockito.when(system.resolveVersionRange(anySession(), anyRangeRequest()))
+          .thenThrow(new VersionRangeResolutionException(any()));
+      Aether aether = Aether.builder().systemSession(anySession(), system).build();


The point is to do mock(Aether.class) here.

kchodorow · 2017-07-20T15:17:42Z

...e_workspace/src/test/java/com/google/devtools/build/workspace/maven/VersionResolverTest.java

+  public void softPinnedVersions() {
+    try {
+      String version =
+          defaultResolver().resolveVersion("something", "something", "3.4");


Unit tests should not contact Maven.

I figured as such. This was more a desperate attempt to at least document the intended behavior.

I am finding testing the actual version resolution, not the failure, to be somewhat prohibitory. Two reasons why. (1) VersionRangeResult can't be mocked since it's a final class. (2) VersionRangeResult is very tightly coupled with other things within Aether, so creating using a constructor is also meh.

What are your thoughts on the following options:

Using PowerMock. This lets us mock final classes. It is a bit of a pain to import this into the code base, but very doable.

Modifying the method for our Aether class to return the highest version. I'm not the biggest fan of this.

Just not test the behavior. "You can't fail unit tests, if you don't have any" - some authoritative source

I am finding testing the actual version resolution, not the failure, to be somewhat prohibitory.

Keep in mind that we don't want to test the actual version resolution. That's Aether's business. We just want to be able to test how your code behaves.

Two reasons why. (1) VersionRangeResult can't be mocked since it's a final class. (2) VersionRangeResult is very tightly coupled with other things within Aether, so creating using a constructor is also meh.

Have it return a Version. Or a list of versions. You don't need a response, here, just the version.

Just not test the behavior. "You can't fail unit tests, if you don't have any" - some authoritative source

ಠ_ಠ

Keep in mind that we don't want to test the actual version resolution. That's Aether's business. We just want to be able to test how your code behaves.

I am not we need to add any further tests or whether we can need the soft pin tests. For full code coverage, I believe we simply need to cover the following cases:

given a null result, it throws an exception

given a versionrangeresolution exception, it throws an exception

given neither, it doesn't throw an exception and returns something.

Have it return a Version. Or a list of versions. You don't need a response, here, just the version.

Again this is a tradeoff. Finer grained tests will mean more responsibility to mimic aether/maven's behavior.

I assume we are trying to match with how aether or maven resolves versions. If so, returning a list of versions means we will need to include code to decide which version to pick. Right now, we can simply write versionRangeResult.getHighestVersion() and call it a day. However, if we return the list, then we will need to account for any weird behavior aether/maven may have.

For example, Aether internally uses VersionRangeResult.getHighestVersion() for resolving all versions. For a soft pin, I have found that this returns the soft pinned value provided it exists. This is regardless of whether the soft pinned value is the highest version or not. The behavior when that soft pin does not exist is undocumented. I am guessing it will be the next closest version, but I am not sure on that front.

I assume we are trying to match with how aether or maven resolves versions. If so, returning a list of versions means we will need to include code to decide which version to pick.

I was basing this on VersionRangeResult's API. It doesn't have any "get the right version" method, so your code will have to handle it regardless.

The behavior when that soft pin does not exist is undocumented.

You can try this out pretty easily and see what Aether does. I wrote up a quick sample program to do version requests for different version constraints on Guava and I have bad news for you: it seem that Aether returns the soft pin version, even if it doesn't exist. I'm not sure what you're supposed to do to check if it's valid.

i'll look into it and determine a solution.

Thank you for pointing this out. It's quite unfortunate aether is not as complete as hoped for. That said, I have been playing around with a few ideas. For any given artifact a:b:version-spec, we can make a version range request for a:b:[,) and aether will return a list of all potential versions of that artifact.

Here is one approach we could take. Whenever resolveVersion is called, we can simply make the request for [,), i.e. the unbounded request. With all potential versions, we could then manually select the version from the available versions. This would push the brunt of selecting versions on us. However, it is much more reliable than what I have right now.

Thank you for pointing that out. Disappointing news, indeed.

I'm not sure what you're supposed to do to check if it's valid.

I was playing around with it, and found that if you request an unbounded version [,), it returns all possible versions for an artifact as a list (ascending from oldest to latest).

For this soft pin issue, here is a potential algorithm.

def resolve_version(group, artifact, version): if is_soft_pin(version): allVersions = fetch_all_versions(group, artifact, version) return select_version(version, all_versions) # what we have implemented return resolve_version_range(group, artifact, version) def select_version(version, all_versions): if version in all_versions: return version # choose one from {fail, find nearest alternative, select highest available}

This issue only comes up with soft pins, so for version ranges, we can default to the implementation in this CL.

As for how we approach it if it's not there, I have a few ideas but let's address that in another pull request. The best approach imo would be to select the nearest valid version.

Here is a simple implementation of what I commented on above. I'll make a separate pull request after this is merged.

googlebot · 2017-07-27T13:44:58Z

So there's good news and bad news.

👍 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.

😕 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.

This is a part of my effort to replace the Resolver, with Aether. Currently, artifacts are built using Resolver.getArtifact(). In addition, the Resolver defines a class of exceptions that is associated with creating a new DefaultArtifact. This functionality can and should be removed from the Resolver. After this change, the resolver is only called from within (1) itself, (2) its tests and (3) the defaultmodelresolver.

This is a part of an ongoing effort to isolate and then replace the existing Resolver with Aether. Version resolution is the only remaining part of the Resolver that is used by any other class, DefaultModelResolver. With this removed the Resolver is currently only used by itself, its tests and GenerateWorkspace. The current implementation of resolving version given a version spec is adhoc and idiosyncratic. This CL isolates this functionality, and provides a canonical/correct version resolution mechanism. Correct version resolution has two components. (1) Pins both hard and soft, e.g. "[3.0]" and "3.0", should select the pinned version, e.g. "3.0", and (2) version ranges should select the highest available version. This functionality is easily accomplished through aether. It involves the construction of a version range request, followed by a call to aether. 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 annot mock final classes), tests are not included for correct behavior of version ranges.

All classes make create and make requests through the Aether facade. This convenience class allows us to not have to pass the RepositorySystemSession, RepositorySystem, and RemoteRepository's in multiple classes. There is one major TODO. The older classes are not constructing this object. They are just using default settings.

googlebot · 2017-07-27T14:01:56Z

CLAs look good, thanks!

provided by aether

petroseskinder · 2017-07-27T14:21:41Z

...e_workspace/src/test/java/com/google/devtools/build/workspace/maven/VersionResolverTest.java

@@ -74,33 +73,44 @@ public void failsOnInvalidVersionRange() {
   * Asserts that given a soft pinned version specification, it selects that version,
   * and does not get the highest version. "3.4" is an example of a soft pinned version specification.
   */
-  //TODO(petros): implicitly using Maven central for resolving version. This is sketchy.
  @Test
  public void softPinnedVersions() {


Here are the changed unit tests. Tried rebasing on your last pull request and that wiped out history.

I simply assert that given a list of versions from aether, we select the highest version (i.e last element in list). I included a test for soft pins, where aether simply returns a single version.

kchodorow

Gotta love GitHub code review. Sigh.

If this ends up being too slow, I think a good optimization would be to only start looking up valid versions if getting the artifact fails. However, I'm willing to let that go, for now.

kchodorow · 2017-07-27T17:57:37Z

...e_workspace/src/test/java/com/google/devtools/build/workspace/maven/VersionResolverTest.java

+          resolver.resolveVersion("something", "something", "1.0");
+      assertThat(version).isEqualTo("1.0");
+
+    } catch (


Remove the try/catch.

kchodorow · 2017-07-27T18:01:12Z

...e_workspace/src/test/java/com/google/devtools/build/workspace/maven/VersionResolverTest.java

+    Aether aether = Mockito.mock(Aether.class);
+
+    try {
+      // Using `anyRangeResult()` will ensure that rangeResult.highestVersion() == null.


Out of date comment.

Also, use https://stackoverflow.com/questions/156503/how-do-you-assert-that-a-certain-exception-is-thrown-in-junit-4-tests here and above.

that's quite neat, actually.

kchodorow · 2017-07-27T18:49:50Z

Jenkins, test this please.

into decouple

kchodorow · 2017-07-27T19:59:53Z

I think it does merge it with HEAD.

…

On Thu, Jul 27, 2017 at 3:02 PM, Petros Eskinder ***@***.***> 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 <#45 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABCkgl16TKXioUBxgVUPpduze7WgSiYks5sSN6pgaJpZM4OcHZ0> .

Previously version resolution was determined locally, without consideration of the maven central repository. This change removes any hacky calls to maven central.

All version resolution is now conducted by the VersionResolver using aether.

petroseskinder · 2017-07-27T20:26:59Z

Jenkins, test this please.

petroseskinder · 2017-07-27T20:35:05Z

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 · 2017-07-28T14:04:09Z

@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.

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.

googlebot · 2017-07-28T14:52:02Z

So there's good news and bad news.

👍 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.

😕 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 · 2017-07-28T14:55:55Z

CLAs look good, thanks!

kchodorow

Jenkins, test this please.

kchodorow · 2017-07-28T15:20:55Z

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

Test this please.

petroseskinder changed the title ~~[Refactor Resolver] Resolve versions using aether, and uncouple existing Resolver from code base.~~ [Refactor] Resolve versions using aether, and uncouple existing Resolver from code base. Jul 19, 2017

petroseskinder changed the title ~~[Refactor] Resolve versions using aether, and uncouple existing Resolver from code base.~~ Resolve versions using aether, and uncouple existing Resolver from code base. Jul 19, 2017

petroseskinder force-pushed the decouple branch 3 times, most recently from 331fafd to a835e37 Compare July 19, 2017 13:00

kchodorow reviewed Jul 19, 2017

View reviewed changes

petroseskinder force-pushed the decouple branch from 0158bce to c06645d Compare July 19, 2017 21:24

kchodorow reviewed Jul 20, 2017

View reviewed changes

petroseskinder force-pushed the decouple branch from 49af405 to 52ba385 Compare July 27, 2017 13:44

petroseskinder added 7 commits July 27, 2017 09:58

Reorder import statements to comply with Google Style Scheme. Nit

5a7e172

Add licenses and remove static functions from non-test classes

7c26134

Add comments on various hacks and workarounds.

838769c

mock aether rather than Systemsession

e4c16c3

petroseskinder force-pushed the decouple branch 2 times, most recently from 287f7ad to 71ab932 Compare July 27, 2017 14:00

petroseskinder force-pushed the decouple branch 2 times, most recently from 3563fca to 92fc31c Compare July 27, 2017 14:18

Asserts the version resolver selects the highest version from the list

92fc31c

provided by aether

petroseskinder commented Jul 27, 2017

View reviewed changes

kchodorow reviewed Jul 27, 2017

View reviewed changes

simplified unit tests

0649a29

kchodorow approved these changes Jul 27, 2017

View reviewed changes

Merge branch 'master' of https://github.com/bazelbuild/migration-tooling

4321fb9

into decouple

petroseskinder added 2 commits July 27, 2017 16:15

Refactor version resolution hacks in resolver and defaultmodelresolver.

894f06e

Previously version resolution was determined locally, without consideration of the maven central repository. This change removes any hacky calls to maven central.

Removed version resolution code from Resolver.

4865d88

All version resolution is now conducted by the VersionResolver using aether.

Pass versionResolver in Resolver rather than aether

f1cc2ae

petroseskinder force-pushed the decouple branch from 3e6a227 to c4dfa9d Compare July 27, 2017 21:33

Add reminder in VersionResolver

c4dfa9d

petroseskinder force-pushed the decouple branch 3 times, most recently from dbf5bdd to a1bf4c0 Compare July 28, 2017 14:30

petroseskinder added 2 commits July 28, 2017 10:48

Add tests for version range identification method

78f1920

petroseskinder force-pushed the decouple branch 2 times, most recently from 78f1920 to 55289fc Compare July 28, 2017 14:51

petroseskinder force-pushed the decouple branch from 55289fc to 78f1920 Compare July 28, 2017 14:55

kchodorow approved these changes Jul 28, 2017

View reviewed changes

kchodorow merged commit 24f423d into bazelbuild:master Jul 28, 2017

petroseskinder deleted the decouple branch July 28, 2017 15:31

petroseskinder mentioned this pull request Jul 31, 2017

generate_workspace should/could choose highest maven version number #16

Open

Zetten mentioned this pull request Aug 2, 2017

Aether dependency resolution fails with "Invalid Range Result", generates partial workspace #47

Open

petroseskinder mentioned this pull request Aug 3, 2017

Add back in local range resolution #50

Closed

pgr0ss mentioned this pull request Aug 8, 2017

Aether dependency resolution fails and generates partial workspace #52

Open

		@@ -0,0 +1,47 @@
		package com.google.devtools.build.workspace.maven;

		@@ -1,7 +1,16 @@
		package com.google.devtools.build.workspace.maven;


		import static com.google.devtools.build.workspace.maven.AetherUtils.mavenCentralRepository;

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

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

Conversation

petroseskinder commented Jul 19, 2017 • edited Loading

Context

Detailed Design

bazel-io commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petroseskinder Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchodorow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petroseskinder Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petroseskinder Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jul 27, 2017

googlebot commented Jul 27, 2017

Choose a reason for hiding this comment

kchodorow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchodorow commented Jul 27, 2017

kchodorow commented Jul 27, 2017 via email

petroseskinder commented Jul 27, 2017

petroseskinder commented Jul 27, 2017 • edited Loading

petroseskinder commented Jul 28, 2017 • edited Loading

googlebot commented Jul 28, 2017

googlebot commented Jul 28, 2017

kchodorow left a comment

Choose a reason for hiding this comment

kchodorow commented Jul 28, 2017

petroseskinder commented Jul 19, 2017 •

edited

Loading

petroseskinder Jul 19, 2017 •

edited

Loading

petroseskinder Jul 24, 2017 •

edited

Loading

petroseskinder Jul 27, 2017 •

edited

Loading

petroseskinder commented Jul 27, 2017 •

edited

Loading

petroseskinder commented Jul 28, 2017 •

edited

Loading