Strict deps infraction passes when it should fail- regression in 0.12.0-RC1

[ittaiz]

Description of the problem / feature request:

Bazel 0.12.0-RC1 (built from b33e5af) passes a strict deps infraction using rules_scala scala_import when it correctly fails it on 0.11.1

BUILD.bazel:

package(default_visibility = ["//visibility:public"])
load(
    "@io_bazel_rules_scala//scala:scala_import.bzl",
    "scala_import",
)

java_library(
    name = "leaf",
    srcs = ["Leaf.java"],
    deps = [":scala_import_transitive"],
)

#failure only on 0.11.1 bazel
java_import(
    name = "scala_import_transitive",
    jars = [],
    deps = [
        ":logback_scala_import",
    ],
)

scala_import(
    name = "logback_scala_import",
    jars = [
        "logback-classic-1.1.11.jar",
    ],
)

#failure on both 0.11.1 bazel and 0.12.0-rc1
java_import(
    name = "java_import_transitive",
    jars = [],
    deps = [":logback_java_import"],
)

java_import(
    name = "logback_java_import",
    jars = [
        "logback-classic-1.1.11.jar",
    ],
)

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

run bazel build on this [repo](repro: https://github.com/ittaiz/strict-deps-false-positive/tree/master
) with 0.11.1 and see it fail, then run it with the above bazel binary and see it pass

What operating system are you running Bazel on?

mac os

Any other information, logs, or outputs that you want to share?

I haven't been able to drill down and figure out if this is a scala_import issue or a java-skylark issue.
Might be relevant to add I'm also witnessing false negative issues with strict deps where it prints out warnings for direct dependencies. Still working on an isolated repro for that

[ittaiz]

cc @dslomov this is the issue we talked about

[buchgr]

cc: @philwo

[cushon]

@tomlu fyi, see also b/74765278

[ittaiz]

This is crucial to our work, any chance a test will be added for this?
@cushon (and cc @iirina) this sounds related to #4584 in the sense that the current SJD behavior is very optimistic but breaks for many cases in the wild (maven jars and exports IMHO)

[ittaiz]

ok, I've tested out the commit but it doesn't restore the previous condition.
in 0.11.1 we had incorrect messages which we could tranform (via a script) to understand the real dependencies.
in 0.12.0-RC1 we have no messages
given this commit we have error messages but they are partial and somewhat misleading:

** Please add the following dependencies:
    to //some/path
 ** You can use the following buildozer command:
buildozer 'add deps  ' //some/path

