Consider an optional switch to turn all `deps` into `exports`, allowing access to all transitive classes just by depending on the top level artifact about rules_jvm_external HOT 22 OPEN

While I get that's what maven and gradle users are used to, it's one of the worst decisions ever made in the maven/gradle space. It's the opposite of "Strict deps". It has led to so much bad graph specification in my experience of several decades of the maven ecosystem.

Indeed, we're doing the exact opposite in our gradle builds, figuring out lint rules or gradle build code to constrain projects so you must declare all the deps you use, and not "get it for free" from the transitive closure.

I cannot disagree more with this recommendation in practice. This is one of those decisions that feels convenient, but has the effect of concealing problems in the deps graph, because you aren't specifying what you use. Maven has always supported this transitive convenience, but it has also recommended against the practice, from the get-go. From the aforelinked doc: "Although transitive dependencies can implicitly include desired dependencies, it is a good practice to explicitly specify the dependencies you are directly using in your own source code. This best practice proves its value especially when the dependencies of your project changes their dependencies."

That said, we can't police users and prevent them from every choice one might make differently. If users will forgo the benefits of your tool because they insist on this convenience, then I'd rather they use the tool. But still, I would advise that it be recommended against in the strongest terms.

from rules_jvm_external.

While that integration is very helpful, it doesn't solve this issue, as it is less about the amount of work needed to declare all the transitive dependencies, but more about the fact that the transitive dependencies are not something one should depend on in the first place: in both the scenarios I described above, if changes in the libraries' versions result in changes to the transitive dependencies one should not need to update all their BUILD files to reflect that if they weren't interested in the transitive dependencies in the first place, and only had to add them to work around this missing feature.

from rules_jvm_external.

I think this is required for scala support: the way the types work means that all libraries need to be compiled with symbols from transitive dependencies available during compilation.

from rules_jvm_external.

I have a PR ready to go, but I'd like to get some folks to look at it before I can submit it. If you beat me to it, please go for it!

from rules_jvm_external.

Is there any update on this?

One thought would be to auto-emit a synthetic target that does re-export all the deps, alongside the existing jvm_import targets. Perhaps something with an _with_exports suffix on the name? This way, existing users and java consumers worried about classpath lengths aren't affected, but scala users can still make reasonable use of rules_jvm_external without the pain described up-thread?

from rules_jvm_external.

I actually had the opposite reaction, and thought Jin's idea was a great way to enable the use case while containing the blast radius. Seems easier to start contained and then go global if that's still needed, than to go the other way.

from rules_jvm_external.

I think there is a use case for both, especially depending on whether you use a single maven_install rule or multiple ones.

1. If you use a single one, then per artifact exporting sounds better: as mentioned above there are specific artifacts that are meant to export a whole umbrella of artifacts, for example selenium-java, and those can be attributed as such.
2. If you use multiple maven_install rules, a global setting to control it sounds better: for example if you have multiple applications, each with their own maven_install and some of them are scala applications, those will want all their dependencies to export transitive dependencies.

In our case we have a mix of both: we have a single maven_install rule used by many applications, but we also have additional maven_install rules and bazel-deps setups that are used by one or two applications each, so we would like to have both options.

from rules_jvm_external.

