Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transitive data files do not propagate through kt_jvm_library as they do in java_library. #1033

Open
freetheinterns opened this issue Sep 21, 2023 · 2 comments · May be fixed by #1034
Open
Assignees
Labels
note: good first issue Good for newcomers note: help wanted Extra attention is needed type: bug Something isn't working

Comments

@freetheinterns
Copy link

I've been tracking down a bug we've noticed where files specified in the data attribute of either a kt_jvm_library or a java_library do not show up in the runtime of our unit tests.

The issue only happens when there is at least 1 hop between the data attribute and the test target. EG:
library w/ data -> kt library -> test binary

The issue is also specific to kt_jvm_library. If all of the libraries in the chain are java_library targets, then we see the file as we expect.

The issue can be reproduced with 3 files (excluding workspace & maven setup).

repro/BUILD.bazel

load("@io_bazel_rules_kotlin//kotlin:jvm.bzl", "kt_jvm_library")
load("@rules_java//java:defs.bzl", "java_library", "java_test")

java_library(
    name = "java_library_with_data",
    data = ["transitive_data.txt"],
)

java_library(
    name = "java_test_lib",
    srcs = ["DataAttributeTest.java"],
    deps = ["@maven//:junit_junit"],
    runtime_deps = ["java_library_with_data"],
)

kt_jvm_library(
    name = "kt_test_lib",
    srcs = ["DataAttributeTest.java"],
    deps = ["@maven//:junit_junit"],
    runtime_deps = ["java_library_with_data"],
)

java_test(
    name = "java_test",
    runtime_deps = ["java_test_lib"],
    test_class = "repro.DataAttributeTest",
)

java_test(
    name = "kt_test",
    runtime_deps = ["kt_test_lib"],
    test_class = "repro.DataAttributeTest",
)

repro/DataAttributeTest.java

package repro;

import static org.junit.Assert.assertEquals;

import java.io.IOException;
import java.nio.file.FileVisitOption;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.List;
import java.util.stream.Collectors;
import org.junit.Test;

public class DataAttributeTest {
  @Test
  public void testTransitiveDataFile() throws IOException {
    // Walk the file tree in the current directory and look for any file matching the filename.
    List<String> searchResult =
        Files.walk(Paths.get("").toAbsolutePath(), 1000, FileVisitOption.FOLLOW_LINKS)
            .map(it -> it.toAbsolutePath().toString())
            .filter(it -> it.endsWith("transitive_data.txt"))
            .collect(Collectors.toList());

    assertEquals(1, searchResult.size());
  }
}

repro/transitive_data.txt

any text

Given the above setup, you will notice that the //repro:java_test will pass, but //repro:kt_test will fail. Adding data = ["transitive_data.txt"] directly to the kt_jvm_library will make things pass, so will adding java_library_with_data to the runtime deps of the kt_test target. Switching java_library_with_data to use kt_jvm_library does not help, and neither does switching kt_test to use kt_jvm_test.

This was tested using:
bazel@6.1.2
rules_kotlin@1.8.1
rules_java@0.1.1
jdk@8.362.08.1
kotlin@1.9.10 targeting 1.5 api version
junit:junit:4.13.2

@alexeagle
Copy link
Contributor

There's a TODO which looks related. I think the solution is something like this:

diff --git a/kotlin/internal/jvm/impl.bzl b/kotlin/internal/jvm/impl.bzl
index 03b3d0c..341b6e5 100644
--- a/kotlin/internal/jvm/impl.bzl
+++ b/kotlin/internal/jvm/impl.bzl
@@ -50,10 +50,10 @@ def _make_providers(ctx, providers, transitive_files = depset(order = "default")
                     # explicitly include data files, otherwise they appear to be missing
                     files = ctx.files.data,
                     transitive_files = transitive_files,
-                    # continue to use collect_default until proper transitive data collecting is
-                    # implmented.
-                    collect_default = True,
-                ),
+                ).merge_all([
+                    target[DefaultInfo].default_runfiles
+                    for target in ctx.attr.runtime_deps
+                ]),
             ),
         ] + list(additional_providers),
     )

@freetheinterns
Copy link
Author

I can confirm that applying that patch appears to fix the issue when I reproduce the example under examples/android 👍

@alexeagle alexeagle linked a pull request Sep 26, 2023 that will close this issue
@restingbull restingbull self-assigned this Oct 20, 2023
@restingbull restingbull added type: bug Something isn't working note: help wanted Extra attention is needed note: good first issue Good for newcomers labels Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
note: good first issue Good for newcomers note: help wanted Extra attention is needed type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants