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.

Sign up for GitHub

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

Jump to bottom

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
Open

fix: convert forward slashes to backslashes for protoc plugins on Windows. #668

wants to merge 3 commits into from
+12 −3

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

@willjschmitt
fix: convert forward slashes to backslashes for protoc plugins on Win…
ba35552 
…dows.

Fixes aspect-build#667

This takes the approach used [elsewhere in rules_ts](https://github.com/aspect-build/rules_ts/blob/da284adac1fcb6bd9da81ce5d4b3e29660ecface/ts/private/ts_project.bzl#L80) to detect if we are building on Windows. protoc will fail to call into the plugins if they are expressed to protoc as forward-slash paths.
@CLAassistant
Copy link

CLAassistant commented Aug 6, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@willjschmitt willjschmitt mentioned this pull request Aug 11, 2024
Open
@aspect-workflows Aspect Workflows
Copy link

aspect-workflows bot commented Aug 13, 2024 •
edited
Loading

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 🙏

@jbedard jbedard mentioned this pull request Sep 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@willjschmitt @CLAassistant @jbedard