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

Modify clang detection to build clang #261

Merged
merged 32 commits into from
Feb 9, 2021
Merged

Modify clang detection to build clang #261

merged 32 commits into from
Feb 9, 2021

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Feb 6, 2021

Start automatically building clang/llvm, dropping it from contribution tool docs.

(this may need a little more work, but I wanted to push state for you if you want to look)

@jonmeow jonmeow requested review from chandlerc and a team February 6, 2021 01:34
@google-cla google-cla bot added the cla: yes PR meets CLA requirements according to bot. label Feb 6, 2021
First, this builds inside the Bazel tree rather than in the source
repository. This ensures we start in a clean directory each time the
repository rule is run again. We don't need to detect an existing build
with this, and it will only be rebuilt when the workspace file or the
repository rule implementation is changed. This can be forced by using:
```
bazel sync --configure
```

Second, we use an implicit dependency on the `WORKSPACE` file to locate
the workspace directory automatically, and the `HEAD` file from the
`llvm-project` submodule to trigger a rebuild if the submodule is
updated.

Third, teach the CMake script to try to use a system-installed `clang`
if installed and not overridden by the `CC` environment variable. This
is very different from the prior logic -- this is only used with the
CMake build, and so should work with any system C++ compiler that can
build Clang and LLVM.

Lastly, this tweaks the CMake options to tune this build given that we
now fully control it and it will only be used in this context. This
still results in a 1.5gb build for me. =/ But its as small as I can make
it really. It's a frustrating long list, but I couldn't find a more
brief way of representing this.
@google-cla
Copy link

google-cla bot commented Feb 6, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no PR does not meet CLA requirements according to bot. and removed cla: yes PR meets CLA requirements according to bot. labels Feb 6, 2021
@chandlerc
Copy link
Contributor

chandlerc commented Feb 6, 2021

I... didn't expect that to work.

Sorry for just pushing a bunch of changes without chatting Jon! Let me know if you'd like me to back them out or do a follow-up PR or something.

Anyways, see the commit log for all the details -- basically a bunch of tweaks to integrate this path a bit more fully into the Bazel workflow. I actually think this result is really, really nice. We basically bootstrap a fully modern C++ toolchain from whatever is on the system relying on the super widely supported CMake build, and then Bazel takes right off. I think I've even got all the dependencies wired up here right.

From the commit log:


First, this builds inside the Bazel tree rather than in the source
repository. This ensures we start in a clean directory each time the
repository rule is run again. We don't need to detect an existing build
with this, and it will only be rebuilt when the workspace file or the
repository rule implementation is changed. This can be forced by using:

bazel sync --configure

Second, we use an implicit dependency on the WORKSPACE file to locate
the workspace directory automatically, and the HEAD file from the
llvm-project submodule to trigger a rebuild if the submodule is
updated.

Third, teach the CMake script to try to use a system-installed clang
if installed and not overridden by the CC environment variable. This
is very different from the prior logic -- this is only used with the
CMake build, and so should work with any system C++ compiler that can
build Clang and LLVM.

Lastly, this tweaks the CMake options to tune this build given that we
now fully control it and it will only be used in this context. This
still results in a 1.5gb build for me. =/ But its as small as I can make
it really. It's a frustrating long list, but I couldn't find a more
brief way of representing this.

@chandlerc
Copy link
Contributor

@googlebot I consent.

@google-cla google-cla bot added cla: yes PR meets CLA requirements according to bot. and removed cla: no PR does not meet CLA requirements according to bot. labels Feb 6, 2021
@jonmeow
Copy link
Contributor Author

jonmeow commented Feb 8, 2021

First, this builds inside the Bazel tree rather than in the source repository.

I'm leaving this in, but note there are pros and cons to this, it means there are no incremental benefits. Everything is always built from scratch.

Second, we use an implicit dependency on the WORKSPACE file to locate

Thanks!

Third, teach the CMake script to try to use a system-installed clang

I'm proposing backing this out -- as discussed, with a default clang install (sudo apt install clang) I cannot compile (the std::atomic error).

Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

LGTM for now I think!

Just some comment tweaks below...

jonmeow and others added 5 commits February 8, 2021 17:12
@jonmeow jonmeow merged commit 153157a into carbon-language:trunk Feb 9, 2021
@jonmeow jonmeow deleted the build-clang branch February 9, 2021 01:18
chandlerc added a commit that referenced this pull request Jun 28, 2022
Per chandlerc's comment:

First, this builds inside the Bazel tree rather than in the source
repository. This ensures we start in a clean directory each time the
repository rule is run again. We don't need to detect an existing build
with this, and it will only be rebuilt when the workspace file or the
repository rule implementation is changed. This can be forced by using:
```
bazel sync --configure
```

Second, we use an implicit dependency on the `WORKSPACE` file to locate
the workspace directory automatically, and the `HEAD` file from the
`llvm-project` submodule to trigger a rebuild if the submodule is
updated.

Third, teach the CMake script to try to use a system-installed `clang`
if installed and not overridden by the `CC` environment variable. This
is very different from the prior logic -- this is only used with the
CMake build, and so should work with any system C++ compiler that can
build Clang and LLVM.

Lastly, this tweaks the CMake options to tune this build given that we
now fully control it and it will only be used in this context. This
still results in a 1.5gb build for me. =/ But its as small as I can make
it really. It's a frustrating long list, but I couldn't find a more
brief way of representing this.

Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR meets CLA requirements according to bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants