Generated BUILD files should use java_import instead of java_library

[cgrushko]

That is, instead of

java_library(
    name = "args4j",
    exports = [
        "//external:jar/args4j/args4j"
    ],
    runtime_deps =[
              <dependencies of args4j, if any>
    ],
    visibility = [
        "//visibility:public"
    ]
)

we should have

java_import(
    name = "args4j",
    jars = [
        "@args4j_args4j//jar:file"  # <--- this is a filegroup generated by maven_jar
    ],
    deps = [
        <dependencies of args4j, if any>
    ],
    visibility = [
        "//visibility:public"
    ]
)

The reason is that javac sometimes needs to access symbols from the dependencies of a jar during a compilation. I can come up with an exact explanation, but waving my hands, one example is using an interface defined in the superclass of your superclass.

Using runtime_deps will result in a broken build in this case.

(you can ask bazel-deps to put everything in exports, but this hurts readability and maintainability, and I recommend against it)

1. What says you?
2. How does this interact with Scala?

[johnynek]

I think you are advocating for the transitivity: runtime_deps option:

https://github.com/johnynek/bazel-deps#options

It is intentional that we assume runtime_deps to keep deps minimal and rebuilds minimal. We add exports: [] to our yaml files when that is inappropriate (i.e. you could use the library A without ever touching library B, but library A cannot ever be used without B on the classpath due to where B appears on A's API.

I don't see this as something that should be changed currently.

(Note, we are using this tool in production at Stripe in several repos, and it seems to be working rather well for us. Can you try one of the two proposals above?)

[cgrushko]

It is intentional that we assume runtime_deps to keep deps minimal and rebuilds minimal.

Huh, I guess this connects back to bazelbuild/rules_scala#235, where scala_library would originally take the exact jars it needs to compile, and would not include the transitive jars in the classpath (in contrast to the native java_library)

It's true that a longer classpath makes longer builds, but is it significant in the case of Maven dependencies? I thought it needs to be hundreds / thousands of unnecessary jars to make a difference.

Either way, I think a better user experience is to free the developer from having to add things manually to exports until the build succeeds.

Are you open to a third exports mode, that behaves as in my proposal?

[ittaiz]

@johnynek using deps instead of runtime_deps will indeed trigger more compilations if changes of that dependency propagate over the ijar. Other than that it should be the same since:

1. the transitive dependencies (other than ones containing macros) are used as ijars.
2. Running Tests is already triggered in your current mechanism since the transitive jars are an input to the run action.
   On the value side we get much better fluency since in our experience the issues Carmi described happen frequently and are hard to debug since they are usually due to the compiler's whim and not to the code the developer sees.

As you know that's why we've been putting so much effort into bringing this to rules_scala.
Btw, the scalac errors are even more vague and widespread than javac so it's more important there :)

[johnynek]

@cgrushko how is your proposal different than transitivity: exports. I'm not clear on that. Isn't that what you want?

[cgrushko]

No; java_library.exports allows you to compile your code without deps-ing on the right things (it violates strict java deps, as defined in https://blog.bazel.build/2017/06/28/sjd-unused_deps.html).

I want to require users to deps on what they import, but not force them to deps on what they don't import.

[ittaiz]

Carmi by "what they import" you mean what they actually use in their sources as opposed to arbitrary needs of the compiler, right?

…

On Wed, 6 Sep 2017 at 23:42 cgrushko ***@***.***> wrote: No; java_library.exports allows you to compile your code without depending on the right things (it violates strict java deps, as defined in https://blog.bazel.build/2017/06/28/sjd-unused_deps.html). I want to require the user to depend on what they import, but not force them to depend on what they don't import (here, "depend" means have an entry in their deps attributes) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#63 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF2s0Go9FE3KkkqSUAfPFu-bkWCB7ks5sfwOsgaJpZM4PNncl> .

[johnynek]

So, I don't see what change you expect using java_import vs java_library to make. I don't see any behavior we can get by using java_library with runtime_deps vs using java_import with deps (something I didn't even know was supported, and am not sure it is, I think it may error that you can't have deps without src).

[cgrushko]

@ittaiz yep, I use "import" as a simplification.

@johnynek exports circumvent strict java deps, while runtime_deps don't participate in compilation. On the other hand, java_import with deps doesn't circumvent strict java deps, while still participating in compilation.
It's the same difference between using exports and deps on a java_library.

something I didn't even know was supported, and am not sure it is, I think it may error that you can't have deps without src

This is not a requirement for java_import, which doesn't have a srcs attribute.

I can construct a demo project, but it'll take me some time, so I prefer doing this as a last resort.

[johnynek]

So, I'm happy to take a PR that replaces java_import as long as both of the modes work the same and it shouldn't break bits.

[tekumara]

Perhaps this can be closed in favour of #102?

[johnynek]

This one was actually first m, it seems.

@tekumara this should not be too much work. If you want to try a PR I am happy to review or answer questions.