Always pass the transitive classpath to the compiler for scala_library #423

pauldraper · 2018-02-15T17:13:13Z

java_library always includes transitive dependencies, regardless of strict deps.

Bazel always passes the entire transitive classpath to javac, not only the direct classpath. This frees the user from having to aggregate their transitive dependencies manually. In other words, javac never fails because of a missing symbol, as long as every rule specifies its direct dependencies.
https://blog.bazel.build/2017/06/28/sjd-unused_deps.html

I think scala_library should work similarly.

And unless I am mistaken, scalac more or less requires essentially the full transitive classpath. There seems to be little practical benefit in ever not including the transitive dependencies.

(FYI, tangentially: The reason I am not just using strict deps anyway is because I don't care enough about its guarantees. Code modifications can break downstream targets. I'm okay with dependency modifications breaking downstream targets.)

Matches behavior of java_library

bazel-io · 2018-02-15T17:13:16Z

Can one of the admins verify this patch?

johnynek · 2018-02-15T17:23:39Z

if you do these, which I am pretty opposed to, understand if any transitive jar changes, you will rebuild your whole repo.

Coupled with the fact that scala does not work great with ijar (lots of private things can change and still change the ijar), I think this will be a miserable experience for a large repo.

I really feel it can't be the default without any options.

Why don't you use strict deps in warn mode? Doesn't that get you want you want right now?

johnynek · 2018-02-15T17:23:44Z

cc @ittaiz

johnynek · 2018-02-15T17:27:31Z

Here is an old issue on where we are with ijar:

#125

we really need to have a better scala-aware ijar, or bazel needs to allow customized comparators/key-functions to see if an artifact has changed.

pauldraper · 2018-02-15T17:47:36Z

Why don't you use strict deps in warn mode? Doesn't that get you want you want right now?

Yeah. Except for a bunch of warnings.

understand if any transitive jar changes, you will rebuild your whole repo.

Yes, just as it does for Scala with strict deps now. And same for Java, with or without strict deps. (True, Java has better ijar, but still...adding a class or non-private method still triggers a massive recompile.)

I think this will be a miserable experience for a large repo.

Do you see that your transitive path differs substantially from the minimal one? My experience was that it was a < 10% difference in practice. But if there actually are significant real-world benefits, I agree transitivity needs to be optional.

I really feel it can't be the default without any options.

Okay. We could overload --strict_java_deps, which has 5 modes default, strict, off, warn, error. IDK if that is a good idea though.

An attribute would probably(?) be better.

Here is an old issue on where we are with ijar:

Hm, good to know. Seems like all method signatures are kept but implementations are removed? Improving that would be great, but TBH it at first glance it doesn't seem awful. I use TypeScript, which keeps private signatures in its .d.ts files (unavoidable, because it doesn't name mangle members and needs to ensure uniqueness.)

johnynek · 2018-02-15T19:22:39Z

ijar leaves the scalasignature fully intact. It includes some private information. So, I fear basically every change will be a full recompile if you ship this.
while ijar is not prefect, consider A depends on B depends on C. With a bad ijar, C changes, and we needlessly recompile B, but since C is not on the classpath for A, and scalac almost always (macros excepted) is reproducible, A is not rebuilt. If we ship this, everything downstream of C is rebuilt.
It is very anti-bazel philosophy to depend on targets implicitly. While you can make the skylark do whatever you want, having worked in a big repo that worked that way (pants used to), it is terrible. You make a minor build refactor and 1000 targets fail because they were depending on something they didn't declare. It also hampers the ability to do bazel query.

I think users in your situation should use strict deps in warn mode, and use builddozer to fix your build if the warnings bother you. If you don't want this, I'm a bit confused why you want to use bazel at all.

pauldraper · 2018-02-16T04:44:53Z

while ijar is not prefect, consider A depends on B depends on C. With a bad ijar, C changes, and we needlessly recompile B, but since C is not on the classpath for A

I understand the theory of direct vs transitive compile-time dependencies. The question is: how much does this happen much in practice? Where compiling A requires B, and compiling B requires C, but compiling A does not require C.

For javac at least, it is (apparently?) uncommon enough, that rules_java always includes the transitive path.

If you don't want this, I'm a bit confused why you want to use bazel at all.

You could ask the implementers of java_library the same thing.

At the end of the day, I want Scala to be able to work the same as --strict_java_deps=off Java. I'd prefer it to work the same by default. But I could add an attribute to make it work the same.

johnynek · 2018-02-16T20:48:05Z

@pauldraper it is not the case that you don't get an error with java. Have you tried it?

https://gist.github.com/johnynek/857579dfd3ce0b1c437826f496448e16

st-oscar1:rules_scala oscar$ bazel build c
INFO: Analysed target //:c (0 packages loaded).
INFO: Found 1 target...
ERROR: /Users/oscar/oss/rules_scala/BUILD:10:1: Building libc.jar (1 source file) failed (Exit 1): java failed: error executing command
  (cd /private/var/tmp/_bazel_oscar/cff0ec2cf472ebc5cb4402685dc7edf3/execroot/io_bazel_rules_scala && \
  exec env - \
    LC_CTYPE=en_US.UTF-8 \
  external/local_jdk/bin/java -XX:+TieredCompilation '-XX:TieredStopAtLevel=1' -Xbootclasspath/p:external/bazel_tools/third_party/java/jdk/langtools/javac-9-dev-r4023-3.jar -jar external/bazel_tools/tools/jdk/JavaBuilder_deploy.jar @bazel-out/darwin-fastbuild/bin/libc.jar-2.params)
