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

llvm: various improvements #83017

Closed
wants to merge 1 commit into from
Closed

Conversation

carlocab
Copy link
Member

@carlocab carlocab commented Aug 9, 2021

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

  1. Enable bootstrap build on Linux. This fails with gcc-5. Let's try again when we switch to Ubuntu 18.04.
  2. Clean up some duplication
  3. Install the lit command line tool
  4. Install Vim editor integration
  5. Add patch to fix parallel builds on ARM

The lit tool adds very little to the bottle size, and it seems some
users might find it useful. Let's install it directly using Python to
avoid having to set LLVM_INCLUDE_TESTS to ON, as that has many other
side-effects that we don't need or want. This is also how Arch installs
lit. [1]

Closes #82479.

[1] https://github.com/archlinux/svntogit-packages/blob/5e7a8f09f53d0fb19c0680bb5b4691b1c6a78ad1/trunk/PKGBUILD#L79-L82

@carlocab carlocab added CI-long-timeout Use longer GitHub Actions CI timeout. CI-linux-self-hosted Build on Linux self-hosted runner CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. labels Aug 9, 2021
@BrewTestBot BrewTestBot added the python Python use is a significant feature of the PR or issue label Aug 9, 2021
@carlocab
Copy link
Member Author

carlocab commented Aug 9, 2021

On Linux:

CMake Error at /tmp/llvm-20210809-5953-1takjgm/llvm-project-12.0.1.src/compiler-rt/lib/crt/CMakeLists.txt:71 (message):
  gcc-5: error: unrecognized command line option ‘-Wimplicit-fallthrough’

  gcc-5: error: unrecognized command line option
  ‘-Wcovered-switch-default’

  gcc-5: error: unrecognized command line option ‘-Wstring-conversion’

Call Stack (most recent call first):
  /tmp/llvm-20210809-5953-1takjgm/llvm-project-12.0.1.src/compiler-rt/lib/crt/CMakeLists.txt:92 (check_cxx_section_exists)

Enabling the bootstrap on Linux might have to wait until we switch to Ubuntu 18.04.

Formula/llvm.rb Outdated
sdk = MacOS.sdk_path_if_needed
# gcc-5 fails at building compiler-rt. Enable PGO
# build on Linux when we switch to Ubuntu 18.04.
pgo_build = false
on_macos do
args << "-DLLVM_BUILD_LLVM_C_DYLIB=ON"
args << "-DLLVM_ENABLE_LIBCXX=ON"
args << "-DLLVM_CREATE_XCODE_TOOLCHAIN=#{MacOS::Xcode.installed? ? "ON" : "OFF"}"
Copy link
Member

Choose a reason for hiding this comment

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

This will always be ON on CI I think

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean LLVM_CREATE_XCODE_TOOLCHAIN? That's true. I'll move it out of the on_macos block so we don't need to declare it in two places.

Formula/llvm.rb Outdated Show resolved Hide resolved
1. Add patch to fix parallel builds on ARM.
2. Clean up some duplication
3. Install the `lit` command line tool
4. Install Vim editor integration

The `lit` tool adds very little to the bottle size, and it seems some
users might find it useful. Let's install it directly using Python to
avoid having to set `LLVM_INCLUDE_TESTS` to `ON`, as that has many other
side-effects that we don't need or want. This is also how Arch installs
`lit`. [1]

Closes Homebrew#82479.

[1] https://github.com/archlinux/svntogit-packages/blob/5e7a8f09f53d0fb19c0680bb5b4691b1c6a78ad1/trunk/PKGBUILD#L79-L82
@danielnachun
Copy link
Member

On Linux:

CMake Error at /tmp/llvm-20210809-5953-1takjgm/llvm-project-12.0.1.src/compiler-rt/lib/crt/CMakeLists.txt:71 (message):
  gcc-5: error: unrecognized command line option ‘-Wimplicit-fallthrough’

  gcc-5: error: unrecognized command line option
  ‘-Wcovered-switch-default’

  gcc-5: error: unrecognized command line option ‘-Wstring-conversion’

Call Stack (most recent call first):
  /tmp/llvm-20210809-5953-1takjgm/llvm-project-12.0.1.src/compiler-rt/lib/crt/CMakeLists.txt:92 (check_cxx_section_exists)

Enabling the bootstrap on Linux might have to wait until we switch to Ubuntu 18.04.

I don't want to waste too much time hunting this down right now, but I'm puzzled as to why this fails when we are actually building compiler-rt with gcc-5 at some point. If there was a way to drop these flags from being added, then it would probably work. But we don't have to hold up progress on the rest of this just to get this figured out.

@carlocab
Copy link
Member Author

carlocab commented Aug 9, 2021

That's not true -- we've never built compiler-rt with gcc-5. Recall that we pass compiler-rt in LLVM_ENABLE_RUNTIMES and not LLVM_ENABLE_PROJECTS.

@carlocab
Copy link
Member Author

Linux runner timed out at testing dependents. This probably needs to be run on the self-hosted runner now.

@carlocab
Copy link
Member Author

I think it'll be good to wait until Homebrew/brew#10769 is merged for this, as it'll be useful to rebuild llvm when we change the Big Sur SDK path.

Also, I should probably write a proper test for lit, as I think this formula being keg-only could make things a bit weird.

@carlocab carlocab added CI-linux-self-hosted Build on Linux self-hosted runner do not merge labels Aug 11, 2021
@carlocab carlocab mentioned this pull request Aug 22, 2021
6 tasks
@carlocab
Copy link
Member Author

carlocab commented Sep 1, 2021

Incorporated into #84364.

@carlocab carlocab closed this Sep 1, 2021
@carlocab carlocab deleted the llvm-lit-etc branch September 1, 2021 14:36
@github-actions github-actions bot added the outdated PR was locked due to age label Oct 2, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-linux-self-hosted Build on Linux self-hosted runner CI-long-timeout Use longer GitHub Actions CI timeout. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. outdated PR was locked due to age python Python use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants