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

Multiprocess build support #16367

Merged
merged 5 commits into from Apr 10, 2020
Merged

Multiprocess build support #16367

merged 5 commits into from Apr 10, 2020

Conversation

ryanofsky
Copy link
Contributor

@ryanofsky ryanofsky commented Jul 10, 2019

This PR is part of the process separation project.

This splits autotools, depends build, and travis changes out of #10102, so code changes and build system changes can be reviewed separately.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 10, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@Sjors
Copy link
Member

Sjors commented Jul 11, 2019

Adding capnp probably deserves it's own PR, but at least a separate commit.

What is native_boost?

On macOS, I looks like I need to do more than just brew install cmake. See flood of errors.

@ryanofsky
Copy link
Contributor Author

On macOS, I looks like I need to do more than just brew install cmake. See flood of errors.

Thanks for the log! It looks like the error is:

#error "This code requires C++14. Either your compiler does not support it or it is not enabled."

So probably what's happening is that for whatever reason my compiler defaults to c++14, while your compiler defaults to an older c++ version. It looks like I could fix the libmultiprocess build to specify this:

https://cmake.org/cmake/help/latest/command/target_compile_features.html
https://cmake.org/cmake/help/latest/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html#prop_gbl:CMAKE_CXX_KNOWN_FEATURES

An alternate approach might be to downgrade capnp from 0.7.0, since 0.7.0 is the first version that requires c++14, and I'm actually not relying on any 0.7.0 features.

I do need to try building and running this code again on mac soon, since I haven't tried it in a while.

@Sjors
Copy link
Member

Sjors commented Jul 11, 2019

See also #13356 for C++14 discussion. However, as long as multiprocess is optional, I think it's more future proof to use the C++14, with the latest capnp version, and fix the macOS build.

@ryanofsky ryanofsky force-pushed the pr/ipc-build branch 2 times, most recently from 5516a43 to f477167 Compare July 15, 2019 18:41
Copy link
Member

@ariard ariard left a comment

Choose a reason for hiding this comment

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

OK, I succeed to build bitcoin-gui and bitcoin-node and bitcoin-wallet on Debian 9.

Main problem I had to build first libmultiprocess as a dependency and then pass its library directory with --with-libmultiprocess to ./configure which isn't documented in the multiprocess.md. Then to run bitcoin-node and bitcoin-wallet I had to add libcapnp-rpc-0.7.0.so in runtime search path. But it succeed and both are working successfully. Don't try to run bitcoin-gui as I was lacking more dependencies.

IMO you should update doc/build-unix.md with a multiprocess section which tell you exactly what to do step by step and requirements. And we should blindly follow these sections on Debian/Fedora/Gentoo and see if it's smooth enough for a user.

But overall, super excited to go forward with multiprocess !

src/Makefile.am Outdated Show resolved Hide resolved
depends/README.md Show resolved Hide resolved
Copy link
Contributor Author

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Thanks for testing, ariard! It sounds like you encountered bugs that you worked around, but I'd like to know more specifics about what wasn't working. I'm happy to help debug anytime, or if you want to work online, you can post issues to https://github.com/chaincodelabs/libmultiprocess/issues/new (general issues, bitcoin-specific issues, and support requests all welcome there). Regarding the specific workarounds:

  • Passing a prefix directory to --with-libmultiprocess (or specifying it at all) shouldn't be necessary, and I actually removed the option to hardcode a prefix in some new changes I haven't pushed yet. The configure script should be able to locate dependencies through pkgconf. If that isn't happening please let me know the specifics or open an issue!

  • Regarding documentation, configure --help text should serve as a reference for config options, and the --enable-multiprocess error messages should provide clear guidance if anything goes wrong. If the help text isn't helpful, I'd want to open an issue to track that. multiprocess.md and libmultiprocess README.md could definitely be improved, but for install steps I would mostly want to add to the existing install steps rather than add separate instructions. So far I've only really looked at the depends instructions, not the unix/mac/windows instructions, and I need to get to those at some point.

  • The problem with library paths sounds like a bug. Rpaths should be set correctly so you shouldn't need to anything special at runtime.

