-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[bazel] Add td_library target for OptParser.td #86363
[bazel] Add td_library target for OptParser.td #86363
Conversation
@@ -4,6 +4,7 @@ | |||
|
|||
load("@bazel_skylib//rules:common_settings.bzl", "string_flag") | |||
load("@bazel_skylib//rules:expand_template.bzl", "expand_template") | |||
load("//mlir:tblgen.bzl", "td_library") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is potentially a bit weird to depend on here, any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely weird, but that file also doesn't seem MLIR specific at all, so it's not like depending on that will pull in any MLIR dependency.
Maybe we should move it under the llvm tree instead? However, llvm/tblgen.bzl already exists, and defines gentbl
, whereas mlir/tblgen.bzl defines gentbl_cc_library
, gentbl_filegroup
, etc. Weird. Untangling that and updating all users to use a single tblgen bazel config is probably a big mess. I think we can go with this for now, and just update the reference to load from :tblgen.bzl
in llvm instead once we can migrate that.
@jpienaar may know more about the differences. I'm guessing it's in mlir/tblgen.bzl because it used to have MLIR specific logic there, but now seems totally generic.
✅ With the latest revision this PR passed the Python code formatter. |
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other packages are able to refer directly to OptParser.td
as exported by export_files
, e.g.
"//llvm:include/llvm/Option/OptParser.td", |
Do you know what the difference is w/ your downstream use case that including that file directly doesn't work?
That said, this seems like a step in the right direction anyway.
The issue is the downstream library doesn't have the right include paths when just importing the file vs the library which propagates its directory |
This allows downstream consumers to depend on this file. Without this target the include paths to this are mangled by bazel.
e800475
to
9b3408a
Compare
This allows downstream consumers to depend on this file. Without this target the include paths to this are mangled by bazel.