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

Fix cli print out completeness and cyclic handling issue #671

Merged
merged 7 commits into from Oct 19, 2017

Conversation

Projects
None yet
2 participants
@wisechengyi
Copy link
Collaborator

wisechengyi commented Oct 12, 2017

Problem

Before resolve/fetch does not output the complete graph. E.g.

./coursier resolve -t org.apache.avro:avro:1.7.4 org.apache.commons:commons-compress:1.4.1
 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
└─ org.apache.commons:commons-compress:1.4.1
          // why is there nothing over here?

Solution

Do not avoid jars have been printed out before. E.g.

]$ cli/target/pack/bin/coursier resolve  -t org.apache.avro:avro:1.7.4   org.apache.commons:commons-compress:1.4.1 
  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
└─ org.apache.commons:commons-compress:1.4.1
   └─ org.tukaani:xz:1.0
@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Oct 13, 2017

Thanks! I'm now realizing that #376, which fixed #266, added no non regression test for it... And I'm not sure the changes here are fine with it.

I think the code in question should simply remember what are the parents of the node about to be printed, and not print it if it has itself has a parent (which the current code does, but in a buggy way).

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Oct 13, 2017

haha yeah I broke it again :D

$ coursier resolve -t \
    -r http://cogcomp.cs.illinois.edu/m2repo \
    edu.illinois.cs.cogcomp:illinois-nlp-pipeline:0.1.21
@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Oct 13, 2017

To clarify what you mean by

should simply remember what are the parents of the node about to be printed, and not print it if it has itself has a parent

Basically it is to see if any direct ancestor has appeared before, if so, do not recurse.

Given

a -> b -> c -> a
            -> e -> f

Example 1

resolve a, e

should give:

a 
-b
--c 
// not printing 'a' again
---e
----f
e
-f

Example 2

resolve a, c

should give:

a 
-b
--c 
// not printing 'a' again
---e
----f
c
-a
--b
// not printing 'c' again
-e
--f

@alexarchambault could you kindly confirm?

@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Oct 14, 2017

@wisechengyi Yep, that's what I meant!

@wisechengyi wisechengyi force-pushed the wisechengyi:661 branch from f741965 to 18c4978 Oct 17, 2017

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Oct 17, 2017

Reverting to the style pre https://github.com/coursier/coursier/pull/376/files, and add cycle detection.

wisechengyi added some commits Oct 17, 2017

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Oct 17, 2017

hey @alexarchambault, do you know if there is a way to trigger a single test from sbt, like coursier.util.TreeTests? still scala newbie here

@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Oct 17, 2017

@wisechengyi Something like testsJVM/testOnly coursier.util.TreeTests at the sbt prompt, should do, like

$ sbt
…
> testsJVM/testOnly coursier.util.TreeTests
…
> ~testsJVM/testOnly coursier.util.TreeTests
…

(second one watches for changes)

@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Oct 17, 2017

@wisechengyi Tell me as soon as you want me to review.

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Oct 18, 2017

@alexarchambault should be good for review now. thank you!

@wisechengyi wisechengyi changed the title [wip] Include complete transitive graph as resolve result Fix completeness and cyclic print out Oct 18, 2017

@wisechengyi wisechengyi changed the title Fix completeness and cyclic print out Fix completeness issue and cyclic print out Oct 18, 2017

@@ -16,8 +16,9 @@ object Tree {
*/
def recursivePrint(elems: Seq[T], ancestors: Set[T], prefix: String, acc: String => Unit): Unit = {
val unseenElems: Seq[T] = elems.filterNot(ancestors.contains)
for (elem <- unseenElems) {
val isLast = unseenElems.indexOf(elem) == unseenElems.length - 1

This comment has been minimized.

@alexarchambault

alexarchambault Oct 18, 2017

Member

I think the call to indexOf makes things quadratic here (calling indexOf for each element should be quadratic wrt to unseenElems.length). I pushed a quick fix.

This comment has been minimized.

@wisechengyi

wisechengyi Oct 18, 2017

Collaborator

ok good catch. thanks!

@wisechengyi wisechengyi changed the title Fix completeness issue and cyclic print out Fix cli print out completeness and cyclic handling issue Oct 18, 2017

@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Oct 19, 2017

Merging, thanks!

@alexarchambault alexarchambault merged commit 07e985b into coursier:master Oct 19, 2017

2 checks passed

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

@wisechengyi wisechengyi deleted the wisechengyi:661 branch Nov 14, 2017

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