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

[Bug]: ts_proto_library protoc cannot execute generated plugin js_binary *.bat file wrappers with forward-slashes #667

Open
willjschmitt opened this issue Aug 6, 2024 · 0 comments · May be fixed by #668
Labels
bug Something isn't working

Comments

@willjschmitt
Copy link

What happened?

Building a ts_proto_library (with or without connect/query enabled) on Windows, we get an error suggesting 'bazel-out' is not recognized as an internal or external command, operable program or batch file.:

bazel build :foo_ts_pb --verbose_failures
INFO: Analyzed target //:foo_ts_pb (0 packages loaded, 0 targets configured).
ERROR: C:/users/willschmitt/documents/achillea/rules_ts_repro/BUILD.bazel:15:17: Generating .js/.d.ts from //:foo_ts_pb failed: (Exit 1): protoc.exe failed: error executing ProtocGenEs command (from target //:foo_ts_pb)
  cd /d C:/users/willschmitt/_bazel_willschmitt/hqqsioi3/execroot/_main
  SET BAZEL_BINDIR=bazel-out/x64_windows-fastbuild/bin
  bazel-out\x64_windows-opt-exec-ST-d57f47055a04\bin\external\protobuf~\protoc.exe --plugin=protoc-gen-es=bazel-out/x64_windows-opt-exec-ST-d57f47055a04/bin/_foo_ts_pb.gen_es.bat --es_opt=keep_empty_files=true --es_opt=target=js+dts --es_out=bazel-out/x64_windows-fastbuild/bin --plugin=protoc-gen-connect-es=bazel-out/x64_windows-opt-exec-ST-d57f47055a04/bin/_foo_ts_pb.gen_connect_es.bat --connect-es_opt=keep_empty_files=true --connect-es_opt=target=js+dts --connect-es_out=bazel-out/x64_windows-fastbuild/bin --descriptor_set_in bazel-out/x64_windows-fastbuild/bin/foo_proto-descriptor-set.proto.bin foo.proto
# Configuration: a5a9a62db26d025cbc2fa4b4fbccebfc6b077137210c76db063d38b8ff442d14
# Execution platform: @@platforms//host:host
'DOSKEY' is not recognized as an internal or external command,
operable program or batch file.
'bazel-out' is not recognized as an internal or external command,
operable program or batch file.
--es_out: protoc-gen-es: Plugin failed with status code 1.
Target //:foo_ts_pb failed to build
INFO: Elapsed time: 1.155s, Critical Path: 0.09s
INFO: 2 processes: 2 internal.
ERROR: Build did NOT complete successfully

which suggests the forward slashes are leading to a failure to find and execute, truncating the rest of the path. Digging in, running the command directly from a shell and toying with the paths, it looks like it's only a problem with the plugin paths and the rest of the toolchain handles the proto, out-dir, and descriptor paths just fine with forward slashes. Manually running the following fails:

cd /d C:/users/willschmitt/_bazel_willschmitt/hqqsioi3/execroot/_main
SET BAZEL_BINDIR=bazel-out/x64_windows-fastbuild/bin
bazel-out\x64_windows-opt-exec-ST-d57f47055a04\bin\external\protobuf~\protoc.exe ^
    --plugin=protoc-gen-es=bazel-out/x64_windows-opt-exec-ST-d57f47055a04/bin/_foo_ts_pb.gen_es.bat ^
    --es_opt=keep_empty_files=true ^
    --es_opt=target=js+dts ^
    --es_out=bazel-out/x64_windows-fastbuild/bin ^
    --plugin=protoc-gen-connect-es=bazel-out/x64_windows-opt-exec-ST-d57f47055a04/bin/_foo_ts_pb.gen_connect_es.bat ^
    --connect-es_opt=keep_empty_files=true ^
    --connect-es_opt=target=js+dts ^
    --connect-es_out=bazel-out/x64_windows-fastbuild/bin ^
    --descriptor_set_in bazel-out/x64_windows-fastbuild/bin/foo_proto-descriptor-set.proto.bin ^
    foo.proto

while this succeeds to execute:

cd /d C:/users/willschmitt/_bazel_willschmitt/hqqsioi3/execroot/_main
SET BAZEL_BINDIR=bazel-out/x64_windows-fastbuild/bin
bazel-out\x64_windows-opt-exec-ST-d57f47055a04\bin\external\protobuf~\protoc.exe ^
    --plugin=protoc-gen-es=bazel-out\x64_windows-opt-exec-ST-d57f47055a04\bin\_foo_ts_pb.gen_es.bat ^
    --es_opt=keep_empty_files=true ^
    --es_opt=target=js+dts ^
    --es_out=bazel-out/x64_windows-fastbuild/bin ^
    --plugin=protoc-gen-connect-es=bazel-out\x64_windows-opt-exec-ST-d57f47055a04\bin\_foo_ts_pb.gen_connect_es.bat ^
    --connect-es_opt=keep_empty_files=true ^
    --connect-es_opt=target=js+dts ^
    --connect-es_out=bazel-out/x64_windows-fastbuild/bin ^
    --descriptor_set_in bazel-out/x64_windows-fastbuild/bin/foo_proto-descriptor-set.proto.bin ^
    foo.proto

Version

Development (host) and target OS/architectures: Windows 10 64bit

Output of bazel --version: 7.1.1

Version of the Aspect rules, or other relevant rules from your
WORKSPACE or MODULE.bazel file:

Language(s) and/or frameworks involved: Typescript/protobuf/rule

How to reproduce

Build a `ts_proto_library` target on Windows

Any other information?

Workaround patch for v2.4.2, which does a hacky check if the path ends with *.bat and rewrites the forward slashes to backslashes (a la https://bazel.build/rules/windows#paths):

diff --git ts/private/ts_proto_library.bzl ts/private/ts_proto_library.bzl
index 6fa8c30..5954ff9 100644
--- ts/private/ts_proto_library.bzl
+++ ts/private/ts_proto_library.bzl
@@ -8,6 +8,20 @@ load("@rules_proto//proto:proto_common.bzl", proto_toolchains = "toolchains")
 
 _PROTO_TOOLCHAIN_TYPE = "@rules_proto//proto:toolchain_type"
 
+
+def _windows_path_normalize(path):
+    """Changes forward slashs to backslashs for Windows paths.
+
+    Only works for batch files.
+    """
+    # This is a hack to check if we are on Windows, since the offending file
+    # paths are exclusively batch files that are used as the entry point for
+    # protoc plugins that are wrapped by js_binary.
+    if path.endswith(".bat"):
+        return path.replace("/", "\\")
+    return path
+
+
 # buildifier: disable=function-docstring-header
 def _protoc_action(ctx, proto_info, outputs, options = {
     "keep_empty_files": True,
@@ -24,19 +38,19 @@ def _protoc_action(ctx, proto_info, outputs, options = {
     inputs = depset(proto_info.direct_sources, transitive = [proto_info.transitive_descriptor_sets])
 
     args = ctx.actions.args()
-    args.add_joined(["--plugin", "protoc-gen-es", ctx.executable.protoc_gen_es.path], join_with = "=")
+    args.add_joined(["--plugin", "protoc-gen-es", _windows_path_normalize(ctx.executable.protoc_gen_es.path)], join_with = "=")
     for (key, value) in options.items():
         args.add_joined(["--es_opt", key, value], join_with = "=")
     args.add_joined(["--es_out", ctx.bin_dir.path], join_with = "=")
 
     if ctx.attr.gen_connect_es:
-        args.add_joined(["--plugin", "protoc-gen-connect-es", ctx.executable.protoc_gen_connect_es.path], join_with = "=")
+        args.add_joined(["--plugin", "protoc-gen-connect-es", _windows_path_normalize(ctx.executable.protoc_gen_connect_es.path)], join_with = "=")
         for (key, value) in options.items():
             args.add_joined(["--connect-es_opt", key, value], join_with = "=")
         args.add_joined(["--connect-es_out", ctx.bin_dir.path], join_with = "=")
 
     if ctx.attr.gen_connect_query:
-        args.add_joined(["--plugin", "protoc-gen-connect-query", ctx.executable.protoc_gen_connect_query.path], join_with = "=")
+        args.add_joined(["--plugin", "protoc-gen-connect-query", _windows_path_normalize(ctx.executable.protoc_gen_connect_query.path)], join_with = "=")
         for (key, value) in options.items():
             args.add_joined(["--connect-query_opt", key, value], join_with = "=")
         args.add_joined(["--connect-query_out", ctx.bin_dir.path], join_with = "=")

Also looking at rules_proto_grpc it looks like they do a similar path mangling for plugin paths on Windows here: https://github.com/rules-proto-grpc/rules_proto_grpc/blob/9e61a4c3dfc78de163febe7530bd04dabfbcca16/modules/core/internal/protoc.bzl#L79-L83, although they look by looking at ctx.configuration.host_path_separator as the heuristic for Windows

@willjschmitt willjschmitt added the bug Something isn't working label Aug 6, 2024
willjschmitt added a commit to willjschmitt/rules_ts that referenced this issue Aug 6, 2024
…dows.

Fixes aspect-build#667

This takes the approach [rules_proto_grpc does](https://github.com/rules-proto-grpc/rules_proto_grpc/blob/9e61a4c3dfc78de163febe7530bd04dabfbcca16/modules/core/internal/protoc.bzl#L79-L83) 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.
willjschmitt added a commit to willjschmitt/rules_ts that referenced this issue Aug 6, 2024
…dows.

Fixes aspect-build#667

This takes the approach [rules_proto_grpc does](https://github.com/rules-proto-grpc/rules_proto_grpc/blob/9e61a4c3dfc78de163febe7530bd04dabfbcca16/modules/core/internal/protoc.bzl#L79-L83) 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.
willjschmitt added a commit to willjschmitt/rules_ts that referenced this issue Aug 6, 2024
…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.
willjschmitt added a commit to willjschmitt/rules_ts that referenced this issue Aug 6, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant