optional dependencies of required dependencies are always imported

[kevingessner]

Per 9868da9d6ee6a94b96dcc470161fad55985725b1, it appears the expected behavior of bazel-deps is to exclude optional dependencies unless they are explicitly exported. That's a good default but I'm not seeing it take effect in every case. In particular, it appears that if dependency A requires dependency B, all of B's optional dependencies are included by bazel-deps. If A is not included and B is used directly by bazel-deps, B's optional dependencies are not included.

These dependencies.yaml examples show the problem, repro'd at f5cb5823409a8b2773fa719deb5afc15a3a83425.

dependencies:
  org.apache.logging.log4j:
    log4j:
      lang: java
      modules: [ "core" ]
      version: "2.8.2"

Running generate on this file correct produces a BUILD file with just log4j-core and its required dependency log4j-api (pom):

> bazel query //...
//3rdparty/jvm/org/apache/logging/log4j:log4j_core
//3rdparty/jvm/org/apache/logging/log4j:log4j_api

However, adding log4j-1.2-api and running generate again produces a much larger set of dependencies:

dependencies:
  org.apache.logging.log4j:
    log4j:
      lang: java
      modules: [ "1.2-api", "core" ]
      version: "2.8.2"

> bazel query //...
//3rdparty/jvm/org/apache/logging/log4j:log4j_1_2_api
//3rdparty/jvm/org/apache/logging/log4j:log4j_core
//3rdparty/jvm/org/zeromq:jeromq
//3rdparty/jvm/org/jctools:jctools_core
//3rdparty/jvm/org/apache/logging/log4j:log4j_api
//3rdparty/jvm/org/apache/kafka:kafka_clients
//3rdparty/jvm/org/xerial/snappy:snappy_java
//3rdparty/jvm/org/apache/commons:commons_csv
//3rdparty/jvm/org/apache/commons:commons_compress
//3rdparty/jvm/net/jpountz/lz4:lz4
//3rdparty/jvm/com/sun/mail:javax_mail
//3rdparty/jvm/javax/activation:activation
//3rdparty/jvm/com/lmax:disruptor
//3rdparty/jvm/com/conversantmedia:disruptor
//3rdparty/jvm/org/slf4j:slf4j_api

These are not dependencies of log4j-1.2-api (pom) but are optional dependencies of log4j-core. bazel-deps is treating them as required and including them.

I'm not sure why bazel-deps is doing this but it seems wrong; it looks like this behavior changed with the Coursier switch. Optional transitive dependencies should remain optional.

[kevingessner]

Aha! This appears to be a regression with the Aether resolver. I didn't realize that was still the default; adding resolverType: coursier gets the expected behavior. Aether didn't behave this way before -- is it possible this was changed during the coursier switch?

[johnynek]

@kevingessner maybe something changed with the classifier changes? cc @roblg

[johnynek]

@kevingessner would you be willing to do a git-bisect to find the PR that changed?

[kevingessner]

@johnynek Assuming I gitted correctly: d06012b598ea16c83a14a3e70b32a4281abcb084 is the first bad commit

https://github.com/johnynek/bazel-deps/commit/d06012b598ea16c83a14a3e70b32a4281abcb084

[roblg]

The commit referenced above as the repro commit was also before the my classifier stuff merged, so it probably wasn't that (though I don't claim it's perfect :) )

FWIW, I noticed when I rebased my PR onto master while working on supporting classifiers in coursier that the deps changed and a lot more stuff started being included (though I don't think it was a simple as just optional dependencies being included). I assumed it was the aether upgrade, since things looked correct when I checked against the maven repo.

[johnynek]

that commit updated the version of aether. We can revert or we can possibly see if previously we were getting lucky somehow and coding to a bug.

Maybe reverting is better if we plan to move to coursier rather than spend more effort on aether.