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

json-report: one file per dependency instead of multiple; use m2 coords #782

Merged
merged 13 commits into from Mar 9, 2018

Conversation

Projects
None yet
3 participants
@baroquebobcat
Copy link
Contributor

baroquebobcat commented Feb 16, 2018

When classifiers are used as part of dependency specifications, it's important to be able to select just the classified artifact. Unfortunately, in the current json, dependencies don't specify classifiers, so it isn't possible to just get one of the artifacts for a dependency when a classifier is required.

This patch introduces maven style artifact prefixes in order to include classifier and packaging information in the coordinates. By doing that, we can use them as keys in dependency lists more easily and it allows consumers of the json to treat those dependency keys as mostly opaque ids rather than having to parse them.

Addresses #743

json-report: one file per dependency instead of multiple; use m2 coor…
…dinates

When classifiers are used as part of dependency specifications, it's important to be able to select just the classified artifact. Unfortunately, in the current json, dependencies don't specify classifiers, so it isn't possible to just get one of the artifacts for a dependency when a classifier is required.

This patch introduces maven style artifact prefixes in order to include classifier and packaging information in the coordinates. By doing that, we can use them as keys in dependency lists more easily and it allows consumers of the json to treat those dependency keys as mostly opaque ids rather than having to parse them.

baroquebobcat added a commit to baroquebobcat/pants that referenced this pull request Feb 16, 2018

[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
@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Feb 18, 2018

@baroquebobcat The CI error seems legit… contains is called on an Option[String], not on the contained String (I guess you want something like .exists(_.contains("…")))

@baroquebobcat

This comment has been minimized.

Copy link
Contributor

baroquebobcat commented Feb 20, 2018

Yeah. Thanks for looking at it.

@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Feb 23, 2018

@baroquebobcat New CI failure seems legit too.

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Feb 23, 2018

Turns out I left something when splitting the tests #788, so CI should pass after that.

@wisechengyi
Copy link
Collaborator

wisechengyi left a comment

Thanks! Looks good overall.

)

assert(
report == "{\"conflict_resolution\":{},\"dependencies\":[],\"version\":\"0.1.0\"}")

This comment has been minimized.

@wisechengyi

wisechengyi Feb 27, 2018

Collaborator

Any particular reason string based assertion is used rather than parsing the json report then assert on structured data? https://github.com/coursier/coursier/blob/master/cli/src/test/scala-2.12/coursier/cli/CliFetchIntegrationTest.scala#L18

This comment has been minimized.

@baroquebobcat

baroquebobcat Feb 28, 2018

Contributor

I think the assertion failure message is a bit clearer with the string, and the string is the actual output, so it feels like the right thing to test against. That said, it could be a bit too fragile.

baroquebobcat added some commits Mar 1, 2018

pass around the override classifiers when generating the json report …
…so that the dependency listings can be corrected
@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Mar 5, 2018

This error consistently in windows CI with this change:
https://github.com/coursier/coursier/pull/782/files#diff-86042d8fbf74d324dff94d39c3760dfbR1073

C:\Users\appveyor\AppData\Local\Temp\1\sbt_a021fee1\scala-sources-javadoc-jars\build.sbt:35)
[00:32:20] [info] 	at $66918475b6504a4b75df$$anonfun$$sbtdef$1.apply(C:\Users\appveyor\AppData\Local\Temp\1\sbt_a021fee1\scala-sources-javadoc-jars\build.sbt:40)
[00:32:20] [info] 	at $66918475b6504a4b75df$$anonfun$$sbtdef$1.apply(C:\Users\appveyor\AppData\Local\Temp\1\sbt_a021fee1\scala-sources-javadoc-jars\build.sbt:8)
[00:32:20] [info] 	at scala.Function1$$anonfun$compose$1.apply(Function1.scala:47)
[00:32:20] [info] 	at sbt.$tilde$greater$$anonfun$$u2219$1.apply(TypeFunctions.scala:40)
[00:32:20] [info] 	at sbt.std.Transform$$anon$4.work(System.scala:63)
[00:32:20] [info] 	at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:226)
[00:32:20] [info] 	at sbt.Execute$$anonfun$submit$1$$anonfun$apply$1.apply(Execute.scala:226)
[00:32:20] [info] 	at sbt.ErrorHandling$.wideConvert(ErrorHandling.scala:17)
[00:32:20] [info] 	at sbt.Execute.work(Execute.scala:235)
[00:32:20] [info] 	at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:226)
[00:32:20] [info] 	at sbt.Execute$$anonfun$submit$1.apply(Execute.scala:226)
[00:32:20] [info] 	at sbt.ConcurrentRestrictions$$anon$4$$anonfun$1.apply(ConcurrentRestrictions.scala:159)
[00:32:20] [info] 	at sbt.CompletionService$$anon$2.call(CompletionService.scala:28)
[00:32:20] [info] 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[00:32:20] [info] 	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
[00:32:20] [info] 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[00:32:20] [info] 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
[00:32:20] [info] 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
[00:32:20] [info] 	at java.lang.Thread.run(Thread.java:748)
[00:32:20] [info] [error] (*:updateClassifiersCheck) java.lang.AssertionError: assertion failed: scala-library javadoc not found
[00:32:20] [info] [error] Total time: 2 s, completed Mar 3, 2018 12:38:34 AM

I also verified it by singling it out at #750

The test should be covered in travis as well, do you have any clue why this would be case, @alexarchambault?

@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Mar 5, 2018

It's possibly https://github.com/coursier/coursier/pull/782/commits/afe30721a00cc9790edc1289d58240d8eade6704… From what I remember, the scripted test that fails corresponds to things that are a bit hackish (but with non reg tests). I'll have a look at it.

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Mar 5, 2018

@alexarchambault the link is invalid btw

baroquebobcat added a commit to baroquebobcat/pants that referenced this pull request Mar 6, 2018

@baroquebobcat

This comment has been minimized.

Copy link
Contributor

baroquebobcat commented Mar 6, 2018

It looks like I've got the CI issue resolved. It seems that the change I made to Resolution broke some assumptions. I've reverted that and instead do the dep flattening in Helper.

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Mar 7, 2018

Thanks. We are still vetting this change against our internal repo. will merge if all checks out

@wisechengyi wisechengyi merged commit d0b4686 into coursier:master Mar 9, 2018

2 checks passed

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

baroquebobcat added a commit to pantsbuild/pants that referenced this pull request Mar 10, 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 to pantsbuild/pants 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 to pantsbuild/pants 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 to pantsbuild/pants 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