Skip to content
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

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

willjschmitt
Copy link
Contributor

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 on ctx.configuration.host_path_separator, so I've taken the same approach here as well.


Changes are visible to end-users: no

Test plan

Copy link

aspect-workflows bot commented Aug 13, 2024

Test

All tests were cache hits

150 tests (100.0%) were fully cached saving 9s.


Buildifier      Format

@jbedard
Copy link
Member

jbedard commented Sep 21, 2024

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.
@willjschmitt
Copy link
Contributor Author

@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 bazel test //... on linux and ran bazel test //ts/test:ts_proto_library_with_dep_test on windows, cherry picking #668, although unfortunately, due to aspect-build/rules_js#1739, a patch to handle bsdtar on windows is still needed. The Windows build is failing but getting past this and the issues of #667 at least

I did borrow from the node_modules in //examples/proto_grpc, since the repo root is not the pnpm workspace root, which might not be acceptable, so let me know if 1) the existing examples essentially are already the test coverage we are looking for, or 2) the pnpm workspace can/should be promoted to repo root, which I figured wouldn't be desirable

@jbedard
Copy link
Member

jbedard commented Nov 10, 2024

Looks like we just have a buildifier error, otherwise LGTM and thanks for the tests 👍

@willjschmitt
Copy link
Contributor Author

Sorry about that! Removed the extraneous imports, so we should be good to go now

@jbedard jbedard enabled auto-merge (squash) November 12, 2024 20:43
@jbedard jbedard merged commit 105f294 into aspect-build:main Nov 12, 2024
24 checks passed
@willjschmitt willjschmitt deleted the patch-1 branch November 16, 2024 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: --descriptor_set_in input files should be joined by ; on Windows rather than : in ts_proto_library
2 participants