depends/README.md Show resolved Hide resolved
@ariard
Copy link
Member

ariard commented Jul 26, 2019

For others reviewers, after talking with Russ, my mistake was not to use depends prefix as described in depends/README.md. Will give it another try after updates.

ryanofsky added a commit to ryanofsky/libmultiprocess that referenced this pull request Aug 6, 2019
This simplifies multiprocess build code somewhat
(bitcoin/bitcoin#16367) so it only has to invoke one
code generator instead of two, and is probably a little more future proof in
that is should make it possible to change capnp options without any requiring
changes outside libmultiprocess.
ryanofsky added a commit to chaincodelabs/libmultiprocess that referenced this pull request Aug 6, 2019
f89e4b3 Invoke capnp compile from mpgen (Russell Yanofsky)

Pull request description:

  This simplifies multiprocess build code somewhat (bitcoin/bitcoin#16367) so it only has to invoke one code generator instead of two, and is probably a little more future proof in that is should make it possible to change capnp options without any requiring changes outside libmultiprocess.

Top commit has no ACKs.

Tree-SHA512: 64534ec8d020eaf6a2dd351e4e48bf3a7a9897c126db1585c259950070b513aa9a416204e64ad1ab16717f41c534b9aafced4844039ed1f9037550946533445e
@ryanofsky ryanofsky force-pushed the pr/ipc-build branch 3 times, most recently from d8ad7a2 to 3b48de7 Compare August 6, 2019 21:48
@ryanofsky
Copy link
Contributor Author

re: http://gnusha.org/bitcoin-builds/2019-08-16.log

<dongcarl> cfields: I think the boostrap part of boost is actually "native", as in, should be moved to a boost_native package like the native part of protobuf was... Does that ring somewhat true to you?

@dongcarl this PR adds a native_boost package, but there should be no need for a native package just to use the bootstrap tool, unless you need to use the bootstrap tool from other packages, and not just use it internally in the boost package.

@fanquake
Copy link
Member

fanquake commented Aug 23, 2019

I depends built, compiled and played around with this (macOS). Going to re-read all the discussion in #10102.

I'm also going to extract 7149b1a and a295b9d into a separate PR. They are clear improvements, and will likely clear up some confusion here and in other depends related PRs.

@fanquake
Copy link
Member

fanquake commented Apr 12, 2020

Where is all this discussion taking place?

@jnewbery It's happening in the #bitcoin-builds channel. Logs available here: http://gnusha.org/bitcoin-builds/.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 13, 2020
b919efa depends: Use default macos clang compiler (Russell Yanofsky)
d54f64c Add multiprocess travis configuration (Russell Yanofsky)
787f406 Set LD_LIBRARY_PATH consistently in travis tests (Russell Yanofsky)
d630646 libmultiprocess depends build (Russell Yanofsky)
e6e44ee Multiprocess build changes (Russell Yanofsky)

Pull request description:

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).

  This splits autotools, depends build, and travis changes out of bitcoin#10102, so code changes and build system changes can be reviewed separately.

ACKs for top commit:
  hebasto:
    re-ACK b919efa, rebased only since my [previous](bitcoin#16367 (comment)) review.

Tree-SHA512: ebc5e403cc99a0d9629ed7fe1595e01d57e6d1255cbf03968a3196ff6f528f734c78060fdc065724ee1f923bcc5aa2b29470fcb36a7f15957eb57c76d58178a4
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 13, 2020
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 15, 2020
Suggested by Cory Fields <cory-nospam-@coryfields.com>
bitcoin#16367 (comment)
as alternate workaround for problem described
bitcoin#16367 (comment)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 15, 2020
depends: Use default macos clang compiler

Suggested by Cory Fields <cory-nospam-@coryfields.com>
bitcoin#16367 (comment)
as alternate workaround for problem described
bitcoin#16367 (comment)
@ryanofsky ryanofsky moved this from Done to In progress in Process Separation Apr 16, 2020
@ryanofsky
Copy link
Contributor Author

This PR, which was reverted in #18588, is a proposed meeting topic. Background in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Process-Separation

Topic could be broad, but two questions I'd like to get input on are:

  1. What should steps and requirements be for moving chaincodelabs/libmultiprocess to bitcoin-core/, assuming this would be a better home for the library.

  2. What should review requirements be for this PR, which adds --enable-multiprocess autoconf option (disabled by default and labeled experimental), and a travis build, and a depends definition to build libmultiprocess (also disabled by default and labeled experimental)?

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 16, 2020
Suggested by Cory Fields <cory-nospam-@coryfields.com>
bitcoin#16367 (comment)
as alternate workaround for problem described
bitcoin#16367 (comment)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 20, 2020
Suggested by Cory Fields <cory-nospam-@coryfields.com>
bitcoin#16367 (comment)
as alternate workaround for problem described
bitcoin#16367 (comment)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 22, 2020
Catalina SDK clang stopped automatically searching the SDK include paths when
invoked without --sysroot:

bitcoin#16367 (comment)
Homebrew/homebrew-core#45061

This hasn't been a problem for current native depends packages because are
passing their own --sysroot values, and hasn't been a problem for current host
packages because they use `darwin_` commands instead of `build_darwin_`
commands.  But the current `build_darwin_CC` and `build_darwin_CXX` commands
are still unnecessarily fragile, and incompatible with new native depends
packages added in bitcoin#18677.

Cory Fields <cory-nospam-@coryfields.com> suggested in
bitcoin#16367 (comment) switching
compiler from SDK clang to native clang (from $PATH) to avoid this problem.
This is easy and makes a certain amount of sense for building native packages,
as opposed to host packages. But fanquake <fanquake@gmail.com> pointed out in
bitcoin#18677 (comment) that it
would be inconsistent use switch to non-SDK compilers while still using other
SDK tools like ranlib and install_name_tool. So simplest, minimal fix seems to
be just adding the missing --sysroot option.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 22, 2020
Suggested by Cory Fields <cory-nospam-@coryfields.com>
bitcoin#16367 (comment)
as alternate workaround for problem described
bitcoin#16367 (comment)
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request May 1, 2020
depends: Add --sysroot option to mac os native compile flags

Catalina SDK clang stopped automatically searching the SDK include paths when
invoked without --sysroot:

bitcoin#16367 (comment)
Homebrew/homebrew-core#45061

This hasn't been a problem for current native depends packages because are
passing their own --sysroot values, and hasn't been a problem for current host
packages because they use `darwin_` commands instead of `build_darwin_`
commands.  But the current `build_darwin_CC` and `build_darwin_CXX` commands
are still unnecessarily fragile, and incompatible with new native depends
packages added in bitcoin#18677.

Cory Fields <cory-nospam-@coryfields.com> suggested in
bitcoin#16367 (comment) switching
compiler from SDK clang to native clang (from $PATH) to avoid this problem.
This is easy and makes a certain amount of sense for building native packages,
as opposed to host packages. But fanquake <fanquake@gmail.com> pointed out in
bitcoin#18677 (comment) that it
would be inconsistent use switch to non-SDK compilers while still using other
SDK tools like ranlib and install_name_tool. So simplest, minimal fix seems to
be just adding the missing --sysroot option.
fanquake added a commit that referenced this pull request May 7, 2020
…flags

1e94a2b depends: Add --sysroot option to mac os native compile flags (Russell Yanofsky)

Pull request description:

  Catalina SDK clang stopped automatically searching the SDK include paths when invoked without `--sysroot`:

  - #16367 (comment)
  - Homebrew/homebrew-core#45061

  This hasn't been a problem for current native depends packages because are passing their own `--sysroot` values, and hasn't been a problem for current host packages because they use `darwin_` commands instead of `build_darwin_` commands.  But the current `build_darwin_CC` and `build_darwin_CXX` commands are still unnecessarily fragile, and incompatible with new native depends packages added in #18677.

  Cory Fields (theuni) suggested in #16367 (comment) switching compiler from SDK clang to native clang (from $PATH) to avoid this problem. This is easy and makes a certain amount of sense for building native packages, as opposed to host packages. But Michael (fanquake) pointed out in #18677 (comment) that it would be inconsistent to switch to non-SDK compilers while still using other SDK tools like `ranlib` and `install_name_tool`. So simplest, minimal fix seems to be just adding the missing `--sysroot` option.

ACKs for top commit:
  ryanofsky:
    > ACK [1e94a2b](1e94a2b) - I think this change is ok, and I prefer it to the previous patch.
  fanquake:
    ACK 1e94a2b - I think this change is ok, and I prefer it to the previous patch. Thanks for the summary in the PR description. I played around with Xcode and the CLT; I think previously I didn't fully grok the slight differences between the two.

Tree-SHA512: 4d4bbb7f49acb76d934a872a15b4e14f36290b508cb9e728815f959767ec174bcfb6d2ca7dcd995cc550d86980d64d4247ea5ecfca2301f0953006e50744fdb4
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 12, 2020
…ompile flags

1e94a2b depends: Add --sysroot option to mac os native compile flags (Russell Yanofsky)

Pull request description:

  Catalina SDK clang stopped automatically searching the SDK include paths when invoked without `--sysroot`:

  - bitcoin#16367 (comment)
  - Homebrew/homebrew-core#45061

  This hasn't been a problem for current native depends packages because are passing their own `--sysroot` values, and hasn't been a problem for current host packages because they use `darwin_` commands instead of `build_darwin_` commands.  But the current `build_darwin_CC` and `build_darwin_CXX` commands are still unnecessarily fragile, and incompatible with new native depends packages added in bitcoin#18677.

  Cory Fields (theuni) suggested in bitcoin#16367 (comment) switching compiler from SDK clang to native clang (from $PATH) to avoid this problem. This is easy and makes a certain amount of sense for building native packages, as opposed to host packages. But Michael (fanquake) pointed out in bitcoin#18677 (comment) that it would be inconsistent to switch to non-SDK compilers while still using other SDK tools like `ranlib` and `install_name_tool`. So simplest, minimal fix seems to be just adding the missing `--sysroot` option.

ACKs for top commit:
  ryanofsky:
    > ACK [1e94a2b](bitcoin@1e94a2b) - I think this change is ok, and I prefer it to the previous patch.
  fanquake:
    ACK 1e94a2b - I think this change is ok, and I prefer it to the previous patch. Thanks for the summary in the PR description. I played around with Xcode and the CLT; I think previously I didn't fully grok the slight differences between the two.

Tree-SHA512: 4d4bbb7f49acb76d934a872a15b4e14f36290b508cb9e728815f959767ec174bcfb6d2ca7dcd995cc550d86980d64d4247ea5ecfca2301f0953006e50744fdb4
fanquake added a commit that referenced this pull request May 21, 2020
e2bab2a multiprocess: add multiprocess travis configuration (Russell Yanofsky)
603fd6a depends: add MULTIPROCESS depends option (Russell Yanofsky)
5d1377b build: multiprocess autotools changes (Russell Yanofsky)

Pull request description:

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).

  ---

  This PR consists of build changes only. It adds an `--enable-multiprocess` autoconf option (off by default and marked experimental), that builds new `bitcoin-node` and `bitcoin-gui` binaries. These currently function the same as existing `bitcoind` and `bitcoin-qt` binaries, but are extended in #10102 with IPC features to execute node, wallet, and gui functions in separate processes.

  In addition to adding the `--enable-multiprocess` config flag, it also adds a depends package and autoconf rules to build with the [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) library, and it adds new travis configuration to exercise the build code and run functional tests with the new binaries.

  The changes in this PR were originally part of #10102 but were moved into #16367 to be able to develop and review the multiprocess build changes independently of the code changes. #16367 was briefly merged and then reverted in #18588. Only change since #16367 has been dropping the `native_boost.mk` depends package which was pointed out to be no longer necessary in #16367 (comment) and #18588 (review)

ACKs for top commit:
  practicalswift:
    ACK e2bab2a
  Sjors:
    tACK e2bab2a on macOS 10.15.4
  hebasto:
    ACK e2bab2a, tested on Linux Mint 19.3 (x86_64):

Tree-SHA512: b5a76eab5abf63d9d8b6d628cbdff4cc1888eef15cafa0a5d56369e2f9d02595fed623f4b74b2cf2830c42c05a774f0943e700f9c768a82d9d348cad199e135c
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 21, 2020
e2bab2a multiprocess: add multiprocess travis configuration (Russell Yanofsky)
603fd6a depends: add MULTIPROCESS depends option (Russell Yanofsky)
5d1377b build: multiprocess autotools changes (Russell Yanofsky)

Pull request description:

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).

  ---

  This PR consists of build changes only. It adds an `--enable-multiprocess` autoconf option (off by default and marked experimental), that builds new `bitcoin-node` and `bitcoin-gui` binaries. These currently function the same as existing `bitcoind` and `bitcoin-qt` binaries, but are extended in bitcoin#10102 with IPC features to execute node, wallet, and gui functions in separate processes.

  In addition to adding the `--enable-multiprocess` config flag, it also adds a depends package and autoconf rules to build with the [libmultiprocess](https://github.com/chaincodelabs/libmultiprocess) library, and it adds new travis configuration to exercise the build code and run functional tests with the new binaries.

  The changes in this PR were originally part of bitcoin#10102 but were moved into bitcoin#16367 to be able to develop and review the multiprocess build changes independently of the code changes. bitcoin#16367 was briefly merged and then reverted in bitcoin#18588. Only change since bitcoin#16367 has been dropping the `native_boost.mk` depends package which was pointed out to be no longer necessary in bitcoin#16367 (comment) and bitcoin#18588 (review)

ACKs for top commit:
  practicalswift:
    ACK e2bab2a
  Sjors:
    tACK e2bab2a on macOS 10.15.4
  hebasto:
    ACK e2bab2a, tested on Linux Mint 19.3 (x86_64):

Tree-SHA512: b5a76eab5abf63d9d8b6d628cbdff4cc1888eef15cafa0a5d56369e2f9d02595fed623f4b74b2cf2830c42c05a774f0943e700f9c768a82d9d348cad199e135c
@ryanofsky ryanofsky moved this from In progress to Done in Process Separation Jun 1, 2020
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 23, 2020
Summary:
```
Catalina SDK clang stopped automatically searching the SDK include paths
when invoked without --sysroot:

bitcoin/bitcoin#16367 (comment)
Homebrew/homebrew-core#45061

This hasn't been a problem for current native depends packages because
are passing their own --sysroot values, and hasn't been a problem for
current host packages because they use `darwin_` commands instead of
`build_darwin_` commands.  But the current `build_darwin_CC` and
`build_darwin_CXX` commands are still unnecessarily fragile, and
incompatible with new native depends packages added in
bitcoin/bitcoin#18677.

Cory Fields <cory-nospam-@coryfields.com> suggested in
bitcoin/bitcoin#16367 (comment)
switching compiler from SDK clang to native clang (from $PATH) to avoid
this problem. This is easy and makes a certain amount of sense for
building native packages, as opposed to host packages. But fanquake
<fanquake@gmail.com> pointed out in
bitcoin/bitcoin#18677 (comment) that
it would be inconsistent use switch to non-SDK compilers while still
using other SDK tools like ranlib and install_name_tool. So simplest,
minimal fix seems to be just adding the missing --sysroot option.
```

Backport of core [[bitcoin/bitcoin#18743 | PR18743]].
Note that it doesn't fix any real life problem for now.

Test Plan: Run the OSX Gitian build.

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7522
janus pushed a commit to janus/bitgesell that referenced this pull request Nov 5, 2020
Suggested by Cory Fields <cory-nospam-@coryfields.com>
bitcoin/bitcoin#16367 (comment)
as alternate workaround for problem described
bitcoin/bitcoin#16367 (comment)
janus pushed a commit to janus/bitgesell that referenced this pull request Nov 8, 2020
Catalina SDK clang stopped automatically searching the SDK include paths when
invoked without --sysroot:

bitcoin/bitcoin#16367 (comment)
Homebrew/homebrew-core#45061

This hasn't been a problem for current native depends packages because are
passing their own --sysroot values, and hasn't been a problem for current host
packages because they use `darwin_` commands instead of `build_darwin_`
commands.  But the current `build_darwin_CC` and `build_darwin_CXX` commands
are still unnecessarily fragile, and incompatible with new native depends
packages added in bitcoin/bitcoin#18677.

Cory Fields <cory-nospam-@coryfields.com> suggested in
bitcoin/bitcoin#16367 (comment) switching
compiler from SDK clang to native clang (from $PATH) to avoid this problem.
This is easy and makes a certain amount of sense for building native packages,
as opposed to host packages. But fanquake <fanquake@gmail.com> pointed out in
bitcoin/bitcoin#18677 (comment) that it
would be inconsistent use switch to non-SDK compilers while still using other
SDK tools like ranlib and install_name_tool. So simplest, minimal fix seems to
be just adding the missing --sysroot option.
maaku pushed a commit to tradecraftio/tradecraft that referenced this pull request Jan 23, 2021
Catalina SDK clang stopped automatically searching the SDK include paths when
invoked without --sysroot:

bitcoin/bitcoin#16367 (comment)
Homebrew/homebrew-core#45061

This hasn't been a problem for current native depends packages because are
passing their own --sysroot values, and hasn't been a problem for current host
packages because they use `darwin_` commands instead of `build_darwin_`
commands.  But the current `build_darwin_CC` and `build_darwin_CXX` commands
are still unnecessarily fragile, and incompatible with new native depends
packages added in bitcoin/bitcoin#18677.

Cory Fields <cory-nospam-@coryfields.com> suggested in
bitcoin/bitcoin#16367 (comment) switching
compiler from SDK clang to native clang (from $PATH) to avoid this problem.
This is easy and makes a certain amount of sense for building native packages,
as opposed to host packages. But fanquake <fanquake@gmail.com> pointed out in
bitcoin/bitcoin#18677 (comment) that it
would be inconsistent use switch to non-SDK compilers while still using other
SDK tools like ranlib and install_name_tool. So simplest, minimal fix seems to
be just adding the missing --sysroot option.
maaku pushed a commit to tradecraftio/tradecraft that referenced this pull request Jan 23, 2021
Catalina SDK clang stopped automatically searching the SDK include paths when
invoked without --sysroot:

bitcoin/bitcoin#16367 (comment)
Homebrew/homebrew-core#45061

This hasn't been a problem for current native depends packages because are
passing their own --sysroot values, and hasn't been a problem for current host
packages because they use `darwin_` commands instead of `build_darwin_`
commands.  But the current `build_darwin_CC` and `build_darwin_CXX` commands
are still unnecessarily fragile, and incompatible with new native depends
packages added in bitcoin/bitcoin#18677.

Cory Fields <cory-nospam-@coryfields.com> suggested in
bitcoin/bitcoin#16367 (comment) switching
compiler from SDK clang to native clang (from $PATH) to avoid this problem.
This is easy and makes a certain amount of sense for building native packages,
as opposed to host packages. But fanquake <fanquake@gmail.com> pointed out in
bitcoin/bitcoin#18677 (comment) that it
would be inconsistent use switch to non-SDK compilers while still using other
SDK tools like ranlib and install_name_tool. So simplest, minimal fix seems to
be just adding the missing --sysroot option.
furszy pushed a commit to furszy/bitcoin-core that referenced this pull request Jun 30, 2021
Catalina SDK clang stopped automatically searching the SDK include paths when
invoked without --sysroot:

bitcoin#16367 (comment)
Homebrew/homebrew-core#45061

This hasn't been a problem for current native depends packages because are
passing their own --sysroot values, and hasn't been a problem for current host
packages because they use `darwin_` commands instead of `build_darwin_`
commands.  But the current `build_darwin_CC` and `build_darwin_CXX` commands
are still unnecessarily fragile, and incompatible with new native depends
packages added in bitcoin#18677.

Cory Fields <cory-nospam-@coryfields.com> suggested in
bitcoin#16367 (comment) switching
compiler from SDK clang to native clang (from $PATH) to avoid this problem.
This is easy and makes a certain amount of sense for building native packages,
as opposed to host packages. But fanquake <fanquake@gmail.com> pointed out in
bitcoin#18677 (comment) that it
would be inconsistent use switch to non-SDK compilers while still using other
SDK tools like ranlib and install_name_tool. So simplest, minimal fix seems to
be just adding the missing --sysroot option.
UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Jul 19, 2021
…ompile flags

1e94a2b depends: Add --sysroot option to mac os native compile flags (Russell Yanofsky)

Pull request description:

  Catalina SDK clang stopped automatically searching the SDK include paths when invoked without `--sysroot`:

  - bitcoin#16367 (comment)
  - Homebrew/homebrew-core#45061

  This hasn't been a problem for current native depends packages because are passing their own `--sysroot` values, and hasn't been a problem for current host packages because they use `darwin_` commands instead of `build_darwin_` commands.  But the current `build_darwin_CC` and `build_darwin_CXX` commands are still unnecessarily fragile, and incompatible with new native depends packages added in bitcoin#18677.

  Cory Fields (theuni) suggested in bitcoin#16367 (comment) switching compiler from SDK clang to native clang (from $PATH) to avoid this problem. This is easy and makes a certain amount of sense for building native packages, as opposed to host packages. But Michael (fanquake) pointed out in bitcoin#18677 (comment) that it would be inconsistent to switch to non-SDK compilers while still using other SDK tools like `ranlib` and `install_name_tool`. So simplest, minimal fix seems to be just adding the missing `--sysroot` option.

ACKs for top commit:
  ryanofsky:
    > ACK [1e94a2b](bitcoin@1e94a2b) - I think this change is ok, and I prefer it to the previous patch.
  fanquake:
    ACK 1e94a2b - I think this change is ok, and I prefer it to the previous patch. Thanks for the summary in the PR description. I played around with Xcode and the CLT; I think previously I didn't fully grok the slight differences between the two.

Tree-SHA512: 4d4bbb7f49acb76d934a872a15b4e14f36290b508cb9e728815f959767ec174bcfb6d2ca7dcd995cc550d86980d64d4247ea5ecfca2301f0953006e50744fdb4
PhotoshiNakamoto added a commit to PhotonicBitcoin/pBTC-core that referenced this pull request Dec 11, 2021
Bitcoin Core PR:bitcoin/bitcoin#16367

Pull request description:

  This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).

  This splits autotools, depends build, and travis changes out of bitcoin/bitcoin#10102, so code changes and build system changes can be reviewed separately.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

10 participants