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

depends: Pin clang search paths for darwin host #19683

Merged

Conversation

dongcarl
Copy link
Contributor

@dongcarl dongcarl commented Aug 7, 2020

Hello clang/lib/frontend,
I search your headers once again.
Because it's time for some housekeeping,
Within the code I was tweaking,
And the targets I was making with my build,
Are unfulfilled,
It's just language compliance.

In reference works I scroll alone
Pages cribbed from holy tomes
In the details of a template
My code's behaviour has now found its fate
When my hopes were dashed as a note left it as described:
As undefined
It's not in compliance

And from the standard text I saw
Ten thousand errors, maybe more
Threading used without locking
Pointers referenced after freeing
Linters writing warnings that coders will never fix
But still they tick
The box that claims compliance

"Fools," said I, "you do not know"
Errors, like a cancer, grow
Hear my words that I might reach you
Use -Wall and it might teach you
But my words and compiler errors fade.
Schedules forbade compliance.

And the people bowed and prayed
With static checking torn and frayed
The markets flashed out their warning
In the words that they were forming
As recruiters said "The search for more profits leads to writing stuff in CSS,
And node.js.
Without a need for compliance"

Many thanks to ajtowns for the above contribution!


This PR is ready for review!

When cross-compiling for macOS, the SDK gives us the entire context/sysroot on which we should base the build. This means that we can be extremely specific w/re our search path ordering in order to avoid build problems that arise out of a user's specific environment/system setup and improve the robustness of our macOS toolchain. This PR does 2 things to this end:

  1. Unset environment variables which are known to alter search paths.

  2. Makes us (in the case of macOS builds) explicitly specify the list of system include search paths and its ordering, rather than rely on clang's unreliable autodetection routine. Here is the rabbit-hole gist.

    See the added comments in depends/hosts/darwin.mk for more details:

    # -Xclang -*system"<path_a>" \
    # -Xclang -*system"<path_b>" \
    # -Xclang -*system"<path_c>" ...
    #
    # Adds path_a, path_b, and path_c to the bottom of clang's list of
    # include search paths. This is used to explicitly specify the list of
    # system include search paths and its ordering, rather than rely on
    # clang's autodetection routine. This routine has been shown to:
    # 1. Fail to pickup libc++ headers in $SYSROOT/usr/include/c++/v1
    # when clang was built manually (see: https://github.com/bitcoin/bitcoin/pull/17919#issuecomment-656785034)
    # 2. Fail to pickup C headers in $SYSROOT/usr/include when
    # C_INCLUDE_DIRS was specified at configure time (see: https://gist.github.com/dongcarl/5cdc6990b7599e8a5bf6d2a9c70e82f9)
    #
    # Talking directly to cc1 with -Xclang here grants us access to specify
    # more granular categories for these system include search paths, and we
    # can use the correct categories that these search paths would have been
    # placed in if the autodetection routine had worked correctly. (see:
    # https://gist.github.com/dongcarl/5cdc6990b7599e8a5bf6d2a9c70e82f9#the-treatment)
    #
    # Furthermore, it places these search paths after any "non-Xclang"
    # specified search paths. This prevents any additional clang options or
    # environment variables from coming after or in between these system
    # include search paths, as that would be wrong in general but would also
    # break #include_next's.

We can be this specific only because macOS builds are neatly contained in an SDK, and we are cross-compiling. Native toolchains should rely on the environment/distro/user to know how best to build for the running system.

Note: Although the -u flag of env is not a POSIX standard flag, it seems like it is useful enough to be implemented in coreutils, busybox, FreeBSD.

@dongcarl dongcarl requested a review from theuni August 7, 2020 17:41
@dongcarl dongcarl added this to PRs in guix build Aug 7, 2020
@fanquake
Copy link
Member

Concept ACK - did not read the @ajtowns poem. Did you want to link to your other recent gist here as well?

@dongcarl
Copy link
Contributor Author

dongcarl commented Aug 10, 2020

Did you want to link to your other recent gist here as well?

Added to description! It's also in the comments added to darwin.mk.

@DrahtBot
Copy link
Contributor

Guix builds

