-
Notifications
You must be signed in to change notification settings - Fork 650
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
allow data input to go_proto_compiler #3733
base: master
Are you sure you want to change the base?
allow data input to go_proto_compiler #3733
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
proto/compiler.bzl
Outdated
@@ -117,13 +117,14 @@ def go_proto_compile(go, compiler, protos, imports, importpath): | |||
args.add_all(imports, before_each = "-import") | |||
args.add_all(proto_paths.keys()) | |||
args.use_param_file("-param=%s") | |||
data = [file for target in compiler.internal.data for file in target.files.to_list()] |
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 problematic as it flattens a depset
with to_list()
, which should always be avoided.
Instead, could you collect the individual target.files
in a list and pass them to the transitive
parameter of depset
below?
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.
@fmeum Thanks for the tip. Updated as you described.
3f719f7
to
79a8081
Compare
@@ -118,14 +118,15 @@ def go_proto_compile(go, compiler, protos, imports, importpath): | |||
args.add_all(imports, before_each = "-import") | |||
args.add_all(proto_paths.keys()) | |||
args.use_param_file("-param=%s") | |||
data_depsets = [target.files for target in compiler.internal.data] |
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.
How does your compiler learn of the paths of these extra files? This will add them to the sandbox, but it doesn't tell the compiler or plugin where to look for them. Paths of generated files have the bazel-out/k8-exec-ST-some-hash
bit in them which makes it impossible to hardcode them.
I'm wondering whether it would be better to add them to the runfiles of the plugin
executable instead, which would allow the plugin to find them using their runfiles path via the Go runfiles 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.
The idea of this change is that the data targets become available during the runtime of the compiler. In my case the compiler knows where to look for a file based on the name of the proto file being processed. The file paths of the data targets are available as relative paths from the working directory when the compiler is executed.
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.
I see, so the data files required depend on the input files and aren't fixed for this particular plugin?
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.
That is correct. In my case I am doing two protoc passes. Both use the same .proto
input files, but the second uses the output of the first as the data
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.
Sorry for the delay, I forgot to reply.
I'm still not sure I understand how this can work, but I guess it does :-) I have some reservations about merging this as the data
attributes of executable rules almost universally end up adding runfiles to actions using the executable, not input files. I would feel much more comfortable supporting adding runfiles to the compiler.
Have you been able to try using runfiles for your purpose instead? That would also resolve the questions of how the file paths are known at execution time: runfiles paths don't contain the bazel-out
path segments.
Bumping this one. Please let me know if this change is acceptable |
What type of PR is this?
Feature
What does this PR do? Why is it needed?
Currently the go_proto_compiler rule accepts custom proto compiler executables, but not the ability to pass custom input data in to the compiler runtime.
This change adds a
data
attribute to thego_proto_compiler
rule. The data files provided are then appended to the input file list in thego.actions.run
where the proto compiler is executed.Which issues(s) does this PR fix?
Fixes #3732
Other notes for review
Let me know what you are looking for regarding tests and I would be happy to add.