Declare exported headers to Bazel to fix bazel build --incompatible_no_implicit_file_export
#13982
Conversation
| # iree_bitcode_library generates targets on the other package | ||
| # we need to expose files used by it. | ||
| exports_files(glob(["*.h"])) |
There was a problem hiding this comment.
Can you clarify what this fixes? (e.g. share some logs of what failed to build)
I thought some of this code might need to move files around or export files, but I'm wondering why our Bazel CI is happy with what we have if you evidently ran into some issues... maybe a difference between Bazel and Blaze?
There was a problem hiding this comment.
I didn't know about exports_files, but reading the documentation now,
https://bazel.build/reference/be/functions#exports_files
it makes sense and the implicit-exporting behavior that we were relying on here is indeed the kind of thing that Bazel would frown upon as sloppy.
The above doc link mentions that --incompatible_no_implicit_file_export gives stricter behavior. I'm trying now locally to check that it does cause Bazel to reject the current code and that this PR fixes that, and I'll report back.
There was a problem hiding this comment.
Confirmed that --incompatible_no_implicit_file_export exposes the preexisting issue, and that this PR overcomes it - though even with this PR, I can't quite get a successful build with --incompatible_no_implicit_file_export due to a similar issue in the LLVM Bazel build.
Error log with --incompatible_no_implicit_file_export without this PR:
benoitjacob@cloud:~/iree$ bazel build --incompatible_no_implicit_file_export //runtime/src/iree/builtins/ukernel/...
INFO: Invocation ID: 52cac289-252b-476c-b020-314d661dd622
ERROR: /usr/local/google/home/benoitjacob/iree/runtime/src/iree/builtins/ukernel/BUILD.bazel:112:10: in filegroup rule //runtime/src/iree/builtins/ukernel:bitcode_internal_headers: target '//runtime/src/iree/schemas:cpu_data.h' is not visible from target '//runtime/src/iree/builtins/ukernel:bitcode_internal_headers'. Check the visibility declaration of the former target if you think the dependency is legitimate. To set the visibility of that source file target, use the exports_files() function
ERROR: /usr/local/google/home/benoitjacob/iree/runtime/src/iree/builtins/ukernel/BUILD.bazel:112:10: in filegroup rule //runtime/src/iree/builtins/ukernel:bitcode_internal_headers: target '//runtime/src/iree/schemas:cpu_feature_bits.inl' is not visible from target '//runtime/src/iree/builtins/ukernel:bitcode_internal_headers'. Check the visibility declaration of the former target if you think the dependency is legitimate. To set the visibility of that source file target, use the exports_files() function
ERROR: /usr/local/google/home/benoitjacob/iree/runtime/src/iree/builtins/ukernel/BUILD.bazel:112:10: Analysis of target '//runtime/src/iree/builtins/ukernel:bitcode_internal_headers' failed
Error log with --incompatible_no_implicit_file_export with this PR exposing the LLVM issue:
ERROR: /usr/local/google/home/benoitjacob/.cache/bazel/_bazel_benoitjacob/eaaf182bb48fe049228a18968abf1f5f/external/llvm-project/clang/BUILD.bazel:2171:16: in expand_template rule @llvm-project//clang:clang_main: target '@llvm-project//llvm:cmake/modules/llvm-driver-template.cpp.in' is not visible from target '@llvm-project//clang:clang_main'. Check the visibility declaration of the former target if you think the dependency is legitimate. To set the visibility of that source file target, use the exports_files() function
ERROR: /usr/local/google/home/benoitjacob/.cache/bazel/_bazel_benoitjacob/eaaf182bb48fe049228a18968abf1f5f/external/llvm-project/clang/BUILD.bazel:2171:16: Analysis of target '@llvm-project//clang:clang_main' failed
Let's merge this PR and separately address the remaining LLVM issue.
There was a problem hiding this comment.
Yeah let's turn on --incompatible_no_implicit_file_export upstream and here
PatriosTheGreat
force-pushed
the
main
branch
3 times, most recently
from
a17fd31
to
3bfbbac
Compare
bjacob
changed the title
bazel build --incompatible_no_implicit_file_export
| @@ -10,6 +10,7 @@ | |||
|
|
|||
| iree_add_all_subdirs() | |||
|
|
|||
| file(GLOB _GLOB_X_H LIST_DIRECTORIES false RELATIVE ${CMAKE_CURRENT_SOURCE_DIR} CONFIGURE_DEPENDS *.h) | |||
There was a problem hiding this comment.
Note @ScottTodd , I was initially confused by this line, which creates a _GLOB_X_H variable that's just unused. But it is created by bazel_to_cmake in the lowering of glob, and then it goes unused because exports_files is implemented as pass.
There was a problem hiding this comment.
I wonder if Python has a concept of macros that could be used instead of functions for the empty-implementation entry points which are currently functions doing pass, so that they could skip evaluating their arguments.
There was a problem hiding this comment.
I think @GMNGeoffrey added that file glob stuff :P
There was a problem hiding this comment.
Anyway. I say we merge this now?
There was a problem hiding this comment.
Indeed I did. I kind of think that the entire implementation of bazel->cmake should be constructing objects and then only writing at the end rather than concatenating strings. Then this sort of thing wouldn't happen. This particular behavior is not awesome because CMake globs are not awesome (which is why we have enforce_glob). Although since implementing that I think I concluded that CMake glob was not as bad as everyone made it out to be as long as you use ninja, which supports CONFIGURE_DEPENDS. Ben was seeing some slowness that he attributed to the globbing, but I think it may have been a red herring. Or maybe it was just a matter of scale.
But long aside aside, these globs appear to mostly be globbing like one file. I think we can just list that explicitly or use enforce_glob if we're worried about it falling out of sync.
It's also usually better to use filegroup rather than exports_files. In the case of schemas, for instance, the only place those files are included, they are included together into another filegroup. Should we just make this one a filegroup?
https://github.com/openxla/iree/blob/main/runtime/src/iree/builtins/ukernel/BUILD.bazel#L118
There was a problem hiding this comment.
oh right, that is the right way. working on it.
There was a problem hiding this comment.
Alright, I just pushed a change to this PR, implementing this.
And I've also sent a LLVM diff to address a similar issue in the LLVM overlay,
https://reviews.llvm.org/D152491
So, putting this together, this command actually succeeds:
bazel build --incompatible_no_implicit_file_export //runtime/src/iree/builtins/ukernel/...
The Bazel build was relying, for the two files enumerated in this diff, on the legacy implicit-export semantics described here: https://bazel.build/reference/be/functions#exports_files This documentation page encourages migrating away from this legacy behavior, and indeed we have a user who reported a Bazel build error and it appears that they were already using the new, stricter behavior: iree-org/iree#13982 and while examining fixes on our side and trying to get a clean Bazel build, I ran into this similar issue in the LLVM overlay. It would arguably be cleaner (in the sense of more structured) to rely on `filegroup` to export this, but I am insufficiently familiar with the Clang build (the dependent targets seem to be below Clang) to do this myself. The present `exports_files` solution has the merit of being localized in these few lines here. Differential Revision: https://reviews.llvm.org/D152491
…no_implicit_file_export` (iree-org#13982) Files exported to other Bazel packages need to be explictly declared with [`exports_files`](https://bazel.build/reference/be/functions#exports_files). This was currently relying on the legacy behavior of implict exporting of inputs to other rules, so this was failing to build with the stricter behavior enabled by `--incompatible_no_implicit_file_export`. --------- Co-authored-by: Benoit Jacob <benoitjacob@google.com>
…no_implicit_file_export` (iree-org#13982) Files exported to other Bazel packages need to be explictly declared with [`exports_files`](https://bazel.build/reference/be/functions#exports_files). This was currently relying on the legacy behavior of implict exporting of inputs to other rules, so this was failing to build with the stricter behavior enabled by `--incompatible_no_implicit_file_export`. --------- Co-authored-by: Benoit Jacob <benoitjacob@google.com>
Files exported to other Bazel packages need to be explictly declared with
exports_files. This was currently relying on the legacy behavior of implict exporting of inputs to other rules, so this was failing to build with the stricter behavior enabled by--incompatible_no_implicit_file_export.