-
Notifications
You must be signed in to change notification settings - Fork 36.2k
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 #18677
Multiprocess build support #18677
Conversation
re-ACK c8672df, only change since my last review is dropping boost and small doc fixup. This review does not cover libmultiprocess 🚕 Show signature and timestampSignature:
Timestamp of file with hash |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reopening and the responses here. I also read through the IRC logs from last night.
Few comments other than on the code. Nothing blocking while this is all still experimental.
Cross-compiling for macOS (on Debian 10.3) with MULTIPROCESS=1
currently fails while building capnp
:
make -C depends/ HOST=x86_64-apple-darwin16 MULTIPROCESS=1
...
libtool: link: clang++ -target x86_64-apple-darwin16 -mmacosx-version-min=10.12 --sysroot /bitcoin/depends/SDKs/MacOSX10.14.sdk -stdlib=libc++ -I./src -I./src -DKJ_HEADER_WARNINGS -DCAPNP_HEADER_WARNINGS -DCAPNP_INCLUDE_DIR=\"/bitcoin/depends/x86_64-apple-darwin16/include\" -D_THREAD_SAFE -pipe -O2 -D_THREAD_SAFE -D_THREAD_SAFE -o .libs/capnp src/capnp/compiler/module-loader.o src/capnp/compiler/capnp.o -Wl,-bind_at_load -L/bitcoin/depends/x86_64-apple-darwin16/lib ./.libs/libcapnpc.dylib ./.libs/libcapnp-json.dylib /bitcoin/depends/work/build/x86_64-apple-darwin16/capnp/0.7.0-87ee694a224/.libs/libcapnp.dylib ./.libs/libcapnp.dylib /bitcoin/depends/work/build/x86_64-apple-darwin16/capnp/0.7.0-87ee694a224/.libs/libkj.dylib ./.libs/libkj.dylib
echo capnp capnpc-c++ src/capnp/test.capnp src/capnp/test-import.capnp src/capnp/test-import2.capnp src/capnp/compat/json-test.capnp | (read CAPNP CAPNPC_CXX SOURCES && ./$CAPNP compile --src-prefix=./src -o./$CAPNPC_CXX:src -I./src $SOURCES)
./capnp: line 117: /bitcoin/depends/work/build/x86_64-apple-darwin16/capnp/0.7.0-87ee694a224/.libs/capnp: cannot execute binary file: Exec format error
./capnp: line 117: /bitcoin/depends/work/build/x86_64-apple-darwin16/capnp/0.7.0-87ee694a224/.libs/capnp: Success
make[1]: *** [Makefile:4326: test_capnpc_middleman] Error 126
make[1]: Leaving directory '/bitcoin/depends/work/build/x86_64-apple-darwin16/capnp/0.7.0-87ee694a224'
make: *** [funcs.mk:254: /bitcoin/depends/work/build/x86_64-apple-darwin16/capnp/0.7.0-87ee694a224/./.stamp_built] Error 2
make: Leaving directory '/bitcoin/depends'
Would you prefer these issues opened here or the libmultiprocess repo?
Can we pass --with-openssl=no
(+ any other relevant flags) to the capnp configure? I assume we aren't using any parts of it that require OpenSSL, and we might as well opt out of any autodetection and/or additional compilation.
Also, I didn't notice before, but just wanted to note that building capnp currently requires gnu extensions. This is at odds with our current compiler requirements.
@@ -1,7 +1,7 @@ | |||
package=boost | |||
$(package)_version=1_70_0 | |||
$(package)_download_path=https://dl.bintray.com/boostorg/release/1.70.0/source/ | |||
$(package)_file_name=$(package)_$($(package)_version).tar.bz2 | |||
$(package)_file_name=boost_$($(package)_version).tar.bz2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems leftover from the boost changes. I'd suggest dropping as it would likely just be changed back. i.e: #17928
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #18677 (comment)
This seems leftover from the boost changes. I'd suggest dropping as it would likely just be changed back. i.e: #17928
Substituting
depends/builders/darwin.mk
Outdated
build_darwin_CC:=$(shell xcrun -f clang) | ||
build_darwin_CXX:=$(shell xcrun -f clang++) | ||
build_darwin_CC:=clang | ||
build_darwin_CXX:=clang++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify why this is needed? I did look at the commit comments about passing --sysroot
and missing includes, however from what I can tell, there should be no change in behaviour with this diff. I did run a few compilation tests and didn't see any obvious difference in compile flags/includes etc.
My assumption is that the problems seen (chaincodelabs/libmultiprocess#5, #16367 (comment)) might have been due to an issue with the development environments i.e missing SDK headers, not installing the CommandLineTools etc.
Mostly curious because if this was an issue it seems like it shouldn't be multiprocess specific? It also seems strange to drop the xcrun
call only for clang/clang++, and not for the rest of the tools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #18677 (comment)
Can you clarify why this is needed? I did look at the commit comments about passing
--sysroot
and missing includes, however from what I can tell, there should be no change in behaviour with this diff. I did run a few compilation tests and didn't see any obvious difference in compile flags/includes etc.
The comment the PR description links to is #16367 (comment). If you follow the instructions there creating test.cpp
echo '#include <stdlib.h>' > test.cpp
and running /Library/Developer/CommandLineTools/usr/bin/clang++ -c test.cpp
, then you can see if the problem affects your system.
It has something to do with the SDK version:
- build fails with OSX Mojave neovim/neovim#9050 (comment)
- https://discourse.brew.sh/t/clang-can-no-longer-find-usr-include-header-files-fatal-error-stdlib-h-file-not-found/4523
- https://stackoverflow.com/questions/52509602/cant-compile-c-program-on-a-mac-after-upgrade-to-mojave/52530212#52530212
My original fix from those comments was ryanofsky@d6921b1, but Cory's suggestion ryanofsky@04bec8d seemed better: just use native compiler for native packages, not the sdk compiler.
My assumption is that the problems seen (chaincodelabs/libmultiprocess#5, #16367 (comment)) might have been due to an issue with the development environments i.e missing SDK headers, not installing the CommandLineTools etc.
Not unless running the shell xcrun -f clang
compiler without a sysroot option is supposed to work.
Mostly curious because if this was an issue it seems like it shouldn't be multiprocess specific?
It's not, just a bad interaction with native packages that don't specify --sysroot, which we didn't have previously.
It also seems strange to drop the
xcrun
call only for clang/clang++, and not for the rest of the tools.
That's a good point. It didn't occur to me to test changing all the other tools paths when I adopted Cory's fix, and at the moment I don't have a convenient way to retest. My original fix ryanofsky@d6921b1 doesn't have this inconsistency, and I'd be happy to switch back to it if it's preferable.
It'd be nice to have automated test coverage for this. It seems curious that the one osx job we have on travis does not test depends, IIUC?
src/Makefile.am
Outdated
bitcoind_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) | ||
bitcoind_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) | ||
# bitcoind & bitcoin-node binaries # | ||
bitcoin_common_sources = bitcoind.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to be bikeshedding this again, but what about naming this node_sources
or daemon_sources
? As of now it sounds confusingly similar to libbitcoin_common, which is an utility library for all Bitcoin Core util binaries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #18677 (comment)
Sorry to be bikeshedding this again, but what about naming this
node_sources
ordaemon_sources
? As of now it sounds confusingly similar to libbitcoin_common, which is an utility library for all Bitcoin Core util binaries
Switched to daemon_sources as suggested. Possible this is just bikeshedding with past-Marco #16367 (comment), but I think all the suggestions have been good, and the confusion with libbitcoin_common didn't occur to me before this
c8672df
to
f80a689
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated c8672df -> f80a689 (pr/ipc-build2.1
-> pr/ipc-build2.2
, compare) with x86_64-apple-darwin16 fixes and suggested automake renames
re: #18677 (review)
Cross-compiling for macOS (on Debian 10.3) with
MULTIPROCESS=1
currently fails while buildingcapnp
:make -C depends/ HOST=x86_64-apple-darwin16 MULTIPROCESS=1 ...
Good catch, I hadn't tested this configuration. I fixed this and more problems with it in the latest push.
Would you prefer these issues opened here or the libmultiprocess repo?
This was a depends system issue so it made sense here, but in general it can be hard to pin down where issues arise so either place should be fine
Can we pass
--with-openssl=no
(+ any other relevant flags) to the capnp configure? I assume we aren't using any parts of it that require OpenSSL, and we might as well opt out of any autodetection and/or additional compilation.
I think you'd have to write an openssl depends package and add it to $(package)_dependencies
in capnp.mk for there to be anything to detect. But maybe requesting to be built without openssl when openssl isn't available would be a good way to demonstrate not liking openssl
Also, I didn't notice before, but just wanted to note that building capnp currently requires gnu extensions. This is at odds with our current compiler requirements.
I don't think gnu extensions are required, and I don't see any problem building without them (following regular build steps but passing CXXFLAGS=-std=c++14 to configure). Digging into the history of this check it looks like it was added to work around a cygwin bug where snprintf wasn't declared with -std=c++0x, but was with -std=gnu++0x
https://groups.google.com/d/msg/capnproto/l46nHozrhCY/vJTHMkJC-ZEJ
https://groups.google.com/d/msg/capnproto/l46nHozrhCY/UG__SqiVbywJ
https://stackoverflow.com/questions/20149633/how-to-use-snprintf-in-g-std-c11-version-4-8-2
depends/builders/darwin.mk
Outdated
build_darwin_CC:=$(shell xcrun -f clang) | ||
build_darwin_CXX:=$(shell xcrun -f clang++) | ||
build_darwin_CC:=clang | ||
build_darwin_CXX:=clang++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #18677 (comment)
Can you clarify why this is needed? I did look at the commit comments about passing
--sysroot
and missing includes, however from what I can tell, there should be no change in behaviour with this diff. I did run a few compilation tests and didn't see any obvious difference in compile flags/includes etc.
The comment the PR description links to is #16367 (comment). If you follow the instructions there creating test.cpp
echo '#include <stdlib.h>' > test.cpp
and running /Library/Developer/CommandLineTools/usr/bin/clang++ -c test.cpp
, then you can see if the problem affects your system.
It has something to do with the SDK version:
- build fails with OSX Mojave neovim/neovim#9050 (comment)
- https://discourse.brew.sh/t/clang-can-no-longer-find-usr-include-header-files-fatal-error-stdlib-h-file-not-found/4523
- https://stackoverflow.com/questions/52509602/cant-compile-c-program-on-a-mac-after-upgrade-to-mojave/52530212#52530212
My original fix from those comments was ryanofsky@d6921b1, but Cory's suggestion ryanofsky@04bec8d seemed better: just use native compiler for native packages, not the sdk compiler.
My assumption is that the problems seen (chaincodelabs/libmultiprocess#5, #16367 (comment)) might have been due to an issue with the development environments i.e missing SDK headers, not installing the CommandLineTools etc.
Not unless running the shell xcrun -f clang
compiler without a sysroot option is supposed to work.
Mostly curious because if this was an issue it seems like it shouldn't be multiprocess specific?
It's not, just a bad interaction with native packages that don't specify --sysroot, which we didn't have previously.
It also seems strange to drop the
xcrun
call only for clang/clang++, and not for the rest of the tools.
That's a good point. It didn't occur to me to test changing all the other tools paths when I adopted Cory's fix, and at the moment I don't have a convenient way to retest. My original fix ryanofsky@d6921b1 doesn't have this inconsistency, and I'd be happy to switch back to it if it's preferable.
It'd be nice to have automated test coverage for this. It seems curious that the one osx job we have on travis does not test depends, IIUC?
@@ -1,7 +1,7 @@ | |||
package=boost | |||
$(package)_version=1_70_0 | |||
$(package)_download_path=https://dl.bintray.com/boostorg/release/1.70.0/source/ | |||
$(package)_file_name=$(package)_$($(package)_version).tar.bz2 | |||
$(package)_file_name=boost_$($(package)_version).tar.bz2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #18677 (comment)
This seems leftover from the boost changes. I'd suggest dropping as it would likely just be changed back. i.e: #17928
Substituting
src/Makefile.am
Outdated
bitcoind_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS) | ||
bitcoind_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) | ||
# bitcoind & bitcoin-node binaries # | ||
bitcoin_common_sources = bitcoind.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #18677 (comment)
Sorry to be bikeshedding this again, but what about naming this
node_sources
ordaemon_sources
? As of now it sounds confusingly similar to libbitcoin_common, which is an utility library for all Bitcoin Core util binaries
Switched to daemon_sources as suggested. Possible this is just bikeshedding with past-Marco #16367 (comment), but I think all the suggestions have been good, and the confusion with libbitcoin_common didn't occur to me before this
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.
f80a689
to
fba09de
Compare
fba09de
to
474f4f1
Compare
I thought we were going to pivot this to just add a |
Updated f80a689 -> fba09de ( re: #18677 (comment)
I think the current naming is better, but I pointed out in the IRC meeting it is easy to change. If it's important, would suggest opening a followup PR and considering this as usability change to discuss ideally after having some experience using the feature |
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.
7c99f2c: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated 474f4f1 -> abf920e (pr/ipc-build2.4
-> pr/ipc-build2.5
, compare) with suggestions
Thanks for review!
re: #18677 (comment)
7c99f2c:
Maybe add fallback to default value for theTEST_RUNNER_ENV
variable in the00_setup_env.sh
script for consistency?
Added
e488518
to
e2bab2a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK e2bab2a, tested on Linux Mint 19.3 (x86_64):
$ make -C depends NO_QT=1 MULTIPROCESS=1
$ ./autogen.sh
$ CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site ./configure
$ make check
$ BITCOIND=bitcoin-node ./test/functional/test_runner.py
Also built depends without NO_QT=1
and tested ./src/bitcoin-gui
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK e2bab2a on macOS 10.15.4
Depends builds now. Without depends still works fine too, using capnp
0.7.0 from homebrew, and the latest libmultiprocess
self compiled. Ideally you should tag a libmultiprocess
(pre-)release after this lands in master, or right before 0.21 branches off.
I'm no longer able to reproduce a problem with BITCOIND=bitcoin-node test/functional/feature_backwards_compatibility.py
Direct link to IRC meeting log for April 16th: http://www.erisian.com.au/meetbot/bitcoin-core-dev/2020/bitcoin-core-dev.2020-04-16-19.03.log.html#l-243
I thought we were going to pivot this to just add a
-process=node/wallet/gui
option to the samebitcoin-qt
binary?
think the current naming is better, but I pointed out in the IRC meeting it is easy to change.
I think separate binaries is a better approach for now. For development I'd like to have them both, so I don't have to reconfigure to see if a bug is related to the multiprocess stuff. I imagine at some point we'll feel comfortable shipping the multi-process binaries, with their dependencies, but not comfortable enough to make them the default.
@@ -0,0 +1,18 @@ | |||
package=native_libmultiprocess | |||
$(package)_version=5741d750a04e644a03336090d8979c6d033e32c0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need to change anything else: maybe bump to 9f5b8355649aca94b4b529d89ba439aa3b2f9642 for consistency with building from master (but the new commits shouldn't be relevant here)
$(package)_download_file=capnproto-c++-$($(package)_version).tar.gz | ||
$(package)_file_name=capnproto-cxx-$($(package)_version).tar.gz | ||
$(package)_sha256_hash=c9a4c0bd88123064d483ab46ecee777f14d933359e23bff6fb4f4dbd28b4cd41 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @fanquake suggested elsewhere, you could add this:
define $(package)_set_vars
$(package)_config_opts=--without-openssl
endef
Note that at least on macOS it won't pick up my homebrew installed openssl@1.1: stable 1.1.1g
even when I use --with-openssl
, but I think it's worth explicitly opting out anyway.
You mentioned:
But maybe requesting to be built without openssl when openssl isn't available would be a good way to demonstrate not liking openssl
It's also consistent with the practice in other dependencies to actively opt out of stuff (see e.g. #17730, #16370)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #18677 (comment)
It's also consistent with the practice in other dependencies to actively opt out of stuff (see e.g. #17730, #16370)
Thanks, I will make that change in a followup, or here if another update is required (reluctant to make changes now it seems like all the build problems so far have been ironed out).
At the time I made that comment about OpenSSL I mistakenly thought the depends build was more hermetic, and that it wouldn't pick up system dependencies in host package builds. This was before I knew about #18915
I think it might be a good time to merge this PR. It seems to be at a point where it is working for the people who've tested it on different platforms. There are definitely improvements to be made going forward, but we know build changes are fragile and a fix for one bug tends to lead to other bugs, so it would be great to be done with this big PR when it's in a pretty good state, and start to track future issues in smaller more focused PRs. |
Maybe a Gitian build to top it off?
|
Thanks. I'm planning on merging this shortly. |
ACK e2bab2a Thanks for doing this excellent architectural work: a dream come true (even if optional)! With this dream off the table I think I only have Sandboxed API integration, control-flow integrity and killing the UUM bug class for good using Thanks @ryanofsky: you're great, and so is your patience -- this has been a very long road :) |
Checked that the ci config file is indeed native and runs on aarch64:
|
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
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
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.
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.
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.
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.
…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
The bare minimum necessary change for bitcoin#19764. Marked as "partial merge" for attribution only, otherwise considered unmerged
This PR is part of the process separation project.
This PR consists of build changes only. It adds an
--enable-multiprocess
autoconf option (off by default and marked experimental), that builds newbitcoin-node
andbitcoin-gui
binaries. These currently function the same as existingbitcoind
andbitcoin-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 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)