If you try to apply these buildozer commands you get an error since no label is now being printed.
@iirina it sounds to me like @cushon also identify some of the problems we discussed (and led to #4584) but solved them in a way which is very problematic for existing users who worked around the problem.

[ittaiz]

IMHO the correct solution is to restore the previous condition and work on #4584 to resolve the problem in a more thorough manner.

[ittaiz]

I hope I'm not spamming this issue too much but the way we currently handle a missing label in rules scala is print:

Unknown label of file {jar_path} which came from {dependency_label}

This is not a good solution but I think it's better than not printing anything.
The ultimate solution IMHO is to have the JavaInfo contain a map of labels->jarPaths and pass that along to compiler plugins. That way all dependencies which stem from rules which output JavaInfo can be recognized and if you have a different one (filegroup for example) than print something like the above.

[cushon]

any chance a test will be added for this

it has test coverage, the test coverage is not open source. I don't know what Bazel team's plans are there, maybe @iirina does?

current SJD behavior is very optimistic but breaks for many cases in the wild (maven jars and exports IMHO)

I'm not sure what this means. Have bugs been filed for the cases where it breaks?

in 0.12.0-RC1 we have no messages

There are still messages, but the summary at the end with the add_dep command is broken.

This part is WAI:

Leaf.java:3: error: [strict] Using type ch.qos.logback.classic.Level from an indirect dependency (TOOL_INFO: "logback-classic-1.1.11.jar").

This part is incorrect:

buildozer 'add deps ' //:leaf 

The reason the add dep command is broken is that the information about the target label that produced the jar is now stored in the jar, and is added by ijar/JavaBuilder/turbine (e.g. support for this was added to ijar in 8a56b16).

If ijar is disabled, which scala_import does, then SJD doesn't know the label for the jar and can't provide an add_dep command. Maybe instead of java_common.create_provider(use_ijar) we should unconditionally use ijar (to ensure the label gets added to the manifest) but have a flag that to disable the regular ijar processing that removes the implementation (which scala can use). @iirina @tomlu WDYT?

[tomlu]

On Thu, Mar 15, 2018 at 5:59 PM, Liam Miller-Cushon < ***@***.***> wrote: any chance a test will be added for this it has test coverage, the test coverage is not open source. I don't know what Bazel team's plans are there, maybe @iirina <https://github.com/iirina> does? current SJD behavior is very optimistic but breaks for many cases in the wild (maven jars and exports IMHO) I'm not sure what this means. Have bugs been filed for the cases where it breaks? in 0.12.0-RC1 we have no messages There are still messages, but the summary at the end with the add_dep command is broken. This part is WAI: Leaf.java:3: error: [strict] Using type ch.qos.logback.classic.Level from an indirect dependency (TOOL_INFO: "logback-classic-1.1.11.jar"). This part is incorrect: buildozer 'add deps ' //:leaf The reason the add dep command is broken is that the information about the target label that produced the jar is now stored in the jar, and is added by ijar/JavaBuilder/turbine (e.g. support for this was added to ijar in 8a56b16 <8a56b16> ). If ijar is disabled, which scala_import does, then SJD doesn't know the label for the jar and can't provide an add_dep command. Maybe instead of java_common.create_provider(use_ijar) we should unconditionally use ijar (to ensure the label gets added to the manifest) but have a flag that to disable the regular ijar processing that removes the implementation (which scala can use). @iirina <https://github.com/iirina> @tomlu <https://github.com/tomlu> WDYT?

I have no opinion except that we should not go back to a map-based approach, either in the providers (as was suggested earlier in this thread), nor in the command lines.

[ittaiz]

I think the important thing I understand from what @cushon is writing above is that you've changed the way strict deps *labels* and *messages* is being implemented but without a public discussion. The reason I think this is wrong is because we (Scala/Kotlin people like @hsyed) are trying to implement strict deps in the JVM ecosystem and interop with java. Mandating ijar as well as deciding that is the interop mechanism instead of command-line or providers should IMHO be discussed in the open.

…

On Fri, Mar 16, 2018 at 12:03 AM tomlu ***@***.***> wrote: On Thu, Mar 15, 2018 at 5:59 PM, Liam Miller-Cushon < ***@***.***> wrote: > any chance a test will be added for this > > it has test coverage, the test coverage is not open source. I don't know > what Bazel team's plans are there, maybe @iirina > <https://github.com/iirina> does? > > current SJD behavior is very optimistic but breaks for many cases in the > wild (maven jars and exports IMHO) > > I'm not sure what this means. Have bugs been filed for the cases where it > breaks? > > in 0.12.0-RC1 we have no messages > > There are still messages, but the summary at the end with the add_dep > command is broken. > > This part is WAI: > > Leaf.java:3: error: [strict] Using type ch.qos.logback.classic.Level from an indirect dependency (TOOL_INFO: "logback-classic-1.1.11.jar"). > > This part is incorrect: > > buildozer 'add deps ' //:leaf > > The reason the add dep command is broken is that the information about the > target label that produced the jar is now stored in the jar, and is added > by ijar/JavaBuilder/turbine (e.g. support for this was added to ijar in > 8a56b16 > < 8a56b16 > > ). > > If ijar is disabled, which scala_import does, then SJD doesn't know the > label for the jar and can't provide an add_dep command. Maybe instead of > java_common.create_provider(use_ijar) we should unconditionally use ijar > (to ensure the label gets added to the manifest) but have a flag that to > disable the regular ijar processing that removes the implementation (which > scala can use). @iirina <https://github.com/iirina> @tomlu > <https://github.com/tomlu> WDYT? > I have no opinion except that we should not go back to a map-based approach, either in the providers (as was suggested earlier in this thread), nor in the command lines. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4846 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF-5e6wR4MaEdCYBaCIjIRvrgK2Njks5teuUegaJpZM4SqLa2> .

[philwo]

Mandating ijar as well as deciding that is the interop mechanism instead of
command-line or providers should IMHO be discussed in the open.

I agree.

[philwo]

I'm not very knowledgeable about this part of Bazel, but I looked at the involved changes and it seems like this is impossible to rollback. Now, what do we do with Bazel 0.12.0?

@ittaiz I totally agree that this should have been discussed in the open. It's really not OK to break the work that our contributors do for Bazel. :(

@tomlu I think your answer is not really sufficient. You can't just break people's work, then not even apologize and then say "Oh, I don't have an opinion except that reverting my work is not an option". You're on the Bazel team, too - can you please follow up here and work with @ittaiz and @hsyed to find a good solution that works for everyone?

(FYI @ulfjack @davidstanke)

[tomlu]

On Fri, Mar 16, 2018 at 4:38 PM, Philipp Wollermann < ***@***.***> wrote: I'm not very knowledgeable about this part of Bazel, but I looked at the involved changes and it seems like this is impossible to rollback. Now, what do we do with Bazel 0.12.0? @ittaiz <https://github.com/ittaiz> I totally agree that this should have been discussed in the open. It's really not OK to break the work that our contributors do for Bazel. :( @tomlu <https://github.com/tomlu> I think your answer is not really sufficient. You can't just break people, then not even apologize and then say "Oh, I don't have an opinion except that reverting my work is not an option". You're on the Bazel team, too - can you please follow up here and work with @ittaiz <https://github.com/ittaiz> and @hsyed <https://github.com/hsyed> to find a good solution that works for everyone?

I'm happy to assist of course. I certainly didn't mean to be snarky, I meant as in "between the two options that were discussed I don't have an opinion on which is better", not "I don't care you guys figure it out".

…

(FYI @ulfjack <https://github.com/ulfjack> @davidstanke <https://github.com/davidstanke>) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4846 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABbnSmsXr6-gcdl8P1BDDg2ZS_b_Vmtyks5tfCK_gaJpZM4SqLa2> .

[tomlu]

I've synched with @cushon on this. Strict deps support has been fixed (modulo possibly some delay in pushing to GitHub), this was a pure bug in the implementation. Where there *is* a publicly visible change is for add_deps support. Each jar manifest will need to contain the "Target-Label" attribute for add_deps to understand where it came from, and it's up to the tools to put it there. We intend to document this somewhere, probably along with general strict deps documentation. For the most part JavaBuilder and ijar will put it there for you. In the case that some tool produces a jar that appears on the compile time classpath, the tool has to add it (should it want add_deps support). We could supply a trivial tool to do add this to any jar, but it would probably generally be better to bake it into the tools to minimise action count. In the Scala case, we are currently investigating if we can instead fix the reason that ijar cannot be used (macros). If so, ijar can be used, and it can put the target label there. Let me know if any of this has any issues. On Fri, Mar 16, 2018 at 4:44 PM, Tomas Lundell <tomas.lundell@gmail.com> wrote:

…

On Fri, Mar 16, 2018 at 4:38 PM, Philipp Wollermann < ***@***.***> wrote: > I'm not very knowledgeable about this part of Bazel, but I looked at the > involved changes and it seems like this is impossible to rollback. Now, > what do we do with Bazel 0.12.0? > > @ittaiz <https://github.com/ittaiz> I totally agree that this should > have been discussed in the open. It's really not OK to break the work that > our contributors do for Bazel. :( > > @tomlu <https://github.com/tomlu> I think your answer is not really > sufficient. You can't just break people, then not even apologize and then > say "Oh, I don't have an opinion except that reverting my work is not an > option". You're on the Bazel team, too - can you please follow up here and > work with @ittaiz <https://github.com/ittaiz> and @hsyed > <https://github.com/hsyed> to find a good solution that works for > everyone? > I'm happy to assist of course. I certainly didn't mean to be snarky, I meant as in "between the two options that were discussed I don't have an opinion on which is better", not "I don't care you guys figure it out". > (FYI @ulfjack <https://github.com/ulfjack> @davidstanke > <https://github.com/davidstanke>) > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#4846 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABbnSmsXr6-gcdl8P1BDDg2ZS_b_Vmtyks5tfCK_gaJpZM4SqLa2> > . >

[philwo]

Thank you, @tomlu! :)

[tomlu]

Well, the ijar investigation ended pretty quickly in the negative :( As far as we understand it a macro implementation can call anything from the full compile time classpath, so we can't strip anything from the jar. We are left with adding the Target-Label manifest entry via a separate action. Since use_ijar should be pretty uncommon, it's probably better to do it in as an explicit action in the scala rules. This is in line with the principle that it's the tool's responsibility to add this entry to the manifest, and if it doesn't, it's explicit in the build graph. The other option is to not bother with any of this. It depends on how important add_dep is when consuming Scala rules from Java. @ittaiz <https://github.com/ittaiz> and @hsyed <https://github.com/hsyed>, what do you think?

…

On Mon, Mar 19, 2018 at 1:36 PM, Philipp Wollermann < ***@***.***> wrote: Thank you, @tomlu <https://github.com/tomlu>! :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4846 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABbnSu8VCYkZmCoNOFZHPrZ7Nmz5duTQks5tf-yOgaJpZM4SqLa2> .

[cushon]

As far as we understand it a macro implementation can call anything from the full compile time classpath, so we can't strip anything from the jar.

We can't strip anything from the jar or from any jars with methods that are reachable from the macro implementation. Macros seem to break the compile-time/runtime jar distinction in a way that I'm not sure is really fixable, unless we restrict ourselves to a subset of uses of macros. Keeping macro implementations on a separate classpath (similar to what is achieved for annotation processors by -processorpath and java_plugin) but I understand that's not how most real-world scala code is structured.

[ittaiz]

@philwo thank you, I appreciate it.

@tomlu @cushon thank your for your efforts; I think smarter people than me do have a few thoughts about how ijar can be fixed (IIUC you can classify macros and so tweak how much to strip given a macro but I might be wrong) over in #632 and if we can continue it there (even if just to finish the thought experiment and close that issue as won't fix) that would be valuable.

Concretely given the TOOL_INFO messages we were able to change our script to still find the relevant java_library <- scala_import bits and make them pass.
This means the usability has taken a somewhat significant regression blow for that use-case but I think we can say it's not a blocker.

I also suggest we continue the discussion about add_deps support and interoperability over in #4584