Use unix_cc_toolchain_config.bzl:cc_toolchain_config from @bazel_tools.#75
Conversation
|
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. |
|
Update: I think that CI fail was just a fluke? Running it again here seems to have passed. |
|
The CI fail on the last commit seems spurious; is Pushing so it'll run again. |
rrbutani
force-pushed
the
feature/use-unix-cc-toolchain-config
branch
5 times, most recently
from
b92d230 to
908b1ce
Compare
|
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? |
|
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. |
|
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. |
|
no worries; thank you for the update! 🙂 |
|
Hi @siddharthab; it's been a few months, just wanted to bump this again. |
rrbutani
force-pushed
the
feature/use-unix-cc-toolchain-config
branch
from
2da3afb to
8df8200
Compare
|
@siddharthab sorry to bother you again I noticed No worries if it is; I just don't want to keep bugging you about this PR if this is the case. |
|
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: bazel-contrib/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
left a comment
siddharthab
left a comment
There was a problem hiding this comment.
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.
siddharthab
force-pushed
the
feature/use-unix-cc-toolchain-config
branch
from
37c4562 to
8c4d4af
Compare
|
Thank you for the very thorough effort on this. I really appreciate it. |
|
@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 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! 😊 |
|
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. |
2 participants
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:
cc_toolchain_configinto a macro so that it could call theunix_cc_toolchain_config.bzl:cc_toolchain_configrulecc_toolchain_config.bzl.tplright intoBUILD.tplbut there isn't really a reason tocc_toolchain_configrule did lackunix_cc_toolchain_config.bzl:cc_toolchain_configcounterparts:llvm-stripfor llvm versions ≥7 (here)cc_toolchain_configon the Bazel version (here) because it was added in v4.0.0cc_toolchain_configrule that seems like that's the only change since pre-1.0For anyone following along at home, you should be able to try out these changes with:
Closes #23.