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.
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.
@fmeum Thanks for the tip. Updated as you described.
louisbuchbinder
force-pushed
the
go_proto_compiler_data
branch
from
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.
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.
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.
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.
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.
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
dataattribute to thego_proto_compilerrule. The data files provided are then appended to the input file list in thego.actions.runwhere 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.