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.

Sign up for GitHub

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

Jump to bottom

doc: add LLVM instruction for macOS < 13 #29934

Merged
merged 1 commit into from
May 2, 2024
Merged

doc: add LLVM instruction for macOS < 13 #29934

merged 1 commit into from
May 2, 2024

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented Apr 22, 2024 •
edited
Loading

#29208 bumped clang to 14, which users of old macOS versions need to install manually. This PR adds instructions.

Xcode 14.3.1 ships clang 14.0.3 (14.0.0 is broken, see #29918):
https://en.wikipedia.org/wiki/Xcode#Xcode_11.0_-_14.x_(since_SwiftUI_framework)

The system requirements for that is macOS Ventura 13.0 or later: https://developer.apple.com/documentation/xcode-release-notes/xcode-14_3_1-release-notes#

Homebrew itself officially supports macOS 12 or later, but may still work on macOS 11: https://docs.brew.sh/Installation

Fwiw macOS 11 Big Sur last got an update in September 2023, so Apple has not entirely written it off: https://en.wikipedia.org/wiki/MacOS_Big_Sur

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 22, 2024 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK maflcko, TheCharlatan
Concept ACK ismaelsadeeq

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Docs label Apr 22, 2024
@Sjors Sjors mentioned this pull request Apr 22, 2024
Merged
@Sjors
Copy link
Member Author

Sjors commented Apr 22, 2024

I tested these instructions by installing llvm 14.0.6 on macOS 14.4.1.

I don't have a macOS 11 or 12 machine, so have not tested those. So there may be other issues, which we'll just have to find out if someone runs into them.

maflcko
maflcko approved these changes Apr 22, 2024
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

lgtm

doc/build-osx.md Outdated Show resolved Hide resolved
@TheCharlatan
Copy link
Contributor

Will test shortly.

@ismaelsadeeq
Copy link
Member

Concept ACK, I ran into this issue sometime ago

@Sjors
Copy link
Member Author

Sjors commented Apr 23, 2024 •
edited
Loading

I spun up a VM with macOS 11 Big Sur and installed the command line tools (I had to download them from Apple, because xcode-select --install got stuck, but that might be a VirtualBox issue).

I then installed Homebrew and did brew install llvm, which should install llvm 18. The openssl@3 install failed due to one broken test (update: worked fine on the second try). openssl@3 is needed by a lot of other things, like brew install ccache (which also needs llvm@17 at the moment).

I'll continue to test, but the VM is excruciatingly slow...

Will update to drop the @14 if that works.

(feel free to mark as draft, I can't find the button)

@fanquake fanquake marked this pull request as draft April 24, 2024 09:33
@Sjors
Copy link
Member Author

Sjors commented Apr 25, 2024 •
edited
Loading

I was able to build bitcoind (didn't try QT) with llvm@17, but not with the current llvm 18.1.4. See log: https://gist.github.com/Sjors/194eb5d67e2e1132df7ff86f103ba16b

It's probably fine to drop @14 anyway, and maybe suggest trying lower versions if building fails. But it would be nice if there's a simple fix for this.

Regarding QT, interestingly Homebrew seems to handle that differently on old macOS versions than on new ones:

qt@5: A full installation of Xcode.app is required to compile
this software. Installing just the Command Line Tools is not sufficient.

I didn't need to install Xcode on macOS 13.6.6. People can either do that or use depends (update: I tested the latter, and ran into a similar error for the default llvm, while it worked fine for llvm@17).

@Sjors Sjors force-pushed the 2024/04/clang-14-osx-old branch from c8cf11f to 13e1c61 Compare April 29, 2024 08:24
@Sjors Sjors marked this pull request as ready for review April 29, 2024 08:24
@Sjors
Copy link
Member Author

Sjors commented Apr 29, 2024

I dropped the @14 from the default instruction, but added a hint to try @17 if that fails. That worked when I tested and I don't expect Homebrew formulas for older versions to change much moving forward, nor llvm 17 itself.

@Sjors Sjors force-pushed the 2024/04/clang-14-osx-old branch from 13e1c61 to b52bf8a Compare April 29, 2024 08:27
@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/24365135335

@Sjors Sjors force-pushed the 2024/04/clang-14-osx-old branch from b52bf8a to 2257404 Compare April 29, 2024 08:33
@maflcko
Copy link
Member

maflcko commented Apr 29, 2024

utACK 2257404

@DrahtBot DrahtBot requested a review from ismaelsadeeq April 29, 2024 08:34
TheCharlatan
TheCharlatan approved these changes Apr 30, 2024
View reviewed changes
Copy link
Contributor

@TheCharlatan TheCharlatan left a comment

Choose a reason for hiding this comment

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

ACK 2257404

@fanquake fanquake merged commit 9d1a286 into bitcoin:master May 2, 2024
16 checks passed
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request May 2, 2024
@Sjors @fanquake
doc: add LLVM instruction for macOS < 13
475aac4 
Github-Pull: bitcoin#29934
Rebased-From: 2257404
@fanquake
Copy link
Member

fanquake commented May 2, 2024

Backported in #29888.

@fanquake fanquake mentioned this pull request May 2, 2024
Merged
fanquake added a commit that referenced this pull request May 13, 2024
@fanquake
Merge #29888: [27.x] Backports
c7885ec 
bd5860b [WIP] doc: release notes for 27.x (fanquake)
475aac4 doc: add LLVM instruction for macOS < 13 (Sjors Provoost)
a995902 depends: Fix build of Qt for 32-bit platforms (laanwj)
0fcceef Fix #29767, set m_synced = true after Commit() (nanlour)
ae9a2ed sign: don't assume we are parsing a sane Miniscript (Antoine Poinsot)
a6a59cf rpc: Reword SighashFromStr error message (MarcoFalke)
364bf01 build: Fix false positive `CHECK_ATOMIC` test for clang-15 (Hennadii Stepanov)
9277793 test: Fix failing univalue float test (MarcoFalke)
5c09791 doc: archive 27.0 release notes (fanquake)
897e5af [rpc, bugfix] Enforce maximum value for setmocktime (dergoegge)
602cfd5 ci: Bump s390x to ubuntu:24.04 (MarcoFalke)
20e6e8d Change Luke Dashjr seed to dashjr-list-of-p2p-nodes.us (Luke Dashjr)
a6862c5 depends: fix mingw-w64 Qt DEBUG=1 build (fanquake)

Pull request description:

  Backports:
  * #29691
  * #29747
  * #29776
  * #29853
  * #29856
  * #29859
  * #29869
  * #29870
  * #29886
  * #29892
  * #29934
  * #29985

ACKs for top commit:
  willcl-ark:
    reACK bd5860b
  stickies-v:
    re-ACK bd5860b
  TheCharlatan:
    ACK bd5860b

Tree-SHA512: a1a40de70cf52b5fc01d9dcc772421751a18c6a48a726c4c05c0371c585a53a27902e17daed9e0d721ab7763c94bb32de05c146bd6bc73fd558edd08b31e8547
@maflcko maflcko mentioned this pull request Aug 14, 2024
Merged
kwvg added a commit to kwvg/dash that referenced this pull request Nov 10, 2024
@kwvg
merge bitcoin#29934: add LLVM instruction for macOS < 13
d97214c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maflcko maflcko maflcko approved these changes

@fanquake fanquake fanquake left review comments

@TheCharlatan TheCharlatan TheCharlatan approved these changes

@ismaelsadeeq ismaelsadeeq Awaiting requested review from ismaelsadeeq

Assignees
No one assigned
Labels
Docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Sjors @DrahtBot @TheCharlatan @ismaelsadeeq @maflcko @fanquake