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

[Platform Transitions] Fix platform transitions for selecting tools. #272

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ankit-agarwal1999
Copy link

@ankit-agarwal1999 ankit-agarwal1999 commented Jul 27, 2023

I ran into an issue the other day where I have a multi-platform repo and I was compiling code for a different platform (in this case QNX).

I ran into an issue with the docs compile rule where I would get the following error:

ERROR: /home/jzahn/.cache/bazel/_bazel_jzahn/7bf1254182ebae36d2817c38fe27106c/external/rules_proto_grpc/doc/BUILD.bazel:63:13: configurable attribute "tool" in @rules_proto_grpc//doc:markdown_plugin doesn't match this configuration. Would a default condition help?

Conditions checked:
 @bazel_tools//src/conditions:darwin_arm64
 @bazel_tools//src/conditions:darwin_x86_64
 @bazel_tools//src/conditions:linux_x86_64
 @bazel_tools//src/conditions:windows

The issue was we were compiling for QNX, but even though the tool is for the execution platform, the select operates on the target platform when selecting the tool. So added an alias buffer rule in between the tool and the select so the select would only operate on the execution platform. That seemed to work, so I implemented the buffer for everywhere where I saw this pattern.

@github-actions github-actions bot added lang-buf Buf rules specific lang-doc Doc rules specific lang-js JavaScript rules specific labels Jul 27, 2023
@ankit-agarwal1999
Copy link
Author

cc @aaliddell

@aaliddell
Copy link
Member

The select shouldn't be taking the target config, as we specifically request the exec config for the tools:

"tool": attr.label(
doc = "The label of plugin binary target. Can be the output of select() to support multiple platforms. If absent, it is assumed the plugin is built-in to protoc itself and builtin_plugin_name will be used if available, otherwise the plugin name",
cfg = "exec",
allow_files = True,
executable = True,
),

The fact the select is failing indicates that your exec config may be an unsupported platform (QNX presumably), which is what needs solving here rather than resorting to host config by introducing the alias. Note that if we actually wanted to switch to host config, changing cfg = "exec" to cfg = "host" would be the easier way rather than introducing the aliases, but this would break remote build when exec != host.

Have you tried constraining the doc building rules with exec_compatible_with etc to prevent it trying to cross-compile the docs rules?

@ankit-agarwal1999
Copy link
Author

ankit-agarwal1999 commented Aug 8, 2023

Sorry, I am a bit confused by your previous comment.

According to the bazel docs:
https://bazel.build/rules/lib/toplevel/attr#parameters_2

They say that cfg can either be exec depending on whether it is building for the execution platform, or target depending on whether it is building for the target platform. I don't think the host platform is involved here (i.e there is no cfg=host). In this case I believe execution platform is the platform where bazel itself is executing on. What I am saying is happening is the proto_plugin rule is being built for the target platform (in my case QNX). Since that rule is built for the target platform, the select chooses based on the target platform, which in my case is incorrect, since the tool itself is to be executed via the execution platform.

Since I believe bazel resolves selects before it actually builds things, my understanding is if I were to compile for the target platform QNX, the proto_plugin rule would resolve its select based on the target platform. But this is incorrect. So we do the alias thing, which builds the alias with a different platform configuration (i.e for the execution platform which is Linux in our case) and that resolves to the correct execution platform.

Let me know if I have any misunderstandings here, but afaik, we aren't executing bazel on QNX. We are building for it, but that would be the target platform (ie cfg=target).

@aaliddell
Copy link
Member

So host config was dropped in v6: bazelbuild/bazel@6464f1c

That section of code I highlighted above is exactly where we are asking the proto plugin to be produced in the exec config as you are looking for, so the select should be resolved appropriately and the fact you are getting target config there indicates bazel is in the wrong configuration. So switching to an alias select is just hiding the problem rather than actually solving it. Are you using configuration transitions?

Could you follow the steps here to check your config for the relevant targets? As it stands I don't have a cross compiling setup to be able to test with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang-buf Buf rules specific lang-doc Doc rules specific lang-js JavaScript rules specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants