Use `unix_cc_toolchain_config.bzl:cc_toolchain_config` from `@bazel_tools`. #75

rrbutani · 2021-02-09T08:53:29Z

A first pass at #23.

(I was trying to just copy over the bits needed to get LTO to work but it ended up being easier to just do #23).

This still needs to be tested and such but I thought I'd open a PR at this point to make sure the general approach here is okay.

A few things I should mention:

as the commit mentions, I had to turn cc_toolchain_config into a macro so that it could call the unix_cc_toolchain_config.bzl:cc_toolchain_config rule
- this was surprisingly hassle free!
- this also means that we could just inline everything in cc_toolchain_config.bzl.tpl right into BUILD.tpl but there isn't really a reason to
a couple of things the existing cc_toolchain_config rule did lack unix_cc_toolchain_config.bzl:cc_toolchain_config counterparts:
- the make vars
- the framework paths feature (only used on macOS)
I changed the tool paths to use llvm-strip for llvm versions ≥7 (here)
I'm not confident that I didn't break compatibility with older versions of Bazel
- I had to gate one of the args to cc_toolchain_config on the Bazel version (here) because it was added in v4.0.0
- looking at the history for the attribute list of the cc_toolchain_config rule that seems like that's the only change since pre-1.0
- but I've only tested these changes on Linux with Bazel 3.7.2 and 4.0.0

For anyone following along at home, you should be able to try out these changes with:

http_archive(
    name = "com_grail_bazel_toolchain",
    strip_prefix = "bazel-toolchain-feature-use-unix-cc-toolchain-config",
    urls = ["https://github.com/rrbutani/bazel-toolchain/archive/feature/use-unix-cc-toolchain-config.tar.gz"],
)

Closes #23.

rrbutani · 2021-02-09T10:04:01Z

I think the remaining CI fail is the same as this issue; not sure how to fix.

Update: I cannot reproduce this fail locally on macOS 11.2 or macOS 10.14.

rrbutani · 2021-02-10T08:37:20Z

Update: I think that CI fail was just a fluke? Running it again here seems to have passed.

rrbutani · 2021-02-13T09:35:22Z

The CI fail on the last commit seems spurious; is secrets.BAZELISK_GITHUB_TOKEN actually set/up to date?

Pushing so it'll run again.

rrbutani · 2021-04-15T18:38:20Z

Hey @siddharthab just wanted to bump this in case it got buried.

I realize that this is potentially a big change; is there anything I can do to make this PR easier for you to review?

siddharthab · 2021-04-16T06:11:21Z

My apologies. I have been distracted with other priorities and have not been able to pay attention here. I have very recently started paying attention to our open source projects, and have just finished wrapping up my work on rules_r. I will be getting to this repo in 1 or 2 days, and wrap up all the pending PRs and issues here. I will also be giving some contributors collaborator status so they will be able to merge the PRs themselves. Thank you for your patience.

siddharthab · 2021-05-05T02:04:06Z

Just a small note to say that I have not forgotten about this. I am trying to take time out, but things have been busy. Please give me a few more days.

rrbutani · 2021-05-05T04:18:15Z

no worries; thank you for the update! 🙂

rrbutani · 2021-08-15T14:29:35Z

Hi @siddharthab; it's been a few months, just wanted to bump this again.

rrbutani · 2021-09-14T00:17:38Z

@siddharthab sorry to bother you again

I noticed grailbio/bazel-compilation-database is in maintenance mode now; is this the plan for bazel-toolchain too?

No worries if it is; I just don't want to keep bugging you about this PR if this is the case.

siddharthab · 2021-09-14T01:28:45Z

No no. I should be the one apologizing. I finally got some time this month so going through the open source repos one by one. The other one was easier so I worked on all the PRs and issues there first. This repo is now my highest priority thing to do and I am going through the items one by one.

For this repo, my plan is to resolve almost all currently open items, and then give you and others write-access so I am not a blocker anymore.

…_tools (See bazel-contrib#23). Since `@bazel_tools//tools/cpp:unix_cc_toolchain_config.bzl:cc_toolchain_config` is a rule, our `cc_toolchain_config` needs to become a macro instead of a rule. Luckily we actually can do this without introducing any breaking changes. Other than the discrepancies listed within (the things marked with `## NOTE:` — make vars and framework paths) everything seemed to just fall into place. So far it seems to _just work_ 🤞. llvm_toolchain: formatting and cleanup llvm_toolchain: fix a typo tests: bump rules_go see: bazelbuild/rules_go#2720 llvm_toolchain: unbreak linking on macOS It's true that modern versions of ld.lld support start and end groups but we don't use ld.lld on macOS (yet). llvm_toolchain: don't break when used with dev builds of bazel llvm_toolchain: fixes for `builtin_sysroot` and older Bazel versions llvm_toolchain: fixes for using/running the toolchain from other locations in the execroot As the message within explains, `rules_foreign_cc` will use a different PWD than the execroot root for cmake targets which breaks the wrapper script. This commit has the wrapper script search alongside itself for `clang` when it isn't in the usual places. llvm_toolchain: don't use an empty sysroot for `builtin_sysroot` Bazel complains about this; see: https://stackoverflow.com/questions/62451307/specify-sysroot-for-bazel-toolchain for Linux the default default sysroot is "" so we hit this error

siddharthab

Sending you my observations. I will be pushing resolutions to these to your branch. Also, I squashed the commits on your branch because it is easier to rebase a single commit.

toolchain/cc_wrapper.sh.tpl

WORKSPACE

toolchain/cc_toolchain_config.bzl.tpl

siddharthab · 2021-09-21T01:42:50Z

Thank you for the very thorough effort on this. I really appreciate it.

rrbutani · 2021-09-21T13:25:53Z

@siddharthab thanks so much!

also, sorry – I should have posted something about this on this PR last week: a couple of things came up in #85 and #90 that made it seem desirable to not use @bazel_tools' cc_toolchain_config since it limits our ability to accurately represent the supported features for a particular target (for #85) and to add extra features to the toolchain like sanitizers (#90).

Definitely not worth reverting this PR over or anything; I think merging this PR makes sense regardless and it's more a question about where to go afterwards. If you have some time, I'd love to get your take. No worries if not.

And thanks again! 😊

siddharthab · 2021-09-21T15:35:43Z

Yes, I saw those comments before merging. I think the way forward would be that we can copy unix_cc_toolchain_config and make our local modifications there, and try to upstream it. It will be easier for us to rebase on any upstream changes as well.

rrbutani force-pushed the feature/use-unix-cc-toolchain-config branch 5 times, most recently from b92d230 to 908b1ce Compare February 19, 2021 11:43

rrbutani force-pushed the feature/use-unix-cc-toolchain-config branch from 2da3afb to 8df8200 Compare August 15, 2021 14:43

rrbutani mentioned this pull request Aug 16, 2021

extra_targets, user configurable toolchains, and wasm support #85

Closed

rrbutani mentioned this pull request Sep 3, 2021

Apple Silicon toolchain support #88

Closed

rrbutani mentioned this pull request Sep 15, 2021

Adding support for cross compilation, CUDA, and sanitizers #90

Closed

siddharthab reviewed Sep 21, 2021

View reviewed changes

siddharthab force-pushed the feature/use-unix-cc-toolchain-config branch from 37c4562 to 8c4d4af Compare September 21, 2021 01:24

siddharthab merged commit b20fe21 into bazel-contrib:master Sep 21, 2021

fmeum mentioned this pull request Dec 21, 2021

High rate of spurious CI failures on macOS machines bazelbuild/bazel#14459

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `unix_cc_toolchain_config.bzl:cc_toolchain_config` from `@bazel_tools`. #75

Use `unix_cc_toolchain_config.bzl:cc_toolchain_config` from `@bazel_tools`. #75

rrbutani commented Feb 9, 2021 •

edited

Loading

rrbutani commented Feb 9, 2021 •

edited

Loading

rrbutani commented Feb 10, 2021

rrbutani commented Feb 13, 2021 •

edited

Loading

rrbutani commented Apr 15, 2021 •

edited

Loading

siddharthab commented Apr 16, 2021

siddharthab commented May 5, 2021

rrbutani commented May 5, 2021

rrbutani commented Aug 15, 2021

rrbutani commented Sep 14, 2021

siddharthab commented Sep 14, 2021

siddharthab left a comment

siddharthab commented Sep 21, 2021

rrbutani commented Sep 21, 2021

siddharthab commented Sep 21, 2021

Use unix_cc_toolchain_config.bzl:cc_toolchain_config from @bazel_tools. #75

Use unix_cc_toolchain_config.bzl:cc_toolchain_config from @bazel_tools. #75

Conversation

rrbutani commented Feb 9, 2021 • edited Loading

rrbutani commented Feb 9, 2021 • edited Loading

rrbutani commented Feb 10, 2021

rrbutani commented Feb 13, 2021 • edited Loading

rrbutani commented Apr 15, 2021 • edited Loading

siddharthab commented Apr 16, 2021

siddharthab commented May 5, 2021

rrbutani commented May 5, 2021

rrbutani commented Aug 15, 2021

rrbutani commented Sep 14, 2021

siddharthab commented Sep 14, 2021

siddharthab left a comment

Choose a reason for hiding this comment

siddharthab commented Sep 21, 2021

rrbutani commented Sep 21, 2021

siddharthab commented Sep 21, 2021

Use `unix_cc_toolchain_config.bzl:cc_toolchain_config` from `@bazel_tools`. #75

Use `unix_cc_toolchain_config.bzl:cc_toolchain_config` from `@bazel_tools`. #75

rrbutani commented Feb 9, 2021 •

edited

Loading

rrbutani commented Feb 9, 2021 •

edited

Loading

rrbutani commented Feb 13, 2021 •

edited

Loading

rrbutani commented Apr 15, 2021 •

edited

Loading