rules_jvm_external icon indicating copy to clipboard operation
rules_jvm_external copied to clipboard

Mark transitive closure of testonly artifacts also as testonly

Open Yannic opened this issue 5 years ago • 2 comments

Assume there is a maven package with the following dependency chain (<- = depends on): org.hamcrest:hamcrest-core <- junit::junit <- corp:junit-utils.

Now assume we have a target (or workspace) that provides common functionality for testing Java. We have Java targets with direct dependencies on hamcrest and corp:junit-utils, but noone is importing junit directly, so we don't add it to the lists of artifacts in our workspace. However, since we're providing functionality for tests, we do mark org.hamcrest:hamcrest-core and corp:junit-utils as testonly = True.

java_library(
    name = "java_testutils",
    testonly = True,
    srcs = [...],
    deps = [
        "@maven//:org_hamcrest_hamcrest_core",
        "@maven//:corp_junit_utils",
    ],
)
maven_install(
    name = "maven",
    artifacts = [
        maven.artifact(
            group = "org.hamcrest",
            artifact = "hamcrest-core",
            version = "who-cares",
            testonly = True,
        ),
        maven.artifact(
            group = "corp",
            artifact = "junit-utils",
            version = "who-cares",
            testonly = True,
        ),
    ],
    repositories = [
        "https://maven.google.com",
        "https://repo1.maven.org/maven2",
    ],
)

Running bazel build :java_testutils will fail because a non-testonly target (@maven//:junit_junit) depends on a testonly target (@maven//:org_hamcrest_hamcrest_core).

Fixing this can be achieved by 1) removing testonly = True from maven.artifact, or 2) by adding an explicit dependency for junit:junit.

Option 1 is bad because it would allow us to depend on, e.g., hamcrest from non-testonly targets, and option 2 is bad because it requires us to manually go through the dependencies of artifacts and add those between testonly artifacts to maven_install.

We should mark artifacts that are in the transitive closure of testonly artifacts but not in the transitive closure of non-testonly artifacts also as testonly.

This obviously can still cause failures if, e.g., guava is marked as testonly, but something that depends on guava isn't. However, I'd consider this case a feature of testonly.

Yannic avatar Jan 02 '20 17:01 Yannic

Any update on this one? It's a PITA to have to manually add all of the dependencies of a 3rdparty jar (say, scalatest) as testonly.

virusdave avatar Feb 10 '21 18:02 virusdave

Checking in here. Any plans to take this on?

kpytlar avatar Jan 23 '23 19:01 kpytlar