Unused dependecies checker does not detect specs2 compile deps

[liucijus]

Repro: https://github.com/wix-playground/scala-unused-deps/blob/master/specs2/

Some sepcs2 deps are required at compile time, but are incorrectly suggested to be removed.

With specs2-matcher and specs2-common:

ERROR: /home/vaidas/projects/scala-unused-deps/specs2/BUILD:3:1: Couldn't build file specs2/specs2.jar: scala //specs2:specs2 failed (Exit 1)
error: Target '@io_bazel_rules_scala_org_specs2_specs2_common//:io_bazel_rules_scala_org_specs2_specs2_common' is specified as a dependency to //specs2:specs2 but isn't used, please remove it from the deps.
You can use the following buildozer command:
buildozer 'remove deps @io_bazel_rules_scala_org_specs2_specs2_common//:io_bazel_rules_scala_org_specs2_specs2_common' //specs2:specs2
error: Target '@io_bazel_rules_scala_org_specs2_specs2_matcher//:io_bazel_rules_scala_org_specs2_specs2_matcher' is specified as a dependency to //specs2:specs2 but isn't used, please remove it from the deps.
You can use the following buildozer command:
buildozer 'remove deps @io_bazel_rules_scala_org_specs2_specs2_matcher//:io_bazel_rules_scala_org_specs2_specs2_matcher' //specs2:specs2

Without specs2-matcher and specs2-common:

ERROR: /home/vaidas/projects/scala-unused-deps/specs2/BUILD:3:1: Couldn't build file specs2/specs2.jar: scala //specs2:specs2 failed (Exit 1)
specs2/MockingBird.scala:7: error: Symbol 'type org.specs2.reflect.ClassesOf' is missing from the classpath.
This symbol is required by 'trait org.specs2.mock.mockito.MocksCreation'.
Make sure that type ClassesOf is in your classpath and check for conflicting dependencies with `-Ylog-classpath`.
A full rebuild may help if 'MocksCreation.class' was compiled against an incompatible version of org.specs2.reflect.
class MockingBird extends Mockito {
                          ^
one error found
one error found
java.lang.RuntimeException: Build failed
	at io.bazel.rulesscala.scalac.ScalacProcessor.compileScalaSources(ScalacProcessor.java:256)
	at io.bazel.rulesscala.scalac.ScalacProcessor.processRequest(ScalacProcessor.java:76)
	at io.bazel.rulesscala.worker.GenericWorker.runPersistentWorker(GenericWorker.java:45)
	at io.bazel.rulesscala.worker.GenericWorker.run(GenericWorker.java:111)
	at io.bazel.rulesscala.scalac.ScalaCInvoker.main(ScalaCInvoker.java:41)

[Jamie5]

@ittaiz do you happen to know if it's possible to specify deps for things using maven_install or if the best thing would be to re-export with deps specified or similar?

[ittaiz]

I'm actually not very familiar with maven_install so I'm not sure.
@liucijus has dove into rules jvm external much more than me.
Also internally we don't use maven_install and see this bug so is it related? Maybe the repro is a bit misleading (I hope not).

[Jamie5]

I think generally there are definitely issues where external dependencies don't declare their deps as deps (which @ittaiz I think you fixed some of). So there could be well another issue. I guess the other possibility is that however maven_install does declare the deps, but we are not catching them due to an internal bug (though that did not seem to be the case as far as I could tell)

[ittaiz]

Actually I’m not sure I understand what your hunch is here @Jamie5
Can you elaborate? What do you think is misconfigured in the above example?
Because I think (not 100% but close) that internally the specs2 deps are correctly configured

[Jamie5]

In the provided repro, I wonder if the "@maven//:org_specs2_specs2_mock_2_12", rule, however it is defined (which I am not familiar with), does not include in its deps and/or exports @io_bazel_rules_scala_org_specs2_specs2_common and @io_bazel_rules_scala_org_specs2_specs2_matcher.

Because in https://github.com/wix-playground/scala-unused-deps/blob/master/specs2/MockingBird.scala we only reference org.specs2.mock.Mockito and mock, which both sound like they would be provided by "@maven//:org_specs2_specs2_mock_2_12",.

Meanwhile, the interface of "@maven//:org_specs2_specs2_mock_2_12", does depend on common and matcher (as we can see by when we remove them the compile fials). So mock ought to have deps on common and matcher, which plus one deps would then detect and hence include, removing the need to include them explicitly.

I hope this makes sense as it feels a bit rambly.

[ittaiz]

I took another look at our internal layout.
I think that fp depends on core which depends on common and matcher.
I’m trying to think what should be the solution for this.
Maybe some tool that does strict deps / exports suggestions for 3rd party? By creating i jars and looking at the signatures or something .
In any case it seems like this isn’t a dependency checker bug but a hole in the minimal dependency story.
Vaidas,
Wdyt?

[liucijus]

The actual problem is in Specs2 project, where mock depends on core, which depends on matcher and common: https://github.com/etorreborre/specs2/blob/master/build.sbt#L383.

I assume there will be more such cases with other libs. I guess at some point we should provide a recommendation how to deal with such cases in docs.

maven_install from rules_jvm_external adds full dep closure to its high level artifacts. The problem exists only if someone (like us at Wix) uses different type of loader which defines full dependency graph by direct deps (which is better for minimal deps).

I guess this issue has to be closed, as not much can be done from rules_scala side, right?

[ittaiz]

I think so but small question.
If maven_install adds all of the closure then how does this reproduce? Maybe I'm missing something.
In any case I'd close this issue after you're able to reproduce that this is indeed the case. If you reproduced this (here or internally) then we can close this

[Jamie5]

@ittaiz wondering if this would be an example where +1 isn't good enough (it sounds like if we translated the situation to plus-one, the plus-one deps of someone including mock would only be core and not matcher and common), and an example of a place where we might need to update the unused deps checker as discussed a bit in #867 (comment) .

(regardless of the situation that is actually happening here, which sounds like it may be something else)

[ittaiz]

I thought about it as well.
I’m not sure since our suspicion is that if we were to apply strict deps for third party then we wouldn’t have this problem which is actual +1 (as opposed to transitive nightmare).
Maybe I’m wrong and maybe I’m right but still your solution is a more pragmatic way then strict deps for 3rd party.
Wdyt?
@liucijus wdyt? Probably best if you read what Jamie linked

[Jamie5]

Not really sure about third party specifically, but it seems like something similar could occur even in a non-third-party situation, and hence we want to make it work (was kind of hoping we'd never run into a scenario like this in real life but that was kind of foolish). Though perhaps third party is different enough that this is still unlikely to happen within a project (though I don't see why that would be the case).

When you say apply strict deps for third party not 100% sure what you mean but do you mean that external deps would use "transitive" mode for whatever they include, and dependency analyzer would analyze as if the option were "direct".

[liucijus]

I think it's an issue on third party loader, with some loaders like maven_install it's not even a problem. I see more value in keeping +1 behavior for third-party deps as it will have leaner classpath. At Wix we plan to fix such cases on our loader side. I would personally delay efforts toward complex heuristic implementation until some one else comes with more similar cases. Though having such implementation would be useful.