add plus one deps transitive mode for compilation #698

ittaiz · 2019-02-21T18:12:22Z

Tries to balance between sternness of strict-deps off and strict-deps on cache issues and false positives.
We, Wix, have been using strict-deps=error for a long time and have been enjoying the relative ease a developer has when needing to add a new dependency.
However the current strict-deps compiler plugin has a serious drawback in generating too many false positives (causing errors about dependencies which aren't really used because of the compiler eagerness). This is a much bigger issue for us then the cache hits (which we also care about) since this means that:

Builds on master fail from seemingly innocent changes. This is because we have many repositories in a logical head dependency.
Additionally this causes developer fatigue which just try to "please the beast".

OTOH using strict-deps off has similar issues of easily breaking builds. For examples you can see the tests I added and also this scala-center discussion.

This PR adds a new heuristic mode which I hope will strike a better balance between the two. It passes to the compiler all direct dependencies and their direct dependencies since very often this is needed by the compiler. This was also confirmed by offline discussions with the google java people.

We also hope in the future to add some kind of post processing action (inside the bazel build) which might work over the byte code or source code to find strict-deps infringements of these plus one deps.
Additionally we have an internal tool in beta mode which goes over the compiler output and via various analyzers (mainly regex based) finds the needed FQN and then looks them up in bazel-out, external binary dependencies as well as a global index we have for our own source code.
This tool is run outside of bazel by the user.
We hope to open source this tool as well as the global index server API.

tries to balance between sterness of strict-deps off and strict-deps on cache issues and false positives

ittaiz · 2019-02-21T18:14:33Z

btw, I really wanted to refrain from adding the aspect to the deps attribute but rather run the aspect from command line but unfortunately this behaved very differently.

johnynek · 2019-02-21T18:26:32Z

@ittaiz can you comment more or share any links about discussions with the google folks? Can you give some references: is this how it works at Google internally, do the standard java rules work this way?

I'd really like to be moving in the other way: less configuration. Adding a new compilation mode adds complexity and tends to cut the effectiveness of our tests since we generally are not multiplicatively expanding our tests to check all configurations.

I wouldn't want to keep inviting new heuristics for people to bolt into the rules that each company thinks is a good solution to the problem. I'd rather push the concerns upstream to scalac where possible, and follow the java standard rules as much as possible.

ittaiz · 2019-02-21T18:55:04Z

the discussions were offline with @cushon.
AFAIU the java rules work similarly to this but not exactly. They add all of the dependencies that were used in the previous layer.
Example:
A->B->(C,D) and D isn't used then AFAIU they will compile A with B and C.

In general the java rules are much more robust (and complicated) since they have more time/people/history behind it.

I agree I'd like less configuration but I have to say that the current default is mainly because it's what Stripe uses and Stripe started the project. It doesn't mean direct-only is the right mode and I'll even add that all of the google people I talked to thought it's unreasonable (see this discussion for a bit info).

Adding things to scalac might be a good long term goal but we can't hold our breath for it.

I'll add that as far as we're concerned we can definitely discuss removing the current implementation of strict-deps

ittaiz · 2019-02-21T19:05:29Z

I’ll also add that e2e indeed aren’t multiplied but the regular test/... are multiplied

Lastly I’d encourage many different approaches from different companies in hopes we’ll find a good one. From my examples you can see the direct mode breaks on simple usecases. People can get used to adding transitive dependencies without understanding but I don’t think it’s something we want to aspire to you.

ittaiz · 2019-02-21T20:23:52Z

I remembered one more thing about java rules- AFAIR they have one action which gets all of the transitive classpath to it but the differences from our current strict deps approach are:

They try to compile with the +1 variation I explained above and if fail they compile with all of it (as a single bazel action)
Their strict deps analysis mechanism is vastly superior to ours due to many reasons (javac/java simpler, conventions enforced like no wildcards imports, a lot of time/people/experience)

ittaiz · 2019-02-21T20:24:48Z

Closed by mistake

ittaiz · 2019-02-22T04:19:20Z

@johnynek ?

johnynek

a couple of questions, but I'm mostly positive here.

.travis.yml

scala/private/common.bzl

johnynek · 2019-02-22T17:53:00Z

scala/private/rule_impls.bzl

@@ -1006,36 +1013,6 @@ def scala_test_impl(ctx):
        jars.jars2labels,
    )

-    # _scalatest is an http_jar, so its compile jar is run through ijar


why is this all removed now? Is this because we shouldn't depend on a test anyway?

Good question.
I have an answer which I’m fairly confident about but unfortunately not 100%.
The answer is that since I added the scalatest label as part of the base classpath it’s jars and labels are collected in the main flow.
I have to say that no tests failed even before I did this so I think this area of scalatest and integration with strict-deps/unused deps isn’t tested well.

Lastly tests can depend on tests, not a problem also in java.

test_rules_scala.sh

ittaiz · 2019-02-22T21:16:39Z

@johnynek replied to everything. Feel free to reopen if you feel I've missed something.
Also not 100% sure about the documentation I added so open for feedback.

ittaiz · 2019-02-25T05:56:49Z

@johnynek wdyt? Also the breakage is due to jmh reproducibility flakiness

johnynek · 2019-02-25T17:26:15Z

@ittaiz there was a recent change the jmh rules: #695 I wonder if that could be causing the issue.

johnynek

Thanks for working on this, @ittaiz

scala/plusone.bzl

johnynek · 2019-02-25T17:32:41Z

scala/private/common.bzl

@@ -22,21 +23,25 @@ def collect_srcjars(targets):
 def collect_jars(
        dep_targets,
        dependency_analyzer_is_off = True,
-        unused_dependency_checker_is_off = True):
+        unused_dependency_checker_is_off = True,
+        plus_one_deps_is_off = True):


I wonder if we should have so many default values here. especially with the same type... but we can punt that concern I guess.

I definitely agree...
I also thought about it and the worst thing is that they are all negative case.
I think we still need to do a lot of refactoring here but it's hard to keep up, fix bugs, refactor while also doing all of the other d2d stuff at work...
So thank you also for taking a bigger part of the work of this repo. I hope to remove some of my responsibilities in the next few months so I can give some TLC back here (and maybe also get 1-2 people from Wix to work here some more)

johnynek · 2019-02-25T17:34:23Z

scala/private/common.bzl

    return struct(
        compile_jars = depset(transitive = compile_jars),
        transitive_runtime_jars = depset(transitive = runtime_jars),
        jars2labels = JarsToLabelsInfo(jars_to_labels = jars2labels),
-        transitive_compile_jars = depset(),


huh, this is interesting. This is a change from before. We weren't really tracking transitive compile jars?

if strict deps was off we didn't track it

ittaiz · 2019-02-25T17:45:26Z

re recent change in jmh- don't think it's related since my branch predates it

johnynek · 2019-02-25T17:52:56Z

👍

add plus one deps transitive mode for compilation

16a6d8b

tries to balance between sterness of strict-deps off and strict-deps on cache issues and false positives

ittaiz requested a review from johnynek as a code owner February 21, 2019 18:12

googlebot added the cla: yes label Feb 21, 2019

Update .travis.yml

66ac5aa

ittaiz closed this Feb 21, 2019

ittaiz reopened this Feb 21, 2019

johnynek reviewed Feb 22, 2019

View reviewed changes

ittaiz added 4 commits February 22, 2019 22:01

move plusone provider and aspect to separate file

b7979ba

try 17 again after moving out provider

a530f8b

move configuration from define to attribute on toolchain

a7e96df

added documentation for the plusone provider

55ded3a

johnynek approved these changes Feb 25, 2019

View reviewed changes

remove trailing 9

cb68338

ittaiz merged commit 7ea9079 into bazelbuild:master Feb 25, 2019

Jamie5 mentioned this pull request Feb 7, 2020

Ignore base class annotation #991

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add plus one deps transitive mode for compilation #698

add plus one deps transitive mode for compilation #698

ittaiz commented Feb 21, 2019

ittaiz commented Feb 21, 2019

johnynek commented Feb 21, 2019

ittaiz commented Feb 21, 2019 •

edited

Loading

ittaiz commented Feb 21, 2019

ittaiz commented Feb 21, 2019 •

edited

Loading

ittaiz commented Feb 21, 2019

ittaiz commented Feb 22, 2019

johnynek left a comment

johnynek Feb 22, 2019

ittaiz Feb 22, 2019

ittaiz commented Feb 22, 2019

ittaiz commented Feb 25, 2019

johnynek commented Feb 25, 2019

johnynek left a comment

johnynek Feb 25, 2019

ittaiz Feb 25, 2019

johnynek Feb 25, 2019

ittaiz Feb 25, 2019

ittaiz commented Feb 25, 2019

johnynek commented Feb 25, 2019

add plus one deps transitive mode for compilation #698

add plus one deps transitive mode for compilation #698

Conversation

ittaiz commented Feb 21, 2019

ittaiz commented Feb 21, 2019

johnynek commented Feb 21, 2019

ittaiz commented Feb 21, 2019 • edited Loading

ittaiz commented Feb 21, 2019

ittaiz commented Feb 21, 2019 • edited Loading

ittaiz commented Feb 21, 2019

ittaiz commented Feb 22, 2019

johnynek left a comment

Choose a reason for hiding this comment

johnynek Feb 22, 2019

Choose a reason for hiding this comment

ittaiz Feb 22, 2019

Choose a reason for hiding this comment

ittaiz commented Feb 22, 2019

ittaiz commented Feb 25, 2019

johnynek commented Feb 25, 2019

johnynek left a comment

Choose a reason for hiding this comment

johnynek Feb 25, 2019

Choose a reason for hiding this comment

ittaiz Feb 25, 2019

Choose a reason for hiding this comment

johnynek Feb 25, 2019

Choose a reason for hiding this comment

ittaiz Feb 25, 2019

Choose a reason for hiding this comment

ittaiz commented Feb 25, 2019

johnynek commented Feb 25, 2019

ittaiz commented Feb 21, 2019 •

edited

Loading

ittaiz commented Feb 21, 2019 •

edited

Loading