Strict Deps smart mode

[natansil]

We want to improve the current state of the dependency analyzer compiler plugin for strict deps,
Currently the plugin looks at which jars were loaded by the compiler, in order to assess if the target should be a direct dependency. This strategy is quite coarse-grained and over-approximates which targets are needed.

In order to improve the plugin, the proposal by @gkk-stripe is to utilize code from Zinc that analyzes source code which has much higher accuracy.

Plan is to use the zinc code callbacks mechanism to output the dependencies to a file and have a separate action that emits the relevant error messages to the user of missing dependencies

[gkossakowski]

Adding a bit more detail: the plan is to take Dependency.scala from zinc and copy it to rules_scala. I think this is a great way to test the idea of using zinc's dependency analysis for strict deps implementation. However, once validated, i think the best would be to refactor zinc so it can output dependencies in its protobuf format without involving the rest of incremental compilation. The cost of maintaing a fork of Depedency.scala long term will be high: it's a very tricky logic.

[ittaiz]

When you say refactor zinc you mean pushing it upstream?

…

On Mon, 13 Nov 2017 at 1:50 Grzegorz Kossakowski ***@***.***> wrote: Adding a bit more detail: the plan is to take Dependency.scala from zinc and copy it to rules_scala. I think this is a great way to test the idea of using zinc's dependency analysis for strict deps implementation. However, once validated, i think the best would be to refactor zinc so it can output dependencies in its protobuf format without involving the rest of incremental compilation. The cost of maintaing a fork of Depedency.scala long term will be high: it's a very tricky logic. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#335 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF1EoKDP5g6XHBjz2uNHj20Lanc4rks5s14RWgaJpZM4QY6fV> .

[gkossakowski]

Yes, refactor upstream zinc to have a mode in which only dependencies are collected and not api objects that are expensive to compute. On 12 November 2017 at 21:53, Ittai Zeidman <[email protected]> wrote:

…

When you say refactor zinc you mean pushing it upstream? On Mon, 13 Nov 2017 at 1:50 Grzegorz Kossakowski ***@***.*** > wrote: > Adding a bit more detail: the plan is to take Dependency.scala from zinc > and copy it to rules_scala. I think this is a great way to test the idea of > using zinc's dependency analysis for strict deps implementation. However, > once validated, i think the best would be to refactor zinc so it can output > dependencies in its protobuf format without involving the rest of > incremental compilation. The cost of maintaing a fork of Depedency.scala > long term will be high: it's a very tricky logic. > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <https://github.com/bazelbuild/rules_scala/issues/ 335#issuecomment-343779083>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ ABUIF1EoKDP5g6XHBjz2uNHj20Lanc4rks5s14RWgaJpZM4QY6fV> > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#335 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAQeYZ8WvQWbPHJckVOSiflClKXuZHPks5s19lVgaJpZM4QY6fV> .

-- gkk

[natansil]

Hi @gkk-stripe ,
I'm starting to take another look at this as I hope to try and get some headway here next week.
I'm sitting with @ittaiz and trying to refresh our memory and would appreciate your help.
We think that we need to have Dependency.scala accept a callback that we write which will only care about callback.binaryDependency and will emit the results to a file to be later processed by a different action.
Does that sound about right?
It seems that callback.binaryDependency happens for both external class files and external jars and we're not sure how to differentiate between the two, any thoughts?

[gkossakowski]

Yes, it sounds right about having Dependency.scala and the callback.

In what case would you see dependencies on .class files? Everything in bazel is packaged into jars, isn't it?

[natansil]

I think you are correct about your statement on bazel.

So in sbt it's different?

[gkossakowski]

Yes, in Sbt/Maven/etc you work with class directories. Packaging into jars is done before publishing or building a deployment artifact.

…

On Sun, Mar 4, 2018 at 7:54 AM Natan Silnitsky ***@***.***> wrote: I think you are correct about your statement on bazel. So in sbt it's different? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#335 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAQeSkZYnPsf_r7o6omAe38RH0zBF4pks5tbA5QgaJpZM4QY6fV> .

[natansil]

@gkossakowski,
I'm having some difficulty introducing the CallbackGlobal to existing setup of rules_scala plugin (as a c'tor param).

we are testing the plugin by using Global to compile sample sources.
Global in turn uses Plugin.scala code that tries to create a plugin instance by calling c'tor with Global type param and fails:

java.lang.NoSuchMethodException: third_party.plugin.src.main.io.bazel.rulesscala.dependencyanalyzer.DependencyAnalyzer.<init>(scala.tools.nsc.Global)

I've tried using toolbox to compile the sample code instead, but still get the same error:

scala.tools.reflect.ToolBoxError: reflective compilation has failed: cannot initialize the compiler due to java.lang.NoSuchMethodException: third_party.plugin.src.main.io.bazel.rulesscala.dependencyanalyzer.DependencyAnalyzer.<init>(scala.tools.nsc.Global)

Sample code here

Seems like CallbackGlobal is tightly coupled to ZincCompiler and does not work with the standard scala compiler.

am I correct?

[natansil]

Hi @gkk-stripe ,
For the time being I've decided to discard the callback part I've mentioned in my last comment and focus on incorporating the dependency analysis and use it synchronously.

When running rules_scala tests using the new analysis i've noticed a case where the dependency expressions is missed by the tree traversal pattern matching.

the expression is of kind: classOf[org.apache.commons.lang3.ArrayUtils]
None of the current cases (Import, Select, TypeTree) matches.

Can you please direct me to which tree type I should use to match against for this kind of expression?

[gkossakowski]

Removing callback is fine. In zinc/sbt it's there to isolate the two scala versions: one used to compile and run sbt and the other one used for compiling the scala project. In bazel you have only one scala version at the time so you don't need to worry about cross-classloader boundries.

Regarding matching on classOf, it's expressed as a Literal(Constant(...)) inside of the compiler.

[gkossakowski]

I just looked briefly at the zinc's source code and it seems like this is a bug in zinc. Nice catch!
It would be good to file a bug report against zinc. The fix will be very simple, though.