bazelbuild / buildtools

A bazel BUILD file formatter and editor
Apache License 2.0
1.02k stars 416 forks source link

unused_deps doesn't seem to be working properly #966

Open joca-bt opened 3 years ago

joca-bt commented 3 years ago

unused_deps doesn't seem to be working properly for dependencies from rules_jvm_external.

I have attached an example.zip which contains the WORKSPACE and BUILD files below and some additional files. The example runs a Spring Boot application with an endpoint that doesn't do anything.

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

RULES_JVM_EXTERNAL_TAG = "4.0"
RULES_JVM_EXTERNAL_SHA = "31701ad93dbfe544d597dbe62c9a1fdd76d81d8a9150c2bf1ecf928ecdf97169"

http_archive(
    name = "rules_jvm_external",
    sha256 = RULES_JVM_EXTERNAL_SHA,
    strip_prefix = "rules_jvm_external-%s" % RULES_JVM_EXTERNAL_TAG,
    url = "https://github.com/bazelbuild/rules_jvm_external/archive/%s.zip" % RULES_JVM_EXTERNAL_TAG,
)

load("@rules_jvm_external//:defs.bzl", "maven_install")
load("@rules_jvm_external//:specs.bzl", "maven")

maven_install(
    artifacts = [
        maven.artifact("org.springframework", "spring-web", "5.3.4"),
        maven.artifact("org.springframework.boot", "spring-boot", "2.4.3"),
        maven.artifact("org.springframework.boot", "spring-boot-autoconfigure", "2.4.3"),
        maven.artifact("org.springframework.boot", "spring-boot-starter-web", "2.4.3"),
    ],
    repositories = [
        "https://repo.maven.apache.org/maven2/",
    ],
)
load("@rules_java//java:defs.bzl", "java_binary", "java_library")

java_library(
    name = "lib",
    srcs = glob(["src/main/java/**/*.java"]),
    deps = [
        "@maven//:org_springframework_spring_web",
        "@maven//:org_springframework_spring_webmvc", # unused dep
        "@maven//:org_springframework_boot_spring_boot",
        "@maven//:org_springframework_boot_spring_boot_autoconfigure",
    ],
)

java_binary(
    name = "app",
    main_class = "org.training.bazel.MainApplication",
    runtime_deps = [
        ":lib",
        "@maven//:org_springframework_boot_spring_boot_starter_web",
    ],
)

When running unused_deps I was expecting for @maven//:org_springframework_spring_webmvc to be flagged as an unused dependency but it's not the case.

> unused_deps //:lib
2021/03/10 12:47:03 running: bazel query --tool_tag=unused_deps --keep_going --color=yes --curses=yes kind('(kt|java|android)_*', //:lib)
Loading: 0 packages loaded
2021/03/10 12:47:03 running: bazel build --tool_tag=unused_deps --keep_going --color=yes --curses=yes --output_groups=+unused_deps_outputs --override_repository=unused_deps=/tmp/unused_deps368956821 --aspects=@unused_deps//:unused_deps.bzl%javac_params //:lib
INFO: Analyzed target //:lib (4 packages loaded, 206 targets configured).
INFO: Found 1 target...
Aspect @unused_deps//:unused_deps.bzl%javac_params of //:lib up-to-date:
  bazel-bin/lib.javac_params
INFO: Elapsed time: 0.373s, Critical Path: 0.02s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action

No unused deps found.
pmbethe09 commented 3 years ago

Unfortunately unused_deps has not been maintained. Internally we moved to a different tool. I will investigate if unused_deps can be updated, but I am not sure at this time. Otherwise would remove it so as not to confuse people into using a non-functioning tool.

tony-scio commented 3 years ago

unused_deps has been invaluable for us. The main issue with it that we've had is that it doesn't understand reflection, so there are risky false positives. Is there any chance the better, internal tool could be open sourced?

manuelnaranjo commented 3 years ago

We forked the buildtools project and created unused_deps_py, it does pretty much the same that unused_deps does + it adds maven support https://github.com/bookingcom/buildtools/tree/master/unused_deps_py/unused_deps, unfortunately I'm too dumb to write this in go, so I can't just make a MR for it.

ysfaran commented 2 years ago

What are alternative tools here? unused_deps really doesn't care about maven dependencies at all. I can add random maven dependencies and it will just detect none.

This issue is really hidden and the only thing you find when searching "bazel java unused dependencies" is unused_deps with no hint that it's actually not maintained anymore. IMO it's really important to have such a tool to keep BUILD files clean.

Overall this experience was really confusing and I couldn't find a solution after few days what so ever. This tool would be especially helpful if you are migrating from maven to bazel but also if you want to split your project into more bazel packages a.k.a adding more BUILD files to sub directories.

brown commented 1 year ago

The bug described here should be fixed by pull request: https://github.com/bazelbuild/buildtools/pull/1151

luiz787 commented 1 year ago

I just downloaded @joca-bt’s example and executed unused_deps (compiled from source), and got the same “No unused deps found” message. Should any extra arguments be passed to unused_deps in this case? I saw that #1151 mentions passing arguments, but it is not clear to me if I should pass any extra arguments, and if so, what arguments should be passed in order to successfully detect unused maven deps.

brown commented 1 year ago

No extra arguments should be required. The example also produces "No unused deps" for me. I'll try debugging it. Thanks for reporting that there's still a problem with some unused external repo deps. The unused_deps utility is successfully identifying unused @maven deps for another project I work on.

brown commented 1 year ago

I believe the problem with the example provided by @joca-bt lies in rules_jvm_external. If you upgrade that package to version 4.5, unused_deps will correctly identify @maven//:org_springframework_spring_webmvc as unused.

I think this issue can be closed.

tony-scio commented 1 year ago

I don't know if this is applicable here or for others, but I wasn't getting anything reported (bazel 6, rules_jvm_external 4.5, rules_java 5.3.5, openjdk 11, go 1.17).

This hack got results that were all safe to remove:

diff --git a/unused_deps/unused_deps.go b/unused_deps/unused_deps.go
index b4f7215..86861e5 100644
--- a/unused_deps/unused_deps.go
+++ b/unused_deps/unused_deps.go
@@ -161,7 +161,7 @@ func directDepParams(blazeOutputPath string, paramsFileNames ...string) (depsByJ
                }
                // The classpath param exceeds MaxScanTokenSize, so we scan just the
                // dependencies section.
-               directDepsFlag := []byte("--direct_dependencies")
+               directDepsFlag := []byte("--classpath")
                arg := bytes.Index(data, directDepsFlag)
                if arg < 0 {
                        continue
@@ -214,9 +214,7 @@ func unusedDeps(depsFileName string, depsByJar map[string]string) (unusedDeps ma
                unusedDeps[label] = true
        }
        for _, dependency := range dependencies.Dependency {
-               if *dependency.Kind == depspb.Dependency_EXPLICIT {
-                       delete(unusedDeps, depsByJar[*dependency.Path])
-               }
+               delete(unusedDeps, depsByJar[*dependency.Path])
        }
        return unusedDeps
 }
brown commented 1 year ago

For me bazel 6.0.0 and rules_jvm_external 4.5 appear to be working correctly with unused_deps. I'm not using any particular rules_java, just whatever Bazel 6.0.0 uses by default.

The differences I see between rules_jvm_external 4.0 and 4.5 are the locations of the Maven external jar files that unused_deps tries to read and whether jars contain a META-INF/MANIFEST.MF file with a Target-Label entry. With 4.5 all jars contain the entry and all their paths begin with "bazel-out". With 4.0 some jars have paths that begin with "external" and some jars do not contain Target-Label info. So with 4.0 unused_deps can identify certain Maven jars as unused, but not others. It looks like newer versions of rules_jvm_external do not use raw unmodified downloaded jars.