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

cmake_external target can be included as data of go_test, but not sh_binary #619

Closed
rickystewart opened this issue Apr 22, 2021 · 14 comments

Comments

@rickystewart
Copy link

rickystewart commented Apr 22, 2021

I've pushed a branch demonstrating this behavior here.

The target //c-deps:libgeos is a cmake_external library that we are able to successfully depend on in the data attribute of a go_test rule:

# pkg/geo/geos/BUILD.bazel
go_test(
    name = "geos_test",
    srcs = ["geos_test.go"],
    data = ["@cockroach//c-deps:libgeos"],
    ...
)

... and indeed, libgeos is there in the runfiles for the test:

# geos_test.runfiles_manifest
cockroach/c-deps/copy_libgeos/libgeos /private/var/tmp/_bazel_ricky/be70b24e7357091e16c49d70921b7985/execroot/cockroach/bazel-out/darwin-fastbuild/bin/c-deps/copy_libgeos/libgeos
cockroach/c-deps/libgeos/include /private/var/tmp/_bazel_ricky/be70b24e7357091e16c49d70921b7985/execroot/cockroach/bazel-out/darwin-fastbuild/bin/c-deps/libgeos/include
cockroach/c-deps/libgeos/lib/libgeos.dylib /private/var/tmp/_bazel_ricky/be70b24e7357091e16c49d70921b7985/execroot/cockroach/bazel-out/darwin-fastbuild/bin/c-deps/libgeos/lib/libgeos.dylib
cockroach/c-deps/libgeos/lib/libgeos_c.dylib /private/var/tmp/_bazel_ricky/be70b24e7357091e16c49d70921b7985/execroot/cockroach/bazel-out/darwin-fastbuild/bin/c-deps/libgeos/lib/libgeos_c.dylib
cockroach/c-deps/libgeos/logs/CMake.log /private/var/tmp/_bazel_ricky/be70b24e7357091e16c49d70921b7985/execroot/cockroach/bazel-out/darwin-fastbuild/bin/c-deps/libgeos/logs/CMake.log
cockroach/c-deps/libgeos/logs/CMake_script.sh /private/var/tmp/_bazel_ricky/be70b24e7357091e16c49d70921b7985/execroot/cockroach/bazel-out/darwin-fastbuild/bin/c-deps/libgeos/logs/CMake_script.sh
cockroach/c-deps/libgeos/logs/wrapper_script.sh /private/var/tmp/_bazel_ricky/be70b24e7357091e16c49d70921b7985/execroot/cockroach/bazel-out/darwin-fastbuild/bin/c-deps/libgeos/logs/wrapper_script.sh

... but a sh_binary (target //build/bazelutil:lint) depending on the same target doesn't also capture the library in its runfiles:

# build/bazelutil/lint.bzl
    native.sh_binary(
        name = name,
        srcs = [script_name],
        data = [
            "//c-deps:libgeos",
@UebelAndre
Copy link
Collaborator

I've pushed a branch demonstrating this behavior here.

You pushed a branch to twitter? 😅

@UebelAndre
Copy link
Collaborator

At first glance, it seems like we're doing all the right things in gathering runfiles
https://github.com/bazelbuild/rules_foreign_cc/blob/11e971d78baeb949ae1c135bb7580fbd4372b75d/foreign_cc/private/framework.bzl#L398-L431

I can look closer if I see your branch. But I also suspect this might be a bug in sh_binary?

@rickystewart
Copy link
Author

Sorry, I am a fool 💀

The branch is here: https://github.com/rickystewart/cockroach/tree/fulllint

@rickystewart
Copy link
Author

I can also try to get a smaller repro for you if necessary.

@UebelAndre
Copy link
Collaborator

Yeah, if you have time to put together a small repro that'd be helpful. I found bazelbuild/bazel#12348 but need more time to dig deeper and having something straight forward to test would make this easier 😄

@UebelAndre
Copy link
Collaborator

@rickystewart
Copy link
Author

Ah, that does seem to be the case. We might be able to bump to an actual rules_foreign_cc release -- give me a second to review that.

@rickystewart
Copy link
Author

I tried that, but unfortunately even the latest rules_foreign_cc release still displays the issue: repro here https://github.com/rickystewart/repro/tree/rules_foreign_cc. The :repro_go and :repro_sh targets are the go_test and sh_binary targets, both of which depend on the cmake_external target :libroach.

repro$ bazel build :repro_go
repro$ cat bazel-bin/repro_go_/repro_go.runfiles_manifest 
cockroach/copy_libroach/libroach /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/execroot/cockroach/bazel-out/darwin-fastbuild/bin/copy_libroach/libroach
cockroach/libroach/include /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/execroot/cockroach/bazel-out/darwin-fastbuild/bin/libroach/include
cockroach/libroach/lib/libroach.a /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/execroot/cockroach/bazel-out/darwin-fastbuild/bin/libroach/lib/libroach.a
cockroach/repro_go_/repro_go /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/execroot/cockroach/bazel-out/darwin-fastbuild/bin/repro_go_/repro_go
repro$ bazel build :repro_sh
repro$ cat bazel-bin/repro_sh.runfiles_manifest 
cockroach/foo.sh /Users/ricky/go/src/github.com/rickystewart/repro/foo.sh
cockroach/repro_sh /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/execroot/cockroach/bazel-out/darwin-fastbuild/bin/repro_sh

@UebelAndre
Copy link
Collaborator

Could you try this?

diff --git a/BUILD b/BUILD
index ac395ef..c17da83 100644
--- a/BUILD
+++ b/BUILD
@@ -15,7 +15,7 @@ cmake_external(
         "cp -r $EXT_BUILD_ROOT/external/libroach/include libroach",
     ],
     static_libraries = ["libroach.a"],
-    tools_deps = [
+    data = [
         "@libroach//:all",
     ],
     visibility = ["//visibility:public"],

@rickystewart
Copy link
Author

That results in the runfiles for the sh_binary including all the libroach source files, but the library is still missing (note that cockroach/libroach/lib/libroach.a isn't in here):

repro$ cat bazel-bin/repro_sh.runfiles_manifest 
cockroach/external/libroach/.clang-format /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/external/libroach/.clang-format
cockroach/external/libroach/BUILD.bazel /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/external/libroach/BUILD.bazel
cockroach/external/libroach/CMakeLists.txt /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/external/libroach/CMakeLists.txt
cockroach/external/libroach/README.md /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/external/libroach/README.md
cockroach/external/libroach/WORKSPACE /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/external/libroach/WORKSPACE
cockroach/external/libroach/include/libroach.h /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/external/libroach/include/libroach.h
cockroach/external/libroach/stack_trace.cc /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/external/libroach/stack_trace.cc
cockroach/external/libroach/stack_trace.h /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/external/libroach/stack_trace.h
cockroach/foo.sh /Users/ricky/go/src/github.com/rickystewart/repro/foo.sh
cockroach/repro_sh /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/execroot/cockroach/bazel-out/darwin-fastbuild/bin/repro_sh
libroach/.clang-format /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/external/libroach/.clang-format
libroach/BUILD.bazel /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/external/libroach/BUILD.bazel
libroach/CMakeLists.txt /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/external/libroach/CMakeLists.txt
libroach/README.md /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/external/libroach/README.md
libroach/WORKSPACE /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/external/libroach/WORKSPACE
libroach/include/libroach.h /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/external/libroach/include/libroach.h
libroach/stack_trace.cc /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/external/libroach/stack_trace.cc
libroach/stack_trace.h /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/external/libroach/stack_trace.h

@rickystewart
Copy link
Author

Gentle ping on this -- even a workaround would be great :)

@rickystewart
Copy link
Author

Ah -- looks like funneling the dependency through a no-op intermediary filegroup does the trick as a workaround :p

repro$ cat bazel-bin/repro_sh.runfiles_manifest 
cockroach/copy_libroach/libroach /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/execroot/cockroach/bazel-out/darwin-fastbuild/bin/copy_libroach/libroach
cockroach/foo.sh /Users/ricky/go/src/github.com/rickystewart/repro/foo.sh
cockroach/libroach/include /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/execroot/cockroach/bazel-out/darwin-fastbuild/bin/libroach/include
cockroach/libroach/lib/libroach.a /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/execroot/cockroach/bazel-out/darwin-fastbuild/bin/libroach/lib/libroach.a
cockroach/repro_sh /private/var/tmp/_bazel_ricky/b12fe6cd3e6e067332cc17cd9865b32d/execroot/cockroach/bazel-out/darwin-fastbuild/bin/repro_sh
repro$ git diff
diff --git a/BUILD b/BUILD
index ac395ef..e8d3939 100644
--- a/BUILD
+++ b/BUILD
@@ -22,6 +22,8 @@ cmake_external(
     deps = [],
 )
 
+filegroup(name = "intermediary", srcs = [":libroach"])
+
 go_test(
     name = "repro_go",
     srcs = ["repro_test.go"],
@@ -31,5 +33,5 @@ go_test(
 sh_binary(
     name = "repro_sh",
     srcs = ["foo.sh"],
-    data = [":libroach"],
+    data = [":intermediary"],
 )

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days. Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_foreign_cc!

@github-actions
Copy link

This issue was automatically closed because it went 30 days without a reply since it was labeled "Can Close?"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants