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 override point for cc_autoconf #7883

Closed
wants to merge 1 commit into from

Conversation

keith
Copy link
Member

@keith keith commented Mar 28, 2019

This adds overriden_labels to the cc_autoconf rule. This allows to
override specific tools that bazel uses when setting up
local_config_cc. The benefit there is that if you want to replace
pieces of the cc toolchain, without having to create your own crosstool
entirely, this allows you to do that.

Specifically for the osx toolchain sometimes overriding flags in
response to Xcode updates, without having to wait for a bazel release is
useful. If we're ok with this approach this could also be extended to support overrides for other platform's setup.

@keith
Copy link
Member Author

keith commented Mar 28, 2019

With this change, here's an example of how you can override a tool:

cc_autoconf(
    name = "local_config_cc",
    overriden_labels = {
        "@bazel_tools//tools/osx/crosstool:osx_archs.bzl": "@lyftios//:foo.bzl"
    },
)

bind(
    name = "cc_toolchain",
    actual = "@local_config_cc//:toolchain",
)

register_toolchains(
    "@local_config_cc//:all",
)

@keith
Copy link
Member Author

keith commented Mar 28, 2019

I wanted to submit this PR as a discussion for how we could provide easier ways for users to override specific pieces of this setup, without having to override the whole thing. Recently we've discovered some issues in the crosstool that have required us to either find a workaround, wait for the next bazel release, or create our own crosstool. Having this would make it easier to override those specific pieces temporarily, until the next bazel release.

@jin jin added the z-team-Apple Deprecated. Send to rules_apple, or label team-Rules-CPP + platform:apple label Mar 29, 2019
@aragos aragos requested a review from hlopko April 1, 2019 11:52
@aragos aragos added team-Rules-CPP Issues for C++ rules and removed z-team-Apple Deprecated. Send to rules_apple, or label team-Rules-CPP + platform:apple labels Apr 1, 2019
@aragos aragos assigned hlopko and unassigned aragos Apr 1, 2019
@keith
Copy link
Member Author

keith commented Apr 8, 2019

@hlopko can you review?

@hlopko
Copy link
Member

hlopko commented Apr 17, 2019

I grew paranoid of changing cc_configure. We have very little test coverage, and we have very frequent cherry picks into Bazel releases because of cc_configure bugs. So I'd prefer if you commented on #6926 to make sure your use case is well understood and addressed by the redesign. If there is an urgency to land this PR, let us know. Thanks!

@hlopko hlopko requested review from scentini and removed request for hlopko April 17, 2019 10:08
@hlopko hlopko assigned scentini and unassigned hlopko Apr 17, 2019
@keith keith mentioned this pull request Apr 17, 2019
@keith
Copy link
Member Author

keith commented Apr 17, 2019

We've had a significant number of issues that required crosstool changes, having to wait for releases after our changes have landed has definitely pushed us to consider creating our own crosstool quite a few times. I think forcing users towards creating their own crosstool should be an antigoal because that will lead to fewer upstream fixes to the crosstool here.

There isn't a huge specific rush from our perspective on landing this (although we did just have to upgrade to the 0.25.0rc2 because of this crosstool bug #7830 that hasn't been released yet), but if those changes aren't coming soon I think it would be great to have this interim solution.

@keith
Copy link
Member Author

keith commented Apr 22, 2019

@hlopko any other thoughts?

@keith
Copy link
Member Author

keith commented May 3, 2019

@hlopko friendly ping

@katre
Copy link
Member

katre commented May 3, 2019

@hlopko is out of office until next week. @scentini, can you comment?

@keith
Copy link
Member Author

keith commented May 16, 2019

@hlopko friendly ping

@keith
Copy link
Member Author

keith commented May 16, 2019

We hit another case where this patch would come in handy where we need this patch a8fa6e2 but can't update to the 0.26.0rc8 because of a crasher #8364

@scentini scentini requested review from hlopko and removed request for scentini May 20, 2019 16:46
@scentini scentini assigned hlopko and unassigned scentini May 20, 2019
This adds `overriden_labels` to the `cc_autoconf` rule. This allows to
override specific tools that bazel uses when setting up
`local_config_cc`. The benefit there is that if you want to replace
pieces of the cc toolchain, without having to create your own crosstool
entirely, this allows you to do that.

Specifically for the osx toolchain sometimes overriding flags in
response to Xcode updates, without having to wait for a bazel release is
useful.
@keith keith force-pushed the ks/override-configure-files branch from d3b4c84 to 0ce3c76 Compare June 26, 2019 16:07
@keith
Copy link
Member Author

keith commented Jun 26, 2019

@hlopko or @scentini any thoughts?

@hlopko
Copy link
Member

hlopko commented Jul 8, 2019

Hi Keith, sorry for keeping you waiting. I don't think we'll merge this PR before somebody sits down and thinks (and designs, and tests) how cc_configure should be customized and what is supported and part of the public contract. Right now cc_configure lives its own organic life which is not maintainable anymore.

We're also in the process of moving cc_configure to @rules_cc and slowly trying to get it under control (= have any test coverage). So I'd say it's not the right time.

@hlopko
Copy link
Member

hlopko commented Jul 8, 2019

Actually, I'm sorry but I'll close this PR now. I'll make sure to include you in the future design discussions of cc_configure. Thank you so much!

@hlopko hlopko closed this Jul 8, 2019
@keith keith deleted the ks/override-configure-files branch August 14, 2020 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants