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 example for multi-arch binary #160

Merged
merged 6 commits into from May 15, 2020
Merged

add example for multi-arch binary #160

merged 6 commits into from May 15, 2020

Conversation

juliexxia
Copy link
Contributor

@juliexxia juliexxia commented May 12, 2020

Final step for bazelbuild/bazel#5575

Note - this logic isn't actually released in bazel yet but I think it's fine to document it prematurely.

@juliexxia
Copy link
Contributor Author

I guess checks won't pass until the logic is in bazel...

Copy link
Collaborator

@gregestren gregestren left a comment

Choose a reason for hiding this comment

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

Same README.md comment as the other review too. :)

implementation = _rule_impl,
attrs = {
"tool": attr.label(cfg = fat_transition),
'_whitelist_function_transition': attr.label(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's comment why this is here, and in every other example that uses this attribute. I know users have been confused by it before.

Do we have a good canonical link to reference that explains this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. How does https://docs.bazel.build/versions/master/skylark/config.html#user-defined-transitions sound? it's where I've been linking to for other stuff.

rules/starlark_configurations/multi_arch_binary/defs.bzl Outdated Show resolved Hide resolved
# The values of `x86_dep` and `armeabi-v7a_dep` here are regular
# dependencies with providers.
x86_dep = tools['x86-platform']
armeabi_v7a_dep = tools['armeabi-v7a-platform']
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have an idea that makes the example a bit more elaborate, but could really drive the point home by showing explicitly in the output what's going on.

What about having _impl below return a provider with a string based on its CPU, like SimpleProvider(my_info= "My CPU is X"). Then have print() statements in _rule_impl that say "tools['x86-platform'] provider says: <whatever that instance of the provider says".

If it's unclear what I'm saying, let me know and I'll share some more specific code snippets to demonstrate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense to me - do you mean by somehow pulling the cpu out of ctx? Is that possible? I know about ctx.fragments but I don't think cpu lives in any of those: https://docs.bazel.build/versions/master/skylark/lib/skylark-configuration-fragment.html

If we wanted to not use cpu/a native option and do a starlark option this would definitely be possible but I feel like it would be nice to keep this example simple AND i feel like cpu really hits home with people for this example.

@gregestren
Copy link
Collaborator

Oh, and if bazelbuild/examples automatically tries to build whatever gets checked in, will we have a problem if it's not released yet?

@gregestren
Copy link
Collaborator

Check out bazelbuild/bazel#11077. This might actually get released soon.

@juliexxia
Copy link
Contributor Author

I temporarily disabled it with a TODO back to this issue to re-enable when the release happens

@juliexxia
Copy link
Contributor Author

Added cpu printouts!

@juliexxia juliexxia merged commit 8a007ec into master May 15, 2020
@aiuto aiuto deleted the fat branch June 14, 2021 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants