Add support for classifier

[GinFungYJF]

For example, the artifact "net.sf.json-lib:json-lib:jar:jdk15:2.4" has a classifier jdk15，but the dependencies in the yaml do not have this option.

[philwo]

+1, I think I need this in order to use Netty's native transports: http://netty.io/wiki/native-transports.html

[johnynek]

happy to accept a PR here.

[werkt]

+1 here, jffi does not work without a classifier for native. I will prepare a PR

[johnynek]

Nd4j seems to need it too: https://nd4j.org/

Would love a PR! :)

[werkt]

Progress has been slow here, mostly due to rusty scala and exploratory coding. jffi doesn't have a dependency on its native interface, which may or may not be their problem, but the existing group -> name -> ProjectRecord definition will not work with matched names to declare/inject target level dependencies or fetch classifier-unique artifacts. Others can comment on the dependency specification pattern of other libraries. Seeking consensus from @johnynek here before I get stuck behind an implementation:

I propose either: switching to maven/pom's dependency definition style:

dependencies:
  com.github.jnr:
    - name: jffi
      lang: java
      version: "1.2.16"
      runtime: ":::native" # shorthand for 'matching group', 'matching name', 'matching version', 'classifier'
      # adds the supplementary 'runtime_deps' entry for a resolved dependency

    - name: jffi
      lang: java
      version: "1.2.16"
      classifier: "native"

or, we follow the pattern of reductive specification, keep the map, and add a global dependency edge definition type:

dependencies:
  com.github.jnr:
    jffi:
      lang: java
      version: "1.2.16"
      classifiers: [null, "native"]

# open to all interpretations here, assuming a flat version mapping
runtime-deps:
  com.github.jnr:
    jffi:
      - "::native"

Thoughts?

[johnynek]

Well, I don't want to change the format of the yaml files if we can help it. So moving from a map to a list is not my ideal.

The classifiers: [null, "native"] but classifiers: ["", "native"] would match the current approach we do with modules: which uses "" to represent the "non-module" case (e.g. when you have artifacts "foo", "foo-bar", "foo-baz" you have foo with modules "", "bar", "baz".

Since classifiers are unusual (we need 0 of them currently at Stripe), I don't want to add too many strange exceptions. Do you think this will work?

Caveat: I don't know much at all about classifiers.

[werkt]

I think it will work fine, just note that I added the runtime-deps explicit runtime dependency specification there as well, so that I can draw the (omitted in pom) requirement of native-classifier imports

[kevingessner]

@werkt Do you have a PR for this work? I'd be happy to help if I can.

[werkt]

@kevingessner posted as #131 - I have a preliminary runtime-deps at https://github.com/werkt/bazel-deps/tree/model-runtime-deps and don't think I have the time to [learn to do enough proper scala to] take that through to completion, if you want to pick it up!

[johnynek]

So, now, we have a few proposals....

It looks like #130 and #131 are a bit different. #131 allows multiple classifers, but #130 does not. Having never used classifiers, I'm not in a good position to say what is needed.

Can we comment on the designs here and try to get something merged soon that meets everyone's needs and is also nice to use?

[roblg]

My proposal in #130 definitely won't work for all cases, since I'm pretty sure it (combined with yaml map) won't allow you to have an artifact both with and without a classifier since they'll get swallowed up by the same key. I'm also a little concerned after thinking about it some more that supporting both classifier and packaging with an array syntax is going to result in cases where bazel-deps ends up trying to download more than it needs.

As far as I know, both classifier and packaging can be many-valued, and the maven repository is OK with that. I don't have an example offhand that uses both packaging and classifier, but @werkt gave an example above of a project that ships the same artifact with multiple classifiers, and we have examples internally of projects that ship artifacts under both jar and test-jar packaging. If we use arrays in the ProjectRecord for both classifiers and packagings, we could end up in a situation where we try to download all combinations of classifier/packaging in the cross product of the two arrays, even if we only need a subset.

I'm wondering if it might make sense to include both packaging and classifier in the ArtifactOrProject entry. It would mean (potentially) that in order to specify a classifier you're forced to specify the packaging as jar, but I think the syntax would be reasonable to maven folks, and it gets us away from the cross-product behavior. (I haven't looked at all the code around this, so it might be an insane idea too)

Concrete example (from @werkt's example above):

dependencies:
  com.github.jnr:
    jffi: # implied `jffi:jar` -- including 'jar' packaging; no classifier
      lang: java
      version: "1.2.16"
    "jffi:jar:native": # 'jar' packaging, 'native' classifier
      lang: java
      version: "1.2.16"

Pros: no cross-product problem, explicit, keeps shorthand module syntax available Cons: have to quote the keys because of the colons (but this should be rare-ish)

One other reason I'm leaning towards favoring this approach is that I'd like to extend exclude to be able to exclude specific packaging types/classifiers as well. (Right now it's either ignoring packaging, or only excluding jar packaging -- I can't remember which)

edit: posted too early.

[roblg]

Getting around to poking around a little bit. The syntax is kinda weird to combine classifiers and/or packaging with the modules key, although it's not ambiguous:

dependencies:
  com.github.jnr:
    "jffi:jar:native":
      modules: [ "", "example" ] # hypothetical examples
      lang: java
      version: "1.2.16"

should result in:

-> com.github.inr:jffi:jar:native
-> com.github.inr:jffi-example:jar:native

[johnynek]

would we ever want the same artifact with different packagings like this?

[roblg]

It's allowed, but not super common I don't think. The case I can think of at Redfin is (at least internally), we have dependencies on different outputs from the same project like:

com.redfin.foo:data:jar, com.redfin.foo:data:test-jar (where test-jar is sources from src/test/java in maven projects)

And TBH, if I were writing the dependencies.yaml file by hand, I wouldn't collapse these into a single entry w/ modules, I'd list them separately. I just realized when I started digging into the code that ArtifactOrProject can be combined with a Subproject, which complicated my initial simple stab at a representation.

[kevingessner]

Either of those approaches would work for me for the classifier. I think the approach in #131 would make for a shorter YAML file in my case; I need to import the -tests JAR of several of twitter-inject's JARs. But #130/#134's syntax would work too.

We don't use non-JAR packaging so I can't speak to the usefulness of that.

[roblg]

I'd been meaning to circle back around on this. I was holding off doubling down on adding tests, etc. until we had consensus around which approach was favored. Sounds like there's now a second vote for #134 (https://github.com/johnynek/bazel-deps/pull/134#issuecomment-374659043).

If others are cool with that approach I'll try to finalize the PR in the next couple days.