As an opt-in feature, this is great. There are plenty of libraries that people depend on via Maven that are designed to bring in others transitively (for example, Selenium's selenium-java package, which gives access to a whole heap of magic)

Ideally, this could be configured at the repository level (if I'm pulling from Maven Central, I want the default maven behaviour, but if I'm pulling from a local maven repo maybe something more disciplined makes sense) At the artifact level would end up being super-noisy in the common case.

from rules_jvm_external.

Should we have this as an opt-in feature per artifact? That is,

maven_install(
    artifacts = [
        maven.artifact("group.id", "artifact", "1.2.3", export_transitive_compile_deps = True),
    ],
)

instead of applying this across the entire graph. So, for the following example,

we would place

into exports instead of deps of the generated aar_import target.

from rules_jvm_external.

@shs96c what do you think? I think this is a more controllable approach than a maven_install wide implementation.

from rules_jvm_external.

bazel-deps handles this at a global level, and that seems to work well in the places that I've seen it used. I think a first pass should have this as a global setting, and then we should wait for user feedback before enabling it per dependency.

No matter what happens, the global setting will be needed anyway to define the default approach for whether or not dependencies are exported.

from rules_jvm_external.

Making it at a global level will most likely to cause most targets to depend on a much, much larger compile time classpath than they actually require.

e.g.

brings in 19 other jars.

With this, I think the default behavior should be opt-in at the local artifact level to help keep track of classpath bloat per top level artifact, instead of bloating up the entire dependency tree for artifacts that don't necessarily need it.

from rules_jvm_external.

I'm happy to take a shot at this over the weekend, unless anyone else is already on it, @shs96c?

from rules_jvm_external.

It's also a bit horrifying to me that scala actually requires this, but I can also see the case for enabling it for scala users' use. But with the appropriate caveats.

from rules_jvm_external.

If users will forgo the benefits of your tool because they insist on this convenience, then I'd rather they use the tool. But still, I would advise that it be recommended against in the strongest terms.

@cgruber, I totally agree with everything you said. This above was really the crux of it for me, and I've always been on the same page. Nonetheless, if you want to get people to move in a positive direction, it's something that unfortunately falls on the less popular tools to provide a pleasant ramp, otherwise it's just another massive barrier to entry. It's incredibly frustrating to be faced with a preexisting mess that you wisely avoided, and still have it be your problem.

The one thing I guess this makes me think is: are we thinking too small when we talk about making the build tool optionally accept the pernicious behavior? I know I have certainly been thinking this way. Would it be better if there was a tool that helped isolate the deps, something git-bisect-like or some other way to do it? I don't know if that's possible, but it's made me want to think about it a bit.

from rules_jvm_external.

That's a separate matter, but, yes, bazel should be helping people far more. The IJ plugin should be doing things like suggesting new dependencies, and highlighting unused dependencies. The behaviour of [unused-deps](https://github.com/bazelbuild/buildtools/tree/master/unused_deps) should be a build flag rather than a separate tool. The suggestions from bazel build should trace visibility too (so that people aren't told to rely on targets they'd need to change the visibility of)

from rules_jvm_external.

I ran into this just yesterday (cf. Issue #275).

One note I would like to make, is that prominent projects actually perpetuate the issue by instructing users to implicitly rely on the transitive nature of maven.

For example:

SLF4J instruct in their user manual to just depend on ch.qos.logback:logback-classic which will pull-in org.slf4j:slf4j-api as a transitive dependency.

And scala-logging gives the same instructions. Probably by following the advise from SLF4J's user manual.

I have no strong opinion on implementing this on a maven_install level, or on a per artifact level. Per-artifact will result in cleaner dependency graphs. But I forecast people stumbling on this issue who have difficulties in getting this right on a per-artifact level as it may require reverse-engineering the build setup of an external project. And official documentation can be misleading. So providing this on a maven_install level, while being a bit of a sledge hammer approach, may be a good idea to reduce support costs.

Most importantly, I would like this to be documented and highlighted well enough. Perhaps in a distinct section on differences to Maven.

from rules_jvm_external.

With #285, auto-suggestions for buildozer's add dep feature now works with artifacts pulled in by maven_install. That is, Bazel's strict-java-deps plugin will report this error when using a class from a non-direct dep:

$ bazel build //:B_no_dep
INFO: Writing tracer profile to '/private/var/tmp/_bazel_jingwen/ac10292842231f27a13ff5ac097aa7a7/command.profile.gz'
INFO: Analyzed target //:B_no_dep (1 packages loaded, 14 targets configured).
INFO: Found 1 target...
ERROR: /Users/jingwen/code/rules_jvm_demo/rules_jvm/BUILD:16:1: Building libB_no_dep.jar (1 source file) failed (Exit 1)
B.java:6: error: [strict] Using type com.google.common.collect.Lists from an indirect dependency (TOOL_INFO: "@maven//:com_google_guava_guava"). See command below **
        System.err.println(Lists.reverse(A.INTEGERS));
                           ^
 ** Please add the following dependencies:
  @maven//:com_google_guava_guava to //:B_no_dep
 ** You can use the following buildozer command:
buildozer 'add deps @maven//:com_google_guava_guava' //:B_no_dep

Target //:B_no_dep failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 6.599s, Critical Path: 0.17s
INFO: 0 processes.
FAILED: Build did NOT complete successfully

This is also integrated into the IntelliJ plugin as a clickable action:

I'm leaning towards closing this issue and not allowing transitive exports in favor of buildozer suggestions like the above. This keeps the build graph minimal and isn't tedious to maintain.

from rules_jvm_external.

@jin Anything to update regarding this issue, specifically concerning Scala compile-classpath issues.? @shs96c you had a PR for this yeah?

from rules_jvm_external.

Does it break expectations from library authors when transitive dependencies aren't exposed in the compile classpath for compile-scoped Maven publications?

For example, if a library author publishes a library with Gradle, which provides this guidance:

Dependencies appearing in the api configurations will be transitively exposed to consumers of the library, and as such will appear on the compile classpath of consumers. Dependencies found in the implementation configuration will, on the other hand, not be exposed to consumers, and therefore not leak into the consumers' compile classpath.
...
An API dependency is one that contains at least one type that is exposed in the library binary interface, often referred to as its ABI (Application Binary Interface). This includes, but is not limited to:

• types used in super classes or interfaces
• types used in public method parameters, including generic parameter types (where public is something that is visible to compilers. I.e. , public, protected and package private members in the Java world)
• types used in public fields
• public annotation types

then it would be surprising for the library author to find that bazel doesn't honor this.

As a more concrete example (published with sbt instead of gradle, but still under POM semantics), scalapb-runtime 0.9.4 has a compile-scoped dependency on com.thesamet.scalapb:lenses because the scalapb-runtime class FileDescriptorProto extends scalapb.lenses.Updatable.

Without the lenses library in the compile classpath, bazel fails with:

ERROR: BUILD:91212:1: scala //:example_scala failed (Exit 1)
bazel-out/.../Example.scala:21: error: Symbol 'type scalapb.lenses.Updatable' is missing from the classpath.
This symbol is required by 'class com.google.protobuf.descriptor.FileDescriptorProto'.
Make sure that type Updatable is in your classpath and check for conflicting dependencies with `-Ylog-classpath`.
A full rebuild may help if 'FileDescriptorProto.class' was compiled against an incompatible version of scalapb.lenses.
    val scalaProto = com.google.protobuf.descriptor.FileDescriptorProto.parseFrom(ProtoBytes)
                     ^

from rules_jvm_external.

Referring to Uri's point, I'm not sure I follow. jvm_import exposes a correctly formed JavaInfo, and that provides access to the entire transitive classpath Doesn't rules_scala pass that to scalac? If not, surely this same problem occurs when --strict_java_deps is enabled?

from rules_jvm_external.

@shs96c It's not that the classpath doesn't contain the transitive dependencies, it's that if the compiler loads them --strict_java_deps will fail the build.

from rules_jvm_external.