-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
fix: join --descriptor_set_in
with host path separator
#671
Conversation
Can you rebase this to kickoff CI again? I'm not sure why it's red atm. This is simple enough (and a noop on *nix) I think we can merge it, but if you have a chance to add some tests along with those for #668 that would be great. |
Fixes aspect-build#670. As described in aspect-build#670, protoc splits the arguments to `--proto_path` and `--descriptor_set_in` using an OS-specific path-separator. On posix, this is `:`, but on Windows this is `;`. The protobuf library takes the approach for its bazel rules to join on `ctx.configuration.host_path_separator`, so I've taken the same approach here as well.
This only simply tests a basic message that depends on a message in another package, but for the scope of the current issue is appropriate. It's probably worth testing grpc functionality, too, but the examples package provides coverage.
@jbedard sorry for the delay, I was out on baby bonding leave and just am getting back in. I rebased, but I don't think it's auto kicking off a build. I manually ran I did borrow from the node_modules in |
Looks like we just have a buildifier error, otherwise LGTM and thanks for the tests 👍 |
Sorry about that! Removed the extraneous imports, so we should be good to go now |
Fixes #670.
As described in #670, protoc splits the arguments to
--proto_path
and--descriptor_set_in
using an OS-specific path-separator. On posix, this is:
, but on Windows this is;
. The protobuf library takes the approach for its bazel rules to join onctx.configuration.host_path_separator
, so I've taken the same approach here as well.Changes are visible to end-users: no
Test plan
https://github.com/AchilleaResearch/rules_ts_667/tree/repro_670 demonstrates the fix as a patch.
bazel build //:foo_ts_pb
on Windows with and without the patch https://github.com/AchilleaResearch/rules_ts_667/blob/repro_670/MODULE.bazel#L47