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

Fix Bazel build instructions and elaborated them #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

camio
Copy link

@camio camio commented Nov 22, 2022

The bazel call that utilized a local LLVM installation failed with
linker errors because the link order of LLVM libraries wasn't in
dependency order. The fix is to utilize LLD via the -fuse-ld flag. LLD,
unlike traditional Unix-like linkers, allows for reverse dependencies
(aka backrefs. See the --warn-backrefs option). Note that the -fuse-id
flag was being used for the Bazel-provided LLVM installation configuration.

Several other changes were made:

  • The build system was broken into two sections: install dependencies and
    build crubit. This was done since the former usually need be done only
    once.
  • Modified the installation instructions to install bazelisk as the
    system installation of bazel was incorrect (the package name is actually
    bazel-bootstrap) and, even if it were correct, would install a version
    that is too old.
  • Clarified the two build options and the reasons why one would prefer
    one to the other.
  • Modified the local LLVM option to build and use lld. This reduces
    variance of tooling used to build this project.
  • Added $LLVM_INSTALL_PATH/bin to the PATH so this compiler is selected
    instead of any system installations.

The `bazel` call that utilized a local LLVM installation failed with
linker errors because the link order of LLVM libraries wasn't in
dependency order. The fix is to utilize LLD via the `-fuse-ld` flag. LLD,
unlike traditional Unix-like linkers, allows for reverse dependencies
(aka backrefs. See the --warn-backrefs option). Note that the `-fuse-id`
flag was being used for the Bazel-provided LLVM installation configuration.

Several other changes were made:
* The build system was broken into two sections: install dependencies and
  build crubit. This was done since the former usually need be done only
  once.
* Modified the installation instructions to install bazelisk as the
  system installation of bazel was incorrect (the package name is actually
  bazel-bootstrap) and, even if it were correct, would install a version
  that is too old.
* Clarified the two build options and the reasons why one would prefer
  one to the other.
* Modified the local LLVM option to build and use `lld`. This reduces
  variance of tooling used to build this project.
* Added `$LLVM_INSTALL_PATH/bin` to the `PATH` so this compiler is selected
  instead of any system installations.
copybara-service bot pushed a commit that referenced this pull request Sep 20, 2024
Well, there's only one right now, but it's structured so that we can do more later.

This is the second time I'm implementing this, and the second implementation is much cleaner: to track the error messages, etc., I just use a fine grained enum. That's it! Doing something with string values is a bad idea. :(

Deployment plan for this CL (and any other CLs that gate to experimental):

- [x] wait until a version of crubit that accepts feature flags is released to the stable toolchain.
- [x] change the bzl to implicitly pass in `:experimental` to all targets -- done in 1fd4aec
- [ ] it's now safe to submit gating CLs, including this one.
- [ ] once we're done gating features, apply `:experimental` _manually_ to all targets that need it, and submit a bzl change that reverts #2. (Which targets to apply `:experimental` to, can be determined by the breakage list from the CL reverting #2.)

PiperOrigin-RevId: 676632613
Change-Id: I693be7e380bfefef3db898a9f02512c678148fd9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant