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

Add platform_transition_binary rule #289

Merged
merged 3 commits into from
Dec 14, 2022

Conversation

kormide
Copy link
Collaborator

@kormide kormide commented Nov 22, 2022

@alexeagle this was mostly copy-pasta'ed from bazelbuild/rules_rust#276 (comment).

Transitions are a new topic in Bazel for me. I'm not sure how to properly exercise or test the rule

@kormide kormide requested a review from alexeagle November 22, 2022 00:41
@alexeagle
Copy link
Collaborator

@f0rmiga and @illicitonion would be good reviewers

@illicitonion
Copy link

Looks great, thanks!

Test-wise, I'd suggest an integration test in lib/tests/transitions/BUILD.bazel that looks something like:

go_binary(
    name = "generic_go_binary",
    srcs = [...],
    tags = ["manual"],
)

platform_transition_binary(
    name = "transitioned_go_binary_x86_64",
    target_platform = ":x86_64_linux",
)

platform_transition_binary(
    name = "transitioned_go_binary_armv7_linux",
    target_platform = ":armv7_linux",
)

sh_test(
    name = "test_go_binary_is_x86_64",
    srcs = [...],
    data = [":transitioned_go_binary_x86_64"],
    args = ["$(rootpath :transitioned_go_binary_x86_64)"],
)

and then a sh_test which runs file "$(realpath "$1")" and asserts the output looks like it's for the right platform.

lib/transitions.bzl Outdated Show resolved Hide resolved
@alexeagle
Copy link
Collaborator

add me back as a reviewer once the test is added?

@alexeagle alexeagle removed their request for review December 1, 2022 18:37
@kormide
Copy link
Collaborator Author

kormide commented Dec 1, 2022

add me back as a reviewer once the test is added?

Yup, sorry I've been meaning to get back to this one.

@kormide
Copy link
Collaborator Author

kormide commented Dec 1, 2022

I'm getting this error when I try to declare a platform_transition_binary with a target platform that doesn't match my linux x64 machine (for the tests):

ERROR: /home/derek/.cache/bazel/_bazel_derek/cb18fde45e94a264d41b0ba5b41ae079/external/io_bazel_rules_go/BUILD.bazel:86:17: While resolving toolchains for target @io_bazel_rules_go//:cgo_context_data: No matching toolchains found for types @bazel_tools//tools/cpp:toolchain_type.

Is there something I need to do to register a cpp toolchain?

@f0rmiga
Copy link
Member

f0rmiga commented Dec 2, 2022

It depends. You may be able to disable CGO if no C code is being used. What target needs it?

@illicitonion
Copy link

Yeah, I suspect setting pure = "on" will make this go away, but if you push a WIP I'm happy to play around with it locally!

@kormide
Copy link
Collaborator Author

kormide commented Dec 6, 2022

Yeah, I suspect setting pure = "on" will make this go away, but if you push a WIP I'm happy to play around with it locally!

@illicitonion I tried pure = "on" but that didn't solve the missing cpp toolchain issue. I pushed up what I have locally to reproduce it.

Copy link

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should fix the issue :)

I've also filed bazel-contrib/rules_go#3390 to make this work slightly more by default

lib/tests/transitions/BUILD.bazel Outdated Show resolved Hide resolved
@kormide kormide force-pushed the transition-binary branch 2 times, most recently from 4a8685a to 0e11922 Compare December 9, 2022 05:03
lib/transitions.bzl Outdated Show resolved Hide resolved
@kormide kormide requested review from f0rmiga and removed request for illicitonion December 9, 2022 05:08
Copy link

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful, thanks so much!!

@kormide kormide merged commit 72a2621 into bazel-contrib:main Dec 14, 2022
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.

4 participants