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: convert forward slashes to backslashes for protoc plugins on Windows. #668

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

willjschmitt
Copy link

Fixes #667

This takes the approach rules_proto_grpc does to detect if we are building on Windows via the host_path_separator. protoc will fail to call into the plugins if they are expressed to protoc as forward-slash paths. Alternatively, we could make this more limited like my patch in the linked issue to check .bat file extensions, which is fragile in its own way, but works for the given use case.


Changes are visible to end-users: no

Test plan

There aren't any tests as far as I can tell currently for ts_proto_library, so my manual test is the best I can currently offer, and it's not clear if the Windows test runners are fully active, but I'd be happy to write a test for initial coverage on ts_proto_library if desired

@CLAassistant
Copy link

CLAassistant commented Aug 6, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

aspect-workflows bot commented Aug 13, 2024

Test

⚠️ GitHub Actions build #706 is currently failing.

//docs:proto.extract failed to build

in deps attribute of starlark_doc_extract rule //docs:proto.extract: missing bzl_library targets for Starlark
module(s) @aspect_bazel_lib//lib:platform_utils.bzl. Since this rule was created by the macro
'stardoc_with_diff_test', the error might have been caused by the macro implementation

//.aspect/bazelrc:update_aspect_bazelrc_presets_0_test failed to build

remote spawn failed: UNAVAILABLE: connection error: desc = "transport: Error while dialing: dial tcp: lookup remote-scheduler on 10.0.0.2:53: no such host"

💡 To reproduce the build failures, run

bazel build //docs:proto.extract //.aspect/bazelrc:update_aspect_bazelrc_presets_0_test

Buildifier

Buildifier lint check has failed

ERROR: Failed to query remote execution capabilities: Remote execution is not supported by the remote server, or the current account is not authorized to use remote execution.
WARNING: Uploading BEP referenced local file: Remote execution is not supported by the remote server, or the current account is not authorized to use remote execution.
WARNING: Uploading BEP referenced local file /mnt/ephemeral/output/rules_ts/__main__/command.profile.gz hash: "fb127741ab11bc355d261285012314b24d195e88b523a89e66985c3d05c7e7f4"
size_bytes: 9824
: Remote execution is not supported by the remote server, or the current account is not authorized to use remote execution.
./ts/private/ts_proto_library.bzl:10: unused-variable: Variable "ctx" is unused. Please remove it. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#unused-variable)

💡 To reproduce the buildifier failures, run

bazel run //:buildifier.check

Some lint failures can be fixed automatically by running the following while other require manual fixes

bazel run //:buildifier

📝 For more information on how to resolve or suppress specific lint failures, see the Buildifier documentation


Format

@jbedard
Copy link
Member

jbedard commented Sep 21, 2024

Would you be able to add some initial ts_proto_library tests? It would be great to merge some tests in and then try to reproduce+fix the windows after 🙏

@willjschmitt willjschmitt force-pushed the fix/667/windows-backslashes-ts_proto_library branch from 3ac906a to 461bb13 Compare November 4, 2024 19:22
@willjschmitt
Copy link
Author

Rebased, and the unit test that would repro and show this PR improving things is f00e046 in #671

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants