JavaBuilder changes for unused dependencies checking - #30807
JavaBuilder changes for unused dependencies checking#30807JonathanPerry651 wants to merge 1 commit into
Conversation
github-actions
Bot
added
team-Rules-Java
|
✅ Bazel docs preview is ready! Preview URL: https://bazel-pr-30807.mintlify.app/ Updated for |
| @@ -204,6 +204,57 @@ public void postAttribute(Env<AttrContext> env) { | |||
| public void finish() { | |||
| implicitDependencyExtractor.accumulate(context, checkingTreeScanner.getSeenClasses()); | |||
|
|
|||
| if (dependencyModule.getUnusedDeps() != UnusedDepsMode.OFF) { | |||
| Optional<String> targetLabel = | |||
There was a problem hiding this comment.
Small nit: Since you're only using this Optional to call isPresent(), why use it?
I'd propose instead moving all of this code into a helper method called checkUnusedDeps, and then in that, if dependencyModule.getTargetLabel() is false, return early. It will reduce the indentation on this code and make it a bit easier to scan the entire finish method.
|
@hvadehra / @katre - thanks for your reviews. I'm happy to fix the nit, or else do it in a follow up PR - whatever's easier to manage and get through to HEAD && the next point release. At this point I also have And then #30140 and bazelbuild/rules_java#364 which were waiting on this one. Please let me know how I can help make reviewing all this stuff easier for you - I really appreciate the time you guys have given me! |
| if (entry.getValue().getKind() != Dependency.Kind.INCOMPLETE) { | ||
| Path jar = entry.getKey(); | ||
| JarOwner owner = readJarOwnerFromManifest(NonPlatformJar.forClasspathJar(jar)); | ||
| String jarLabel = owner.label().orElseGet(jar::toString); |
There was a problem hiding this comment.
This fallback to the jar path (if Target-Label isn't in the manifest) does not seem to make sense. So just FYI will be dropping this while importing.
github-actions
Bot
removed
the
awaiting-review
|
@bazel-io flag Please consider cherry-picking this change into Bazel 9.3.0. |
This pull request contains the JavaBuilder-side changes for unused dependencies checking, splitting the functionality as requested by @hvadehra in bazelbuild#29770. Closes bazelbuild#30807. PiperOrigin-RevId: 970370525 Change-Id: Icfe4e07d825db282af54c6e40ae9dc900593f706 (cherry picked from commit 61218bb)
….create_compilation_action` (bazelbuild#30981) This allows faster iteration on the `rules_java` <> `JavaBuilder` contract without having to wait for Bazel changes/releases. In the immediate future, this will be used for the unused-deps checking. Longer term this should also help in migrating off the native compilation code. Note: the added integration shell test is currently a no-op on CI (since we need the corresponding rules_java changes) but does allow for local testing with an overridden `@rules_java`. Closes bazelbuild#30808. PiperOrigin-RevId: 971826913 Change-Id: I8047e057716475a7a4d8bff48b8c7bfe6d79ba1c (cherry picked from commit 0e6f09d) 9.3.0 adaptation: `java_common.bzl` has no `_ALLOWLIST` constant on this branch, so `bazel_internal/test_rules` is added to the allowlist passed inline to `check_private_api`. `JavaCompileActionBuilderTest` needed a `testutil:TestConstants` dep for the new test. The `bazel_java_test.sh` cases are left out: they exercise `--experimental_check_unused_deps`, which is a JavaBuilder option added by bazelbuild#30807 and not present on this branch, and they are a no-op on CI anyway. `JavaCompileActionBuilderTest` and all four shards of `//src/test/shell/bazel:bazel_java_test_jdk17_toolchain_head` pass locally. Closes bazelbuild#30968 Co-authored-by: Jonathan Perry <jonathan.perry@gs.com>
3 participants
This pull request contains the JavaBuilder-side changes for unused dependencies checking, splitting the functionality as requested by @hvadehra in #29770.