rules_scala
rules_scala copied to clipboard
bazelbuild
Unused dependency checker reports actual rule instead of alias
With the unused dependency checker, currently if there is an alias, the alias's target is printed in the error message instead. That is, in the following
scala_library(
name = "a",
srcs = ["A.scala"],
deps = [":b2"],
)
alias(
name = "b2",
actual = ":b"
)
scala_library(
name = "b",
srcs = ["B.scala"]
)
the error is
ERROR: XXX/bazel_play/BUILD.bazel:4:1: scala //:a failed (Exit 1)
error: Target '//:b' is specified as a dependency to //:a but isn't used, please remove it from the deps.
You can use the following buildozer command:
buildozer 'remove deps //:b' //:a
one error found
one error found
As opposed to complaining about ":b2".
This breaks buildozer as well as making it confusing for people.
Hmm, That is indeed a limitation in the current mechanism. This can be resolved by re-exporting labels but unfortunately this was discussed thoroughly with bazel and java bazel people and the decision was to remove label collection entirely, take the labels from the jars (that is from creation site and not allow re-exporting) and then add required logic in a separate tool that knows to take these instructions and augment them. Bottom line I think we need to understand how java rules handle this issue right now since we try to follow them closely. Any chance you can repro this in java and update here what happens?
On Mon, 11 Nov 2019 at 21:06 Jamie5 [email protected] wrote:
With the unused dependency checker, currently if there is an alias, the alias's target is printed in the error message instead. That is, in the following
scala_library( name = "a", srcs = ["A.scala"], deps = [":b2"], )
alias( name = "b2", actual = ":b" )
scala_library( name = "b", srcs = ["B.scala"] )
the error is
ERROR: XXX/bazel_play/BUILD.bazel:4:1: scala //:a failed (Exit 1) error: Target '//:b' is specified as a dependency to //:a but isn't used, please remove it from the deps. You can use the following buildozer command: buildozer 'remove deps //:b' //:a one error found one error found
As opposed to complaining about ":b2".
This breaks buildozer as well as making it confusing for people.
— 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/875?email_source=notifications&email_token=AAKQQFYVMOH4CUQGP5UA5SDQTGUK3A5CNFSM4JLZRMC2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HYP3U6A, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKQQF5LSNAFOURFWNBMDBDQTGUK3ANCNFSM4JLZRMCQ .
Not sure if I used unused_deps correctly, but the behavior was that when not using aliases, it reports
buildozer 'remove deps :d' //:c
buildozer 'remove deps :e' //:c
However when using aliases, it reported
No unused deps found.
So does the unused_deps tool not work with aliases at all?
Then tried triggering strict java deps with the following
java_library(
name = "c",
srcs = ["C.java"],
deps = [":d2"],
)
alias(
name = "d2",
actual = ":d"
)
java_library(
name = "d",
srcs = ["D.java"],
deps = [":e2"],
)
alias(
name = "e2",
actual = ":e"
)
java_library(
name = "e",
srcs = ["E.java"]
)
and this resulted, during compile, in
ERROR: XXX/bazel_play/BUILD.bazel:4:1: Building libc.jar (1 source file) failed (Exit 1)
C.java:3: error: [strict] Using type E from an indirect dependency (TOOL_INFO: "//:e"). See command below **
E e = null;
^
** Please add the following dependencies:
//:e to //:c
** You can use the following buildozer command:
buildozer 'add deps //:e' //:c
Target //:c failed to build
which suggests that this is not respecting aliases either.
So that seems to be the current state of things in java-world.
Yeah, that’s what I thought. Wdyt about opening an issue in rules_java saying what you want and cc’ing me there to get the discussion going?
I’ll elaborate a bit about the plan to handle these with the external tool I mentioned: Let’s take strict deps because this is what we’ve thought about already. The rule will emit “ new_tool 'add deps //:e' //:c” and new_tool, using bazel query, finds all paths from c to e and chooses the shortest one taking into account exports, visibility, and aliases.
We haven’t discussed unused deps at the time but if I need to improvise I’d guess it will be similar maybe with a preliminary check to see if c directly depends on e (if so remove it) otherwise look at paths.
We have such a tool in the making and if this isn’t pressing than I can definitely share it when it will be ready (roughly end of year)
On Tue, 12 Nov 2019 at 20:05 Jamie5 [email protected] wrote:
Not sure if I used unused_deps correctly, but the behavior was that when not using aliases, it reports
buildozer 'remove deps :d' //:c buildozer 'remove deps :e' //:c
However when using aliases, it reported
No unused deps found.
So does the unused_deps tool not work with aliases at all?
Then tried triggering strict java deps with the following
java_library( name = "c", srcs = ["C.java"], deps = [":d2"], )
alias( name = "d2", actual = ":d" )
java_library( name = "d", srcs = ["D.java"], deps = [":e2"], )
alias( name = "e2", actual = ":e" )
java_library( name = "e", srcs = ["E.java"] )
and this resulted, during compile, in
ERROR: XXX/bazel_play/BUILD.bazel:4:1: Building libc.jar (1 source file) failed (Exit 1) C.java:3: error: [strict] Using type E from an indirect dependency (TOOL_INFO: "//:e"). See command below ** E e = null; ^ ** Please add the following dependencies: //:e to //:c ** You can use the following buildozer command: buildozer 'add deps //:e' //:c
Target //:c failed to build
which suggests that this is not respecting aliases either.
So that seems to be the current state of things in java-world.
— You are receiving this because you commented.
Reply to this email directly, view it on GitHub https://github.com/bazelbuild/rules_scala/issues/875?email_source=notifications&email_token=AAKQQF7KAUAEUS5HXLFGHHTQTLV7LA5CNFSM4JLZRMC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOED3GNNI#issuecomment-553019061, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKQQFZJCNHASRRMOCATRBTQTLV7LANCNFSM4JLZRMCQ .
To be clear, my request to https://github.com/bazelbuild/rules_java is that strict_deps will, if rule A is missing a dependency on rule B, suggest the label for B which is used closest to A (that is, if there are two chains "A -> X -> B" and "A -> Y -> Z -> B", then we would pick the label that X uses for B (whether that be alias of B or not) in a deterministic fashion. And that this happens without using any external tools or anything like that?
Do I also want a request for https://github.com/bazelbuild/buildtools regarding unused_deps, or is that mainly dependent on improving strict_deps?
Should the first point be addressed in rules_java, does that mean that rules_scala can then easily improve unused_deps to handle this by emitting error messages that point directly to the alias needed?
The external tool does sound useful though it will be unfortunate if the core rules_scala can't emit the correct messages leading people to still be confused.
No not at all. The "valid" nodes (and so paths) are B itself or targets that export B, or aliases of it. Additionally targets with visibility might be invalid. Thing is that I've had lengthy discussions with the java people and they were adamant about not supporting this in the rules and they were the ones who suggested the external tool idea.
My suggestion was to open an issue specifically about unused deps to understand from them how they see it. unused-deps as opposed to strict-deps is a bit trickier.
On Wed, Nov 13, 2019 at 8:17 PM Jamie5 [email protected] wrote:
To be clear, my request to https://github.com/bazelbuild/rules_java is that strict_deps will, if rule A is missing a dependency on rule B, suggest the label for B which is used closest to A (that is, if there are two chains "A -> X -> B" and "A -> Y -> Z -> B", then we would pick the label that X uses for B (whether that be alias of B or not) in a deterministic fashion. And that this happens without using any external tools or anything like that?
Do I also want a request for https://github.com/bazelbuild/buildtools regarding unused_deps, or is that mainly dependent on improving strict_deps?
Should the first point be addressed in rules_java, does that mean that rules_scala can then easily improve unused_deps to handle this by emitting error messages that point directly to the alias needed?
The external tool does sound useful though it will be unfortunate if the core rules_scala can't emit the correct messages leading people to still be confused.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/rules_scala/issues/875?email_source=notifications&email_token=AAKQQF27A2FGDS4UKXXE5HDQTRAFDA5CNFSM4JLZRMC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOED7EEHQ#issuecomment-553533982, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKQQF5QCAFE5G63KWAGLEDQTRAFDANCNFSM4JLZRMCQ .
Ok, so open an issue on https://github.com/bazelbuild/rules_java saying that the unused_deps tool in https://github.com/bazelbuild/buildtools seems to be broken with java_rule aliases, and what would be the way to support unused_deps correctly reporting aliases (and presumably exports, though unsure if that is broken), i.e. if they think that the core rules_java needs to add support for something, or what is it that the external tool of unused_deps needs to do to make this work correctly.
Yes. You can mention me in the issue. The core of it is that they propagate labels from the creation site and not from the usage site. For adding deps the external tool solution might be elegant enough. Not sure about removing deps (I think it can work but haven't had the time to dive in).
On Wed, Nov 13, 2019 at 11:33 PM Jamie5 [email protected] wrote:
Ok, so open an issue on https://github.com/bazelbuild/rules_java saying that the unused_deps tool in https://github.com/bazelbuild/buildtools seems to be broken with java_rule aliases, and what would be the way to support unused_deps correctly reporting aliases (and presumably exports, though unsure if that is broken), i.e. if they think that the core rules_java needs to add support for something, or what is it that the external tool of unused_deps needs to do to make this work correctly.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bazelbuild/rules_scala/issues/875?email_source=notifications&email_token=AAKQQFYSGLRUOY5QK3W65M3QTRXDFA5CNFSM4JLZRMC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOED7XSTQ#issuecomment-553613646, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKQQF3BF4MB2QH7YCOQRHLQTRXDFANCNFSM4JLZRMCQ .
@ittaiz I know you said so, but wonder if ^ was created in the right place as opposed to https://github.com/bazelbuild/bazel , as the former seems kind of empty while the latter seems to be quite busy and include issues around java rules.
Ok put it at https://github.com/bazelbuild/bazel/issues/10318 for now and will hope for the best
@ittaiz ok so I guess the long-term goal is label stamping (how long term?) and at that point there will be a tool to come up with the right label. Is that tool also part of label stamping?
If the complexity is large enough that this is a long term project, then in the short term is it reasonable to look for ways to mitigate this issue?
The main blocker is https://github.com/bazelbuild/bazel/issues/6752
Wix already has a tool for this (@sha1n how ready is it) and the work in rules_scala assuming the above issue is fixed is not huge and can definitely happen short term.
Jamie, Do you want to take a stab at 6752?
Is that still blocked on people deciding if they would take contributions for it, or do you mean to duplicate the jar stamping code as mentioned in a comment there?
And the diff that should succeed, but fails with current bazel, is https://github.com/bazelbuild/rules_scala/commit/a76792b99bc4470819340051973007adb6f9e49a .
In any case this looks unfamiliar and confusing but I can try to take a look, though may not be immediately and may not be able to figure things out.
I think it really depends on the change. If it's local I'd try sending it upstream and move it over here if they decline (which I'd be surprised and I can try and help to push it forward). If it's a bigger change then we need to see. In any case someone needs to go into the stamping code and take a look. If you can take a look that would be really helpful
So far not making progress. But IIUC, the situation is we have this code in stampJar
String basename = FileSystemUtils.removeExtension(inputJar.getFilename()) + "-stamped.jar";
Artifact outputJar = actions.declareFile(basename, inputJar);
and declareFile fails in our case, because we are passing inputJar as the sibling. And inputJar's path is external, so we are trying to create in external instead of our path. And for our use case (ignoring all the things it would break in other use cases), in theory we could just change declareFile to pass null for sibling instead of inputJar. (but have not tested yet as haven't yet set up being able to test with a local bazel).
If scala_import copied over* the jars would it also bypass the issue (since they’re not under external)? We have a different issue over in IntelliJ which stems from scala_import not copying over the jars. We had thoughts of fixing it over in IJ but if this issue can also be solved that way then maybe we should change scala_import (since it sounds bazel works better this way). I’m a bit concerned about disk space and IO but if that’s the only concern maybe we should benchmark it
That probably would solve the problem, yes (though again this is theoretical without trying it). (Maybe symlinks would also work, not sure).
Let me know if you'd like for me to try one or both of such approaches and see if they work. Note I don't know much about the area and my definition of "work" would be that the commit above which calls stamp_jar compiles without error.
I didn’t reply since I wanted to try out the symlink thing. It might take me a few more days before I’m able to free up a slot so if you want to take a shot that would be great. Should be simple enough. If the above commit will pass with a symlink then I’ll checkout your branch and check it against the IJ issue
@ittaiz this diff: https://github.com/Jamie5/rules_scala/commit/3f3a79d59fc129b95dd0b0fff20352e8b17817d2
seems to improve things - whereas bazel test //test/src/main/scala/scalarules/test/scala_import/... failed on just https://github.com/bazelbuild/rules_scala/commit/a76792b99bc4470819340051973007adb6f9e49a , it now succeeds.
Not everything is perfect - bazel test //third_party/... fails with some errors at runtime about missing things, but maybe it is expected that this is still a WIP.
@ittaiz in case the notification was missed (rather than that you're too busy right now, which would be very much understandable even if we weren't in these current circumstances), does the above diff help you or is there more I should look into?
hey, I indeed missed the notifications in the sense that I was just trying to keep the head a float. I've now taken a few days off to be with the family and so I'll also spend some time on OSS. This issue is now at the top of my list
ok, took a deeper look. The above diff indeed helps though I think we need it to pass. Do you think you'll have time to get it to pass? Do you need help there?
And thank you for your efforts on this! I really value your work in this area of dependencies. I think it has great value for our users.
Haha makes sense, I'm taking a few days off work to take a nice relaxing vacation in my living room.
Am not very familiar with this, I could try to take a further look though unclear on timeline. By pass, we mean that the CI build runs successfully, right (which would hopefully catch all the issues)?
Yeah. Alternatively you can open a PR and I can also try and iterate on it and push commits there.
PR here for you to investigate: https://github.com/bazelbuild/rules_scala/pull/1037 as future time somewhat limited
So due to (voluntary) unemployment might have some time to look at this further, in case you haven't already done so. Though the dev environment setup is somewhat annoying so far.