File commit 197450f
(master)
commit d9bf0c9
(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz 8943872ac3d5df7c... d794e47112269afb...
*-aarch64-linux-gnu.tar.gz 7307c4ee3d4934bd... e1c0d9e7ec4595c4...
*-arm-linux-gnueabihf-debug.tar.gz fccc26d7c7dfa90b... c91689dbc8d561cf...
*-arm-linux-gnueabihf.tar.gz 34e1140f65a0f606... 315c0695d8e6b05e...
*-riscv64-linux-gnu-debug.tar.gz a94385d318571b00... 59c84856272e8ed9...
*-riscv64-linux-gnu.tar.gz 3abb016b01e33949... d67c08201b63b914...
*-win-unsigned.tar.gz f35d661c7f1bbdae... cfe56e013cffd22d...
*-win64-debug.zip 62437d10c94cfef6... 4e99dd7d12a58df5...
*-win64-setup-unsigned.exe 5e2b6831dd645b79... 5f881cdc73dff930...
*-win64.zip c9829996dc3386f5... b19b255203e71057...
*-x86_64-linux-gnu-debug.tar.gz effc996453960003... e7f7bf96c01bda79...
*-x86_64-linux-gnu.tar.gz 7ef18c4f4cd4f2e2... 831d078480a6c38e...
*.tar.gz 5c60a590f5e9971c... 26ced5e629e9442e...
guix_build.log 3eca0392f5149475... d622e2144f1d27de...
guix_build.log.diff 41f7c68911c50f38...

@DrahtBot
Copy link
Contributor

Gitian builds

File commit 93ab136
(master)
commit bdde84c
(master and this pull)
*-aarch64-linux-gnu-debug.tar.gz d9677652e379d0fb... 8f31f5cfa71cbf24...
*-aarch64-linux-gnu.tar.gz ccea6adc4185f412... 9bff9b0232aac533...
*-arm-linux-gnueabihf-debug.tar.gz a6eb099566fbfaf7... bb3623af1913ff5e...
*-arm-linux-gnueabihf.tar.gz a778747de3c362d3... 5d0db25bd4d4a4ae...
*-osx-unsigned.dmg 7f2fe092602c5f24... 60478ecfe49cb20c...
*-osx64.tar.gz 572ad9470570b795... 1c0f22743bb14727...
*-riscv64-linux-gnu-debug.tar.gz e3c3530a641bf3ff... f8fc251b5c0a5865...
*-riscv64-linux-gnu.tar.gz 9e35a022bcbb24b5... 93e7e066ba6ff587...
*-win64-debug.zip 183ec1685b274201... 1af6f04e68b5e7fd...
*-win64-setup-unsigned.exe b0345c819e39cf5b... 48940692c654388b...
*-win64.zip ee5279efd7b75303... ec16942779035be9...
*-x86_64-linux-gnu-debug.tar.gz 1528d365bd4947b0... 6b1c825a3530c37a...
*-x86_64-linux-gnu.tar.gz 721e213b9f0ada44... 9b7b9b889bd607b0...
*.tar.gz 5e4e2d09bfcdc38c... 9a328246657d6bbc...
bitcoin-core-linux-0.21-res.yml 0366af80fb44ac95... 7c30769618819c28...
bitcoin-core-osx-0.21-res.yml 1635b70517fa98f9... 9d8ac5adfad89196...
bitcoin-core-win-0.21-res.yml 25db96826213443f... e8272d520bb4d12b...
linux-build.log 6c3144ff05037c6e... 04796c1da003d08b...
osx-build.log ce3a375d3051d5ba... 4f55ee0961daa8e9...
win-build.log 3a0f70b25edb7239... fae8119f5c5fdfd5...
bitcoin-core-linux-0.21-res.yml.diff e48921ddf9c03320...
bitcoin-core-osx-0.21-res.yml.diff 1c8900aa1d2ee134...
bitcoin-core-win-0.21-res.yml.diff 1f91673bd536ecbf...
linux-build.log.diff 9a5a4b928e8c6355...
osx-build.log.diff b21f11c25e247b41...
win-build.log.diff 6d3c16143aec17b4...

@dongcarl dongcarl force-pushed the 2020-08-clang-search-path-robustness branch from abc1325 to 79f4693 Compare September 25, 2020 20:12
@dongcarl dongcarl changed the title WIP: depends: Explicitly specify clang search paths for darwin host depends: Pin clang search paths for darwin host Sep 25, 2020
@dongcarl dongcarl marked this pull request as ready for review September 25, 2020 20:27
@dongcarl
Copy link
Contributor Author

This PR is now based on #20019, please review that first. I've added some more details to the original description and I believe this is in a good state to be reviewed and merged (after #20019 ofc).

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 24, 2020

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.

@dongcarl
Copy link
Contributor Author

dongcarl commented Nov 24, 2020

Updated so that it no longer depends on #20019.

@maflcko
Copy link
Member

maflcko commented Nov 24, 2020

Build failure. Verbose build follows.
Making all in src
make[1]: Entering directory '/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin18/src'
make[2]: Entering directory '/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin18/src'
/bin/bash ../libtool  --tag=CXX --preserve-dup-deps  --mode=compile /usr/bin/ccache env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH -u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH -u LIBRARY_PATH clang++ --target=x86_64-apple-darwin18 -mmacosx-version-min=10.14 -B/tmp/cirrus-ci-build/depends/x86_64-apple-darwin18/native/bin -mlinker-version=530 --sysroot=/tmp/cirrus-ci-build/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers -stdlib=libc++ -nostdinc++ -Xclang -cxx-isystem/tmp/cirrus-ci-build/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/usr/include/c++/v1 -Xclang -internal-externc-isystem/tmp/cirrus-ci-build/depends/x86_64-apple-darwin18/native/lib/clang/8.0.0/include -Xclang -internal-externc-isystem/tmp/cirrus-ci-build/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/usr/include -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config   -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I./obj -I./secp256k1/include -DBUILD_BITCOIN_INTERNAL -I/tmp/cirrus-ci-build/depends/x86_64-apple-darwin18/include/  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0  -Wstack-protector -fstack-protector-all -fcf-protection=full   -Werror=gnu -Werror=vla -Werror=shadow-field -Werror=switch -Werror=thread-safety -Werror=range-loop-analysis -Werror=unused-variable -Werror=date-time -Werror=return-type -Werror=conditional-uninitialized -Werror=sign-compare -Werror=unreachable-code-loop-increment    -pipe -O2  -fvisibility=hidden -c -o support/libbitcoinconsensus_la-cleanse.lo `test -f 'support/cleanse.cpp' || echo './'`support/cleanse.cpp
libtool: compile:  /usr/bin/ccache env -u C_INCLUDE_PATH -u CPLUS_INCLUDE_PATH -u OBJC_INCLUDE_PATH -u OBJCPLUS_INCLUDE_PATH -u CPATH -u LIBRARY_PATH clang++ --target=x86_64-apple-darwin18 -mmacosx-version-min=10.14 -B/tmp/cirrus-ci-build/depends/x86_64-apple-darwin18/native/bin -mlinker-version=530 --sysroot=/tmp/cirrus-ci-build/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers -stdlib=libc++ -nostdinc++ -Xclang -cxx-isystem/tmp/cirrus-ci-build/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/usr/include/c++/v1 -Xclang -internal-externc-isystem/tmp/cirrus-ci-build/depends/x86_64-apple-darwin18/native/lib/clang/8.0.0/include -Xclang -internal-externc-isystem/tmp/cirrus-ci-build/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/usr/include -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I./obj -I./secp256k1/include -DBUILD_BITCOIN_INTERNAL -I/tmp/cirrus-ci-build/depends/x86_64-apple-darwin18/include/ -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0 -Wstack-protector -fstack-protector-all -fcf-protection=full -Werror=gnu -Werror=vla -Werror=shadow-field -Werror=switch -Werror=thread-safety -Werror=range-loop-analysis -Werror=unused-variable -Werror=date-time -Werror=return-type -Werror=conditional-uninitialized -Werror=sign-compare -Werror=unreachable-code-loop-increment -pipe -O2 -fvisibility=hidden -c support/cleanse.cpp  -fno-common -DPIC -o support/.libs/libbitcoinconsensus_la-cleanse.o
/usr/bin/env: ‘clang++’: No such file or directory
Makefile:14178: recipe for target 'support/libbitcoinconsensus_la-cleanse.lo' failed

@dongcarl
Copy link
Contributor Author

Currently figuring out how to make ccache work properly with this change. Looks like CCACHE_PREFIX should be set somehow.

@dongcarl dongcarl force-pushed the 2020-08-clang-search-path-robustness branch 2 times, most recently from a3c1505 to 549e2ae Compare December 21, 2020 22:28
kwvg added a commit to kwvg/dash that referenced this pull request Jul 15, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Jul 20, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Jul 20, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Jul 20, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Jul 20, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 1, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 24, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 24, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 24, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 24, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 25, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 25, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 26, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 26, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 26, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 27, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Aug 30, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Sep 1, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Sep 1, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Sep 1, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Sep 1, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Sep 2, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Sep 3, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Sep 3, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Sep 3, 2021
gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 4, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
guix build
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants