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

[coursier/m2-coords] update coursier json parsing; use maven's coords #5475

Merged
merged 15 commits into from Mar 10, 2018

Conversation

Projects
None yet
3 participants
@baroquebobcat
Copy link
Contributor

baroquebobcat commented Feb 16, 2018

This is a multi-tool change. The coursier side is here: coursier/coursier#782

This fixes the issue with coursier resolution where transitive dependencies that have classifiers can't be differentiated. Ie, if you have a classifier on a dependency, you get all the jars even if they have a different classifier.

Additionally this updates the m2 coordinate string representation to bring it inline with maven's coordinate language. This is because I've chosen that language to use in the coursier change as well.

[coursier/m2-coords] update coursier json parsing; use maven's coords
This is a multi-tool change. The coursier side is here: coursier/coursier#782

This fixes the issue with coursier resolution where transitive dependencies that have classifiers can't be differentiated. Ie, if you have a classifier on a dependency, you get all the jars even if they have a different classifier.

Additionally this updates the m2 coordinate string representation to bring it inline with maven's coordinate language. This is because I've chosen that language to use in the coursier change as well
@wisechengyi
Copy link
Contributor

wisechengyi left a comment

Thanks @baroquebobcat! The default resolve looks good.

The area on sources and javadoc may need a little more work. Currently Pants passes --sources and --javadoc (https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/tasks/coursier_resolve.py#L305-L309) to coursier, but it is not working with one file per Dependency model in coursier/coursier#782. For example:

$ fetch -t  org.apache.avro:avro:1.7.4 --sources --json-output-file z.out
  Result:
└─ org.apache.avro:avro:1.7.4
   ├─ com.thoughtworks.paranamer:paranamer:2.3
   ├─ org.apache.commons:commons-compress:1.4.1
   │  └─ org.tukaani:xz:1.0
   ├─ org.codehaus.jackson:jackson-core-asl:1.8.8
   ├─ org.codehaus.jackson:jackson-mapper-asl:1.8.8
   │  └─ org.codehaus.jackson:jackson-core-asl:1.8.8
   ├─ org.slf4j:slf4j-api:1.6.4
   └─ org.xerial.snappy:snappy-java:1.0.4.1
/Users/yic/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/tukaani/xz/1.0/xz-1.0-sources.jar
/Users/yic/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/com/thoughtworks/paranamer/paranamer/2.3/paranamer-2.3-sources.jar
/Users/yic/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/slf4j/slf4j-api/1.6.4/slf4j-api-1.6.4-sources.jar
/Users/yic/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/codehaus/jackson/jackson-core-asl/1.8.8/jackson-core-asl-1.8.8-sources.jar
/Users/yic/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/xerial/snappy/snappy-java/1.0.4.1/snappy-java-1.0.4.1-sources.jar
/Users/yic/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/codehaus/jackson/jackson-mapper-asl/1.8.8/jackson-mapper-asl-1.8.8-sources.jar
/Users/yic/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/apache/commons/commons-compress/1.4.1/commons-compress-1.4.1-sources.jar
/Users/yic/Library/Caches/Coursier/v1/https/repo1.maven.org/maven2/org/apache/avro/avro/1.7.4/avro-1.7.4-sources.jar

$ jq < z.out 
{
  "conflict_resolution": {},
  "dependencies": [
    {
      "coord": "com.thoughtworks.paranamer:paranamer:2.3",
      "file": null,
      "dependencies": []
    },
    {
      "coord": "org.slf4j:slf4j-api:1.6.4",
      "file": null,
      "dependencies": []
    },
    {
      "coord": "org.xerial.snappy:snappy-java:1.0.4.1",
      "file": null,
      "dependencies": []
    },
    {
      "coord": "org.apache.commons:commons-compress:1.4.1",
      "file": null,
      "dependencies": [
        "org.tukaani:xz:1.0"
      ]
    },
    {
      "coord": "org.apache.avro:avro:1.7.4",
      "file": null,
      "dependencies": [
        "org.slf4j:slf4j-api:1.6.4",
        "org.tukaani:xz:1.0",
        "org.xerial.snappy:snappy-java:1.0.4.1",
        "com.thoughtworks.paranamer:paranamer:2.3",
        "org.apache.commons:commons-compress:1.4.1",
        "org.codehaus.jackson:jackson-mapper-asl:1.8.8",
        "org.codehaus.jackson:jackson-core-asl:1.8.8"
      ]
    },
    {
      "coord": "org.tukaani:xz:1.0",
      "file": null,
      "dependencies": []
    },
    {
      "coord": "org.codehaus.jackson:jackson-mapper-asl:1.8.8",
      "file": null,
      "dependencies": [
        "org.codehaus.jackson:jackson-core-asl:1.8.8"
      ]
    },
    {
      "coord": "org.codehaus.jackson:jackson-core-asl:1.8.8",
      "file": null,
      "dependencies": []
    }
  ],
  "version": "0.1.0"
}

sources files are fetched but do not show up in the report. This is expected because coursier finds multiple files associated with each dependency such as pom, jar, src, doc, etc.

I can see a couple of options:

  1. Instead of passing --sources --javadoc from Pants, specify the modules with explicit classifier arguments. x:y:z --sources --javadoc will be converted to x:y:z,classifier=sources x:y:z,classifier=javadoc, or
  2. From coursier side, recognize --sources --javadoc as special cases (which is already the case for fetch https://github.com/coursier/coursier/blob/master/cli/src/main/scala-2.12/coursier/cli/Helper.scala#L576-L577) and then automatically expand that in the json report.

Depending on the effort required, 1) is likely easier and 2) is more consistent for coursier cli handling.

elif ct == 3:
org, name, packaging, rev = string_coord.split(':')
elif ct == 4:
org, name, packaging, classifier, rev = string_coord.split(':')

This comment has been minimized.

@wisechengyi

wisechengyi Feb 19, 2018

Contributor

woot!


for dep in result['dependencies']:
simple_coord = dep['coord']

This comment has been minimized.

@wisechengyi

wisechengyi Feb 19, 2018

Contributor

nit: simple_coord meant the simplest format org:name:version (

def simple_coord(self):
"""
A simple version of coordinate representation with org:name:version without classifier or ext
:return: org:name:version
"""
return '{}:{}:{}'.format(self.org, self.name, self.rev)
). Since the coordinates in the json can take on the full maven, it may be better to rename it to maven_coord

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 20, 2018

Contributor

yeah. Another way to do it would be to be more literal, ie, org_name_rev

(org, name, _) = coord.split(':')
org_name_to_org_name_rev['{}:{}'.format(org, name)] = coord
m2coord = M2Coordinate.from_string(coord)
org_name_to_org_name_rev['{}:{}'.format(m2coord.org, m2coord.name)] = coord

This comment has been minimized.

@wisechengyi

wisechengyi Feb 19, 2018

Contributor

org_name_to_org_name_rev was used to as catch all for a certain groupId:artifactId (line 447-449), I think with your change it shouldn't be needed anymore, or at least it's worth looking into why it may be still needed and get it clarified.

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 20, 2018

Contributor

cool

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 6, 2018

Contributor

It's still needed, because conflict resolution in the json doesn't always have entries for requested coordinates. Sometimes this is because pants removes them in its own version adjustments.

if not jar_path:
# NB: Not all coordinates will have associated files.
# This is fine. Some coordinates will just have dependencies.
continue

This comment has been minimized.

@wisechengyi

wisechengyi Feb 19, 2018

Contributor

Is there an example about this?

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 20, 2018

Contributor

Yes. I should add a test though. I don't have an example off the top of my head.

@stuhood
Copy link
Member

stuhood left a comment

Probably needs an implementation_version change.

@baroquebobcat

This comment has been minimized.

Copy link
Contributor

baroquebobcat commented Mar 6, 2018

Added implementation bump to Coursier mixin.

@baroquebobcat baroquebobcat changed the title [WIP][coursier/m2-coords] update coursier json parsing; use maven's coords [coursier/m2-coords] update coursier json parsing; use maven's coords Mar 6, 2018


self._populate_results_dir(vt_set_results_dir, results)
resolve_vts.update()

def _override_classifiers_for_conf(self, conf):
if conf == 'src_doc': #TODO encapsulate this in the result instead of here.

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 6, 2018

Contributor

By this I mean that I think that Coursier's json output should note what the forced classifiers were. I think that's a more invasive change though. I could add an issue on Coursier and link it here rather than having a bare TODO.

