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

Add <rule>_variant macros #734

Merged
merged 1 commit into from
Jul 28, 2021
Merged

Add <rule>_variant macros #734

merged 1 commit into from
Jul 28, 2021

Conversation

jheaff1
Copy link
Collaborator

@jheaff1 jheaff1 commented Jul 21, 2021 •
edited
Loading

The macros utilise bazel "transitions" to set the make toolchain to
be the preinstalled nmake tool.

Note that the msvc constraint was removed from the
exec_compatible_with attribute of preinstalled_nmake_toolchain as
the condition is not actually met even when building with msvc. See
bazelbuild/bazel#7730.

This will be tested in PR #729

@jheaff1 jheaff1 mentioned this pull request Jul 21, 2021
Merged
@UebelAndre UebelAndre mentioned this pull request Jul 21, 2021
Closed
UebelAndre
UebelAndre requested changes Jul 21, 2021
View reviewed changes
foreign_cc/nmake.bzl Outdated
load(":configure.bzl", "configure_make")

def _extra_toolchains_transition_impl(settings, attrs):
return {"//command_line_option:extra_toolchains": attrs.extra_toolchains + settings["//command_line_option:extra_toolchains"]}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not too familiar with extra toolchains, can you explain how this can use a toolchain that isn't registered? What's the guarantee that the thing that's returned here is going to be used for the target if another toolchain was matched?

Copy link
Collaborator Author

@jheaff1 jheaff1 Jul 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sure

extra_toolchains are considered before those registered in a WORKSPACE file (by register_toolchains). As such, by applying the transition it is like the target is being built like

bazel build <target> --extra_toolchains="@rules_foreign_cc//toolchains:preinstalled_nmake_toolchain"

and the nmake toolchain will be considered before the toolchains registered in the WORKSPACE file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I personally prefer this approach to transitioning the host platform as I think it is less confusing.

When transitioning the host platform, the transition is from a Windows OS platform targeting MSVC to a Windows OS platform targeting MSVC. Essentially the approach leverages this issue.

foreign_cc/nmake.bzl Outdated Show resolved Hide resolved
@jheaff1 jheaff1 force-pushed the add_configure_nmake_macro branch 3 times, most recently from 2f59b54 to 91147fc Compare July 23, 2021 10:58
@jheaff1
Copy link
Collaborator Author

jheaff1 commented Jul 23, 2021 •
edited by UebelAndre
Loading

@UebelAndre How does this PR look now? To see how the new make_variant macro can be used, please see below. make_variant can be used for both make() and configure_make(). The desired rule is passed into make_variant as an argument

https://github.com/bazelbuild/rules_foreign_cc/blob/b46cec49dc1f7e32f61a9e6adee05e17287b1550/examples/third_party/openssl/BUILD.openssl.bazel#L38-L47

UebelAndre
UebelAndre requested changes Jul 23, 2021
View reviewed changes
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

This looks good to me! I had one nit but the real reason I didn't click approve was to ask if you could update the PR title and description 🙏 😄

Thanks for working so hard on this!

foreign_cc/make_variant.bzl Outdated Show resolved Hide resolved
@jheaff1 jheaff1 changed the title Add configure_nmake macro Add <rule>_variant macros Jul 23, 2021
@jheaff1
Copy link
Collaborator Author

jheaff1 commented Jul 23, 2021

This looks good to me! I had one nit but the real reason I didn't click approve was to ask if you could update the PR title and description 🙏 😄

Thanks for working so hard on this!

I've updated the PR title and description 👍

@jheaff1 jheaff1 force-pushed the add_configure_nmake_macro branch 2 times, most recently from 5992d02 to e608dcf Compare July 23, 2021 15:59
@UebelAndre UebelAndre requested a review from jsharpe July 24, 2021 01:38
UebelAndre
UebelAndre approved these changes Jul 24, 2021
View reviewed changes
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for working with me on this and all the hard work you've put in!

Buildifier seems unhappy and would like @jsharpe to take a look too but I'm happy here!

@jheaff1
Copy link
Collaborator Author

jheaff1 commented Jul 25, 2021

Looks good to me! Thanks for working with me on this and all the hard work you've put in!

Buildifier seems unhappy and would like @jsharpe to take a look too but I'm happy here!

My pleasure! :)

@jheaff1
Copy link
Collaborator Author

jheaff1 commented Jul 28, 2021

Hi @jsharpe,

If you are happy with this PR, could you please merge it? 😁

jsharpe
jsharpe requested changes Jul 28, 2021
View reviewed changes
Copy link
Member

@jsharpe jsharpe left a comment

Choose a reason for hiding this comment

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

Apologies for the delay in getting round to reviewing this.

One small bit of logic to fix up around the handling of tags but otherwise I think I'm happy with this.

foreign_cc/private/transitions.bzl Outdated Show resolved Hide resolved
@jheaff1
Add <rule>_variant macros
52da86b 
The macros utilise bazel "transitions" to set the `make` toolchain used
in the configure_make(), cmake() or make() rules to
a given make variant toolchain, e.g. preinstalled_nmake.

Note that the msvc constraint was removed from the
`exec_compatible_with` attribute of `preinstalled_nmake_toolchain` as
the condition is not actually met even when building with msvc. See
bazelbuild/bazel#7730.

This will be tested in PR#729
@jsharpe jsharpe merged commit 2e61d95 into bazel-contrib:main Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UebelAndre UebelAndre UebelAndre approved these changes

@jsharpe jsharpe jsharpe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jheaff1 @jsharpe @UebelAndre