Unused dependency checker offers to fix external source deps

[liucijus]

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

ERROR: /home/vaidas/.cache/bazel/_bazel_vaidas/fe796626ea1b61600b06a08b03d1d927/external/ext_b/BUILD:3:1: scala @ext_b//:b failed (Exit 1)
error: Target '@maven//:commons_io_commons_io' is specified as a dependency to @ext_b//:b but isn't used, please remove it from the deps.
You can use the following buildozer command:
buildozer 'remove deps @maven//:commons_io_commons_io' @ext_b//:b

Problems:

• buildozer command does not work
• in case one does not control external dependency and can't update it, it becomes a blocking state (either don't depend on it, or do not use unused deps checking)

[Jamie5]

@ittaiz From my POV the suggestion of not emitting errors on external deps seems quite reasonable, because if the external dep wanted to have unused and stuff, they could turn it on in their own workspace.

If you concur, the only issue is then implementing, which I have not investigated. (Maybe just checking that the rule starts with @ and then skipping dependency analyzer if so is enough, but not sure)

(I don't think it makes sense to add configuration of what external deps should do wrt unused/strict deps, as suggested in the wishlist, and that the only sensible option is to do nothing, as the wishlist implies)

[ittaiz]

Two main thoughts-
Caching- I think there’s a difference between having this “if” in starlark than having it in the scalac plugin. If the conditional is in starlark then one loses the ability to reuse caching between workspaces. If it’s inside the tool then the inputs to the action are kept and that might work.
Note though that this probably means that if one works with “warn” then they’re likely to get warnings even when building the external repo since they will have a cache hit which AFAIR will also print the warnings.
I don’t know how big of a problem this is if any. My guess is that this specific case is more of a problem for migration and could be resolved via an external script.

Wdyt?

The second thought is that this is non idiomatic in bazel- perhaps this isn’t a big issue and perhaps this can evolve into an idiom.

[Jamie5]

Don't quite follow why this is more of a migration problem. Is it because if someone was consuming some repository not under their control, they would probably be importing jars and stuff instead of source code, and that these external things are likely to still be internal to the organization?

Also what would an external script do in this case?

[ittaiz]

Mainly for that reason.
But also in addition I’m not entirely sure why would someone want to run with warn dependency checking apart from migration (where you need an alignment phase).

The solution to the caching problem won’t solve the issue Vaidas is demonstrating when someone is using warn.

If I’m right and warn is mainly for migration then an external script can read this and filter out such messages so that all repos can advance on their own

[Jamie5]

@liucijus was your mention of someone not controlling the external dependency just an example of something that could happen, or something which actually is happening currently?

@ittaiz Ok I get what you mean, that we could solve this by checking within the plugin, which hopefully allows caching (though we may not know this for sure, I guess it can't be worse than doing it in starlark which definitely wouldn't be cache friendly). And there is the downside of showing warn messages, but that no one will use warn anyways except for migration which will likely be a short bounded time.

And hence to address the problem, one option is doing it in the plugin which might be more cache friendly, and the other option is that possibly this isn't a problem at all because people will only import source directory with code they control, and be using jars otherwise?

Though wait, if we are compiling external deps with current workspace settings, how will dependency mode and stuff work? Because if A builds with transitive mode (and elides some deps which direct would require), and B which depends on A builds with direct mode, then when B is trying to compile A it would break right?

[ittaiz]

which hopefully allows caching

I’m fairly certain it will since all inputs are the same as far as Bazel is concerned. Actually thinking about this point I’m suddenly unsure how does cross workspace caching happens if the paths are different. And if they aren’t then how will the plugin be able to differentiate.
I’ll ask this in slack.

no one will use warn anyways except for migration

No one is a strong statement but I think it’s definitely not the common case to use warn as your d2d mode

people will only import source directory with code they control, and be using jars otherwise

Again a bit of a strong statement :) but I think that predominantly they will do that and if they import source code of others it’s likely libraries which will be very open to aligning themselves to their users needs like we have done in the past

when B is trying to compile A it would break right

You’re right. There needs to be some alignment. My guess is that inside an organization you should have a common toolchain and community projects should use the strictest configuration.

[ittaiz]

Apparently I was mistaken and there is no such caching.
Given repos A, B, and C and given C -> A and given B -> A then B & C will NOT have cache hits from A's own build.
They will of course be able to use cache hits of A from each other's builds.

Having said that we can do the toggle in starlark.
However I think that this warrants a discussion with the bazel folk (issue/slack/google-group) to see we're not making a mistake. I say this because I strongly remember bazel discussions (google group? issue?) about wanting to suppress warnings for external workspaces and bazel devs saying this can't be done in bazel.

@liucijus do you want to start this discussion?

[liucijus]

@Jamie5 my mention of not controlling external dependency is a bit made up, but inspired by the situation that we have at Wix interconnected mono-repos where fixing an external warning takes time (by crossing the border into another team's code).