@@ -348,7 +371,8 @@ def _construct_cmd_args(jars, common_args, global_excludes,

# Dealing with intransitivity and forced versions.
for j in jars:
if not j.rev:
if not j.rev: #TODO we should still resolve these right?
logger.warn('Coursier does not allow using latest as the revision: ignoring {}'.format(j.coordinate))

This comment has been minimized.

@baroquebobcat

baroquebobcat Mar 6, 2018

Contributor

I'm going to just xx this TODO now that I've added a warning and a xfail test.

baroquebobcat added some commits Mar 7, 2018

resolve some issues from the merge of master; add test case for jarle…
…ss pom

- I verified that the jarless pom case fails by removing the continue on no jars clause
@@ -349,6 +374,7 @@ def _construct_cmd_args(jars, common_args, global_excludes,
# Dealing with intransitivity and forced versions.
for j in jars:
if not j.rev:
logger.warn('Coursier does not allow using latest as the revision: ignoring {}'.format(j.coordinate))

This comment has been minimized.

@wisechengyi

wisechengyi Mar 7, 2018

Contributor

Raise task error to be more explicit.

logger = logging.getLogger(__name__)


def with_new_classifier(coord, classifier):

This comment has been minimized.

@wisechengyi

wisechengyi Mar 7, 2018

Contributor

Move to M2Coordinate class

"a": ResolvedJar(classifier='', path/cache_path="a.jar"),
"a:sources": ResolvedJar(classifier='sources', path/cache_path="a-sources.jar"),
"b": ResolvedJar(classifier='', path/cache_path="b.jar"),
"c": ResolvedJar(classifier='', path/cache_path="c.jar"),

This comment has been minimized.

@wisechengyi

wisechengyi Mar 7, 2018

Contributor

Update doc:
{
M2cooord(...): ResolvedJar(classifier='', path/cache_path="a.jar"),
M2cooord(...): ResolvedJar(classifier='', path/cache_path="b.jar")
}

if not os.path.exists(pants_path):
safe_mkdir(os.path.dirname(pants_path))
os.symlink(jar_path, pants_path)
if not os.path.exists(jar_path):

This comment has been minimized.

@wisechengyi

wisechengyi Mar 7, 2018

Contributor

xxx


lib_cp = compile_classpath.get_for_target(lib)

self.assertEqual(5, len(lib_cp))

This comment has been minimized.

@wisechengyi

wisechengyi Mar 7, 2018

Contributor
with self.assertRaises(TaskError('cannot handle latest revision in courseri')):
  do x
self.assertEquals(org_name_ref, M2Coordinate.from_string(str(org_name_ref)))

org_name_ref_classifier = M2Coordinate(org='org.example', name='lib',
rev='the-ref', classifier='classify')

self.assertEquals('org.example:lib:the-ref:classify:jar', str(org_name_ref_classifier))
self.assertEquals('org.example:lib:jar:classify:the-ref', str(org_name_ref_classifier))

This comment has been minimized.

@wisechengyi

wisechengyi Mar 7, 2018

Contributor

nit: the-rev

@wisechengyi
Copy link
Contributor

wisechengyi left a comment

Thanks @baroquebobcat !

@baroquebobcat baroquebobcat merged commit 61d8f48 into pantsbuild:master Mar 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wisechengyi wisechengyi added this to the 1.5.x milestone Mar 13, 2018

wisechengyi added a commit that referenced this pull request Mar 14, 2018

[coursier/m2-coords] update coursier json parsing; use maven's coords (
…#5475)

This is a multi-tool change. The coursier side is here: coursier/coursier#782

This fixes the issue with coursier resolution where transitive dependencies that have classifiers can't be differentiated. Ie, if you have a classifier on a dependency, you get all the jars even if they have a different classifier.

Additionally this updates the m2 coordinate string representation to bring it inline with maven's coordinate language. This is because I've chosen that language to use in the coursier change as well. I've bumped ivy and coursier's implementation version to invalidate cached artifacts that used the old format.

wisechengyi added a commit that referenced this pull request Mar 15, 2018

[coursier/m2-coords] update coursier json parsing; use maven's coords (
…#5475)

This is a multi-tool change. The coursier side is here: coursier/coursier#782

This fixes the issue with coursier resolution where transitive dependencies that have classifiers can't be differentiated. Ie, if you have a classifier on a dependency, you get all the jars even if they have a different classifier.

Additionally this updates the m2 coordinate string representation to bring it inline with maven's coordinate language. This is because I've chosen that language to use in the coursier change as well. I've bumped ivy and coursier's implementation version to invalidate cached artifacts that used the old format.

wisechengyi added a commit that referenced this pull request Mar 20, 2018

[coursier/m2-coords] update coursier json parsing; use maven's coords (
…#5475)

This is a multi-tool change. The coursier side is here: coursier/coursier#782

This fixes the issue with coursier resolution where transitive dependencies that have classifiers can't be differentiated. Ie, if you have a classifier on a dependency, you get all the jars even if they have a different classifier.

Additionally this updates the m2 coordinate string representation to bring it inline with maven's coordinate language. This is because I've chosen that language to use in the coursier change as well. I've bumped ivy and coursier's implementation version to invalidate cached artifacts that used the old format.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment