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

jar dependency match with json report #743

Closed
wisechengyi opened this Issue Jan 18, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@wisechengyi
Copy link
Collaborator

wisechengyi commented Jan 18, 2018

Problem

Coursier command:

fetch -t \
com.esotericsoftware.reflectasm:reflectasm:1.09,classifier=shaded \ # from pants jar_dep A
com.esotericsoftware.reflectasm:reflectasm:1.09 \ # from pants jar_dep B
org.ow2.asm:asm:4.0,classifier=sources \ # from pants jar_dep C
-A jar,src,doc,test-jar --json-output-file x.out

  Result:
├─ com.esotericsoftware.reflectasm:reflectasm:1.09
│  └─ org.ow2.asm:asm:4.0
├─ com.esotericsoftware.reflectasm:reflectasm:1.09
│  └─ org.ow2.asm:asm:4.0
└─ org.ow2.asm:asm:4.0

Since there are 3 roots to resolve, the tree is correct. One com.esotericsoftware.reflectasm:reflectasm:1.09 wants the default jar, and the other wants the shaded classifier jar. (UI wise it could be better to show that which is which)

Now look at the json output:

{
  "conflict_resolution": {},
  "dependencies": [
    {
      "coord": "org.ow2.asm:asm:4.0",
      "files": [
        [
          "sources",
          "<cache_dir>/https/repo1.maven.org/maven2/org/ow2/asm/asm/4.0/asm-4.0-sources.jar"
        ]
      ],
      "dependencies": []
    },
    {
      "coord": "com.esotericsoftware.reflectasm:reflectasm:1.09",
      "files": [
        [
          "shaded",
          "<cache_dir>/https/repo1.maven.org/maven2/com/esotericsoftware/reflectasm/reflectasm/1.09/reflectasm-1.09-shaded.jar"
        ]
      ],
      "dependencies": [
        "org.ow2.asm:asm:4.0"
      ]
    },
    {
      "coord": "org.ow2.asm:asm:4.0",
      "files": [
        [
          "",
          "<cache_dir>/https/repo1.maven.org/maven2/org/ow2/asm/asm/4.0/asm-4.0.jar"
        ]
      ],
      "dependencies": []
    },
    {
      "coord": "com.esotericsoftware.reflectasm:reflectasm:1.09",
      "files": [
        [
          "",
          "<cache_dir>/https/repo1.maven.org/maven2/com/esotericsoftware/reflectasm/reflectasm/1.09/reflectasm-1.09.jar"
        ]
      ],
      "dependencies": [
        "org.ow2.asm:asm:4.0"
      ]
    }
  ]
}

For jar_dep A, it has to find the coordinate "com.esotericsoftware.reflectasm:reflectasm". There are two of them, but it knows to find the one with classifier 'shaded':

    {
      "coord": "com.esotericsoftware.reflectasm:reflectasm:1.09",
      "files": [
        [
          "shaded",
          "<cache_dir>/https/repo1.maven.org/maven2/com/esotericsoftware/reflectasm/reflectasm/1.09/reflectasm-1.09-shaded.jar"
        ]
      ],
      "dependencies": [
        "org.ow2.asm:asm:4.0"
      ]
    },

Great. jar_dep A grabs reflectasm-1.09-shaded.jar into its collection. Then it needs to grab its dependencies as well. Given the clue org.ow2.asm:asm:4.0, it finds:

    {
      "coord": "org.ow2.asm:asm:4.0",
      "files": [
        [
          "",
          "<cache_dir>/https/repo1.maven.org/maven2/org/ow2/asm/asm/4.0/asm-4.0.jar"
        ]
      ],
      "dependencies": []
    },

and

    {
      "coord": "org.ow2.asm:asm:4.0",
      "files": [
        [
          "sources",
          "<cache_dir>/https/repo1.maven.org/maven2/org/ow2/asm/asm/4.0/asm-4.0-sources.jar"
        ]
      ],
      "dependencies": []
    },

Now, there is no way to tell which it needs, because all it knows is just org.ow2.asm:asm:4.0. Currently Pants' behavior is to grab both. However the correct jar to grab is just asm-4.0.jar and not asm-4.0-sources.jar

Proposed solution

Option 1:

Add classifier to dependencies org:name:version, i.e. org:name:version:classifier. For the same example:

{
  "conflict_resolution": {},
  "dependencies": [
    {
      "coord": "org.ow2.asm:asm:4.0",
      "files": [
        [
          "sources",
          "<cache_dir>/https/repo1.maven.org/maven2/org/ow2/asm/asm/4.0/asm-4.0-sources.jar"
        ]
      ],
      "dependencies": []
    },
    {
      "coord": "com.esotericsoftware.reflectasm:reflectasm:1.09",
      "files": [
        [
          "shaded",
          "<cache_dir>/https/repo1.maven.org/maven2/com/esotericsoftware/reflectasm/reflectasm/1.09/reflectasm-1.09-shaded.jar"
        ]
      ],
      "dependencies": [
        "org.ow2.asm:asm:4.0:<empty classifier>"
      ]
    },
    {
      "coord": "org.ow2.asm:asm:4.0",
      "files": [
        [
          "",
          "<cache_dir>/https/repo1.maven.org/maven2/org/ow2/asm/asm/4.0/asm-4.0.jar"
        ]
      ],
      "dependencies": []
    },
    {
      "coord": "com.esotericsoftware.reflectasm:reflectasm:1.09",
      "files": [
        [
          "",
          "<cache_dir>/https/repo1.maven.org/maven2/com/esotericsoftware/reflectasm/reflectasm/1.09/reflectasm-1.09.jar"
        ]
      ],
      "dependencies": [
        "org.ow2.asm:asm:4.0:<empty classifier>"
      ]
    }
  ]
}

Now jar_dep A knows the dependency is org.ow2.asm:asm:4.0:<empty classifier>, thus finding the following and grabs asm-4.0.jar only.

    {
      "coord": "org.ow2.asm:asm:4.0",
      "files": [
        [
          "",
          "<cache_dir>/https/repo1.maven.org/maven2/org/ow2/asm/asm/4.0/asm-4.0.jar"
        ]
      ],
      "dependencies": []
    },

Option 2:
Make both coord and dependencies to adopt org:name:version:classifier. I am not sure what the report would look like when --sources and --javadoc are both specified, so might need more thoughts there. But option 1 should be more straight forward.

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Jan 18, 2018

Currently the command is only valid in my branch #735

@alexarchambault

This comment has been minimized.

Copy link
Member

alexarchambault commented Jan 21, 2018

Couldn't this rely on the syntax you added in #735? (the coords would be replaced by the more detailed ones of #735) As this syntax is better suited for full fledged dependencies.

@wisechengyi

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Jan 22, 2018

I think it depends, as we may not need to surface all attributes passed on command line into the json report. @baroquebobcat will be working on this and proposed to use groupId:artifactId:packaging:classifier:version according to https://maven.apache.org/pom.html#Maven_Coordinates in json report.

That said, option 1 could be an intermediary step, before Pants can fully adopt the full maven spec.

I plan to add documentation on json report and versioning and to highlight the json report is not stable yet. (#749)

@baroquebobcat

This comment has been minimized.

Copy link
Contributor

baroquebobcat commented Jan 23, 2018

One thought I had here is that with either option I think we ought to adopt Maven's coordinate structure instead of org:name:version:classifier. Maven's coordinates have 3 different forms.

  • Default org:name:version, eg org.ow2.asm:asm:4.0
  • Including packaging/type org:name:packaging:version, eg org.ow2.asm:asm:jar:4.0
  • Including classifier org:name:packaging:classifier:version, eg org.ow2.asm:asm:jar:sources:4.0

By using the above scheme we don't have to have an explicit value for empty classifier because it's encoding in the formatting. It also opens up a place to include file-extension/packaging information on a per artifact level.

I don't know enough about the currently accepted schema to be sure that this'll work, but I'd appreciate some feedback.

wisechengyi added a commit to pantsbuild/pants that referenced this issue Jan 24, 2018

Single resolve with coursier (#5362)
### Problem

Earlier `jar_dependency`s are divided into classifiers for coursier to resolve because coursier's `--classifier` option is applied globally.

### Solution
Add the classifier specification on module level coursier/coursier#735. This is the pants side of the change which removes the classifier grouping.

### Result

See added back test case. 

Note: finding the correct strict subset jars for transitive dependencies is still an issue coursier/coursier#743. However, it is very, very rare.

stuhood added a commit to pantsbuild/pants that referenced this issue Jan 24, 2018

Single resolve with coursier (#5362)
### Problem

Earlier `jar_dependency`s are divided into classifiers for coursier to resolve because coursier's `--classifier` option is applied globally.

### Solution
Add the classifier specification on module level coursier/coursier#735. This is the pants side of the change which removes the classifier grouping.

### Result

See added back test case. 

Note: finding the correct strict subset jars for transitive dependencies is still an issue coursier/coursier#743. However, it is very, very rare.

wisechengyi added a commit that referenced this issue Mar 9, 2018

json-report: one file per dependency instead of multiple; use m2 coor…
…ds (#782)

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

This comment has been minimized.

Copy link
Collaborator

wisechengyi commented Mar 13, 2018

closed by d0b4686

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