C.java:5: error: [strict] Using type test.A from an indirect dependency (TOOL_INFO: "//:a"). See command below **
  public C(B b, A a) {
                ^
 ** Please add the following dependencies:
  //:a  to //:c
 ** You can use the following buildozer command:
buildozer 'add deps //:a ' //:c

Target //:c failed to build

the build fails without strict deps by default as far as I know. Can you show an example with java that behaves as you want it to behave in scala?

pauldraper · 2018-02-16T23:26:17Z

// BUILD
java_library(
  name = 'a',
  srcs = ['A.java'],
)
java_library(
  deps = ['a'],
  name = 'b',
  srcs = ['B.java'],
)
java_library(
  deps = ['b'],
  name = 'c',
  srcs = ['C.java'],
)

// A.java
package example;
class A {}

// B.java
package example;
class B {
    private A a = new A();
}

// C.java
package example;
class C {
    private A a = new A();
    private B b = new B();
}

// tools/bazel.rc
build --strict_java_deps=OFF

$ bazel build c
INFO: Analysed target //:c (9 packages loaded).
INFO: Found 1 target...
INFO: From Executing genrule @bazel_tools//tools/jdk:gen_platformclasspath [for host]:
Note: external/bazel_tools/tools/jdk/DumpPlatformClassPath.java uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
Target //:c up-to-date:
  bazel-bin/libc.jar
INFO: Elapsed time: 4.525s, Critical Path: 3.93s
INFO: Build completed successfully, 12 total action

johnynek · 2018-02-17T00:26:05Z

Can you share your bazelrc of baze version? Why do I get different behavior I wonder?

On Fri, Feb 16, 2018 at 13:26 Paul Draper ***@***.***> wrote: BUILD java_library( name = 'a', srcs = ['A.java'], ) java_library( deps = ['a'], name = 'b', srcs = ['B.java'], ) java_library( deps = ['b'], name = 'c', srcs = ['C.java'], ) // A.java package example; class A {} // B.java package example; class B { private A a = new A(); } // C.java package example; class C { private A a = new A(); private B b = new B(); } $ bazel build c INFO: Analysed target //:c (9 packages loaded). INFO: Found 1 target... INFO: From Executing genrule @bazel_tools//tools/jdk:gen_platformclasspath [for host]: Note: external/bazel_tools/tools/jdk/DumpPlatformClassPath.java uses unchecked or unsafe operations. Note: Recompile with -Xlint:unchecked for details. Target //:c up-to-date: bazel-bin/libc.jar INFO: Elapsed time: 4.525s, Critical Path: 3.93s INFO: Build completed successfully, 12 total action — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#423 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEJdjpOK6lOldDY_Bc0OxSbJ2e3sWMAks5tVg6ZgaJpZM4SHMXC> .

-- P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

johnynek · 2018-02-17T02:12:54Z

When I try your example I get:

st-oscar1:rules_scala oscar$ bazel build //:c
INFO: Analysed target //:c (0 packages loaded).
INFO: Found 1 target...
ERROR: /Users/oscar/oss/rules_scala/BUILD:10:1: Building libc.jar (1 source file) failed (Exit 1): java failed: error executing command
  (cd /private/var/tmp/_bazel_oscar/cff0ec2cf472ebc5cb4402685dc7edf3/execroot/io_bazel_rules_scala && \
  exec env - \
    LC_CTYPE=en_US.UTF-8 \
  external/local_jdk/bin/java -XX:+TieredCompilation '-XX:TieredStopAtLevel=1' -Xbootclasspath/p:external/bazel_tools/third_party/java/jdk/langtools/javac-9-dev-r4023-3.jar -jar external/bazel_tools/tools/jdk/JavaBuilder_deploy.jar @bazel-out/darwin-fastbuild/bin/libc.jar-2.params)
C.java:4: error: [strict] Using type test.A from an indirect dependency (TOOL_INFO: "//:a"). See command below **
  private A myA = new A();
          ^
 ** Please add the following dependencies:
  //:a  to //:c
 ** You can use the following buildozer command:
buildozer 'add deps //:a ' //:c

Target //:c failed to build
INFO: Elapsed time: 0.335s, Critical Path: 0.12s
FAILED: Build did NOT complete successfully

The same kind of error I get.

Here is my bazel version:

Build label: 0.9.0-homebrew
Build target: bazel-out/darwin-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Sun Jul 12 03:29:57 +49936 (1513677382197)
Build timestamp: 1513677382197
Build timestamp as int: 1513677382197

this is with no .bazelrc.

Are you maybe setting an option in a bazelrc somewhere?

pauldraper · 2018-02-17T03:11:50Z

Sorry, I added the tools/bazel.rc after initially posting.

So basically, this "issue" comes down to the fact that for Java java_strict_deps does not affect transitivity, and for Scala it does.

I think the right, expected behavior would be Java's, by default. But I would be satisfied at least being able to have that behavior enabable by attribute.

ittaiz · 2018-02-17T06:28:03Z

I think practically there are two issues:

rules_scala has a different semantics for “off”- I think we can definitely change that to match Java’s setting.
rules_scala uses DEFAULT to mean “no transitivity” while java means “error”. I want to change that DEFAULT but it sounds like we need to then have a value that says “no transitivity”. I wonder if that’s the implementation of “strict”. Paul can you maybe check?
WDYT about adding this to the toolchain?

johnynek · 2018-02-17T17:34:18Z

I think making the default, unconfigured way of operating match java is mostly fine. I do think there is a practical issue that java compiles much faster and ijar works much better for it.

I do think we need an option to preserve the current settings.

Lastly, I wonder if we need to work on making a scala specific ijar-like tool. Really something that processes the scala-signature of a jar before sending to ijar. If we could remove some of the unneeded things maybe we would have less recompilation.

It's tricky from what I hear.

ittaiz · 2018-02-17T17:43:15Z

Good. Paul, If you can confirm or refute what “strict” means in java strict deps that would be really helpful. If strict means the current Scala settings (and it’s not deprecated) then it’s easy to change the default to error and add a note for people wanting the old behavior to use “strict”. If strict is an alias of error or is deprecated then I think we need to open an issue for Bazel itself. Re a scala specific tool for ijar- that sounds awesome but we (Wix) won’t have the ability to build it in the near future

…

On Sat, 17 Feb 2018 at 19:34 P. Oscar Boykin ***@***.***> wrote: I think making the default, unconfigured way of operating match java is mostly fine. I do think there is a practical issue that java compiles much faster and ijar works much better for it. I do think we need an option to preserve the current settings. Lastly, I wonder if we need to work on making a scala specific ijar-like tool. Really something that processes the scala-signature of a jar before sending to ijar. If we could remove some of the unneeded things maybe we would have less recompilation. It's tricky from what I hear. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#423 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIFy2m3UlrPcrDKBv_GrKyxioz3s51ks5tVw2bgaJpZM4SHMXC> .

pauldraper · 2018-02-18T20:47:48Z

--strict_java_deps strict, default, and error all have the same behavior https://github.com/bazelbuild/bazel/blob/70c2e051e9b8db92228845b04d10b570ded8bfb7/src/main/java/com/google/devtools/build/lib/rules/java/JavaConfiguration.java#L371 (also, in bazel docs)

IDK what the history or plans are for this.

If we follow java's use of "off", we could use "strict" to retain current intransivity (this admittedly being different than Java "strict")

Or we could use a rule attribute to set intransitivity.

Separately, I agree scala-ijar needs a look. I'll take a look over the next couple days and report what I find.

gkossakowski · 2018-02-19T22:59:58Z

paul, happy to give you pointers for understandign the issues with scala's pickling (or api extraction) if you're interested
I dealt with it when I was working on redesign of zinc

ittaiz · 2018-02-22T07:46:39Z

@pauldraper any chance you can open a bazel side issue for adding a new value "non-transitive" which we'll be able to leverage here? I'd really rather not overload "strict" though I'm not vetoing this proposal

johnynek · 2018-08-28T21:52:33Z

Seems like @pauldraper has an alternative repo supporting this approach.

I my personal goal is to make the default very cache efficient, but if you don't like that, you can use the strict deps setting that would enable this for users that don't care about cache hit rates as much as large users do.

Always pass the transitive classpath to the compiler for scala_library

f50a4a5

Matches behavior of java_library

googlebot added the cla: yes label Feb 15, 2018

ittaiz mentioned this pull request Mar 10, 2018

[Feature Request] Add a new "non-transitive" java strict deps enum value bazelbuild/bazel#4821

Closed

johnynek closed this Aug 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always pass the transitive classpath to the compiler for scala_library #423

Always pass the transitive classpath to the compiler for scala_library #423

pauldraper commented Feb 15, 2018 •

edited

Loading

bazel-io commented Feb 15, 2018

johnynek commented Feb 15, 2018

johnynek commented Feb 15, 2018

johnynek commented Feb 15, 2018

pauldraper commented Feb 15, 2018 •

edited

Loading

johnynek commented Feb 15, 2018

pauldraper commented Feb 16, 2018 •

edited

Loading

johnynek commented Feb 16, 2018

pauldraper commented Feb 16, 2018 •

edited

Loading

johnynek commented Feb 17, 2018 via email

johnynek commented Feb 17, 2018

pauldraper commented Feb 17, 2018

ittaiz commented Feb 17, 2018

johnynek commented Feb 17, 2018

ittaiz commented Feb 17, 2018 via email

pauldraper commented Feb 18, 2018 •

edited

Loading

gkossakowski commented Feb 19, 2018

ittaiz commented Feb 22, 2018

johnynek commented Aug 28, 2018

Always pass the transitive classpath to the compiler for scala_library #423

Always pass the transitive classpath to the compiler for scala_library #423

Conversation

pauldraper commented Feb 15, 2018 • edited Loading

bazel-io commented Feb 15, 2018

johnynek commented Feb 15, 2018

johnynek commented Feb 15, 2018

johnynek commented Feb 15, 2018

pauldraper commented Feb 15, 2018 • edited Loading

johnynek commented Feb 15, 2018

pauldraper commented Feb 16, 2018 • edited Loading

johnynek commented Feb 16, 2018

pauldraper commented Feb 16, 2018 • edited Loading

johnynek commented Feb 17, 2018 via email

johnynek commented Feb 17, 2018

pauldraper commented Feb 17, 2018

ittaiz commented Feb 17, 2018

johnynek commented Feb 17, 2018

ittaiz commented Feb 17, 2018 via email

pauldraper commented Feb 18, 2018 • edited Loading

gkossakowski commented Feb 19, 2018

ittaiz commented Feb 22, 2018

johnynek commented Aug 28, 2018

pauldraper commented Feb 15, 2018 •

edited

Loading

pauldraper commented Feb 15, 2018 •

edited

Loading

pauldraper commented Feb 16, 2018 •

edited

Loading

pauldraper commented Feb 16, 2018 •

edited

Loading

pauldraper commented Feb 18, 2018 •

edited

Loading