Unused dependency checker ignores transitive runtime deps

[liucijus]

Repro: https://github.com/wix-playground/scala-unused-deps/tree/master/runtime_deps

The problem: in plus one mode runtime dependencies are transitively collected and become a problem for unused dep checking which checks only compile dependencies. So in case A has a compile dependency B, but actually uses runtime deps from B, unused dependency checker offers to remove it with buildozer command. This works fine for compilation, but may not be detected if there's no proper test that executes a case to validate runtime requirements.
While the actual problem is in misuse of the compile deps, currently such combination of deps is a valid use of bazel.

[Jamie5]

@ittaiz what should we do here?

My POV is probably skewed due to my former project, but we didn't use resources that much and when we did they were of the form which didn't have fallbacks or anything like that, so that the failure in unit tests was loud (and even if unit tests didn't exist, then integration tests would catch them easily enough).

We could detect the risk and emit warnings (within the error messages) when an unused dep has runtime_deps and maybe buildozer commands to add the runtime_deps, but then there are the following issues

• People may well not read the output, and hence miss warnings (when mass-migrating to use unused/strict deps).
  • However, maybe we make the warnings easy to grep, and also write documentation saying to grep for those specific warnings if you are doing a mass migration, so that at least people can look closer at the errors.
  • So basically generally speaking, whenever any removal of unused dep (or addition of a strict dep) can cause any silent change in runtime behavior, we emit a warning.
  • Note this involves recording information about all transitive deps, because if we say that A has an unused dependency B, then B could depend on C which depends on D which ... depends on Y which has a resource Z.
  • Because of the transitive nature, it is possible that the user is flooded with warnings if some highly-depended-on code has runtime deps and hence still just ignore everything.
  • Have not evaluated how difficult this is to do, but as we do some transitive collection anyways it is probably not the worst.
• People may just grep buildozer commands to run them, so we may throw in runtime_deps which are completely unnecessary and just cause confusion

We could choose to suppress unused warnings whenever the target dep has runtime_deps (or resources, I guess) but that seems bad because I think people would generally want warnings about them anyways to get rid of the unused stuff, and when it's not actually unused to fix things appropriately. (the ideal case being that all the testing infra is in place that nothing goes unnoticed)

In any case should we try to add to documentation about this kind of issue?

(I believe this is a general issue which applies to any dependency tracking method)

(side question, what is the deference between runtime_deps and resources for the use of resources?)

[ittaiz]

I think I agree with @Jamie5
My take is that this isn’t a big issue and that tests should be the protector of runtime dependencies
We could theoretically add a config option that will emit “move to runtime deps” instead of remove but I’m not that sure this is needed as well.
If the need is migration then maybe we need an external utility (bash script checked into this repo?) that does this string replace by itself.
I don’t want to emit two warnings for the same dep since some people use ibazel to automatically apply these suggestions for dev velocity (and probably request the dev to review before they push). I think this is an important capability which I wouldn’t want to lose for this.

[liucijus]

Judging from the effort required to find a solution for this behavior, I think it's enough to have a documentation line explaining effect on runtime deps or just leave it as it is.

[ittaiz]

I’m good with either option (and we can do documentation and leave this issue open if someone will want to tackle it later on). Wdyt?

On Wed, 20 May 2020 at 13:14 Vaidas Pilkauskas ***@***.***> wrote: Judging from the effort required to find a solution for this behavior, I think it's enough to have a documentation line explaining effect on runtime deps or just leave it as it is. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1045 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKQQF5XVDXAQZMI3FQLQ2TRSOUPJANCNFSM4NAM2DJQ> .

-- *Ittai Zeidman* 40 Hanamal street, Tel Aviv, Israel <http://www.wix.com>