contrib: add macOS test for fixup_chains usage #27999

pull fanquake wants to merge 2 commits into bitcoin:master from fanquake:test_chained_fixups changing 4 files +49 −12
  1. fanquake commented at 11:12 am on June 29, 2023: member

    Followup to #27676, adding the check for chained fixups.

    Somewhat annoyingly, we have to patch support for -no_fixup_chains into ld64. As it doesn’t seem to have been added until a later version.

    Guix Build:

    00e17d462808f86aa7157e27a957da88fd1adeb491ad6c01138aca93e5ad1d018  guix-build-7f96638723a0/output/arm64-apple-darwin/SHA256SUMS.part
1ceb208e6374f5d7367b73128e90ca6eaeea15d50c69e49c8cf75b47212525ad7  guix-build-7f96638723a0/output/arm64-apple-darwin/bitcoin-7f96638723a0-arm64-apple-darwin-unsigned.dmg
2e31663554cfde8a37a9f3438c9c895dde94b90ff87e28f12f78be71ef6421d93  guix-build-7f96638723a0/output/arm64-apple-darwin/bitcoin-7f96638723a0-arm64-apple-darwin-unsigned.tar.gz
368a7bbc42418641eab391a85725b5c2f3c46d38a7acc07e7a8cef98909be07ec  guix-build-7f96638723a0/output/arm64-apple-darwin/bitcoin-7f96638723a0-arm64-apple-darwin.tar.gz
438d966ad93e7384f4f1ce16faded003a675ecce7be1987e6c4eee8e4b82c0432  guix-build-7f96638723a0/output/dist-archive/bitcoin-7f96638723a0.tar.gz
59d314f595d897a715a321a9fba0d552220fbd4bf69aff84eb8c0001cdb48234f  guix-build-7f96638723a0/output/x86_64-apple-darwin/SHA256SUMS.part
6c218ebfd0e96348c4912e6d522492b621bb043ef45b75105ff1fde979d1004d0  guix-build-7f96638723a0/output/x86_64-apple-darwin/bitcoin-7f96638723a0-x86_64-apple-darwin-unsigned.dmg
71c5ff7fa82f5c76d7d8b9582ad5202f4a82a917102ecafdc3c1fb7b783f6bc3e  guix-build-7f96638723a0/output/x86_64-apple-darwin/bitcoin-7f96638723a0-x86_64-apple-darwin-unsigned.tar.gz
815fb01e5afcc842db6a3e793b42c70c05ce07bec79e0d2d605e241901ff9f639  guix-build-7f96638723a0/output/x86_64-apple-darwin/bitcoin-7f96638723a0-x86_64-apple-darwin.tar.gz
  2. build: support -no_fixup_chains in ld64 
    Patch in suport for using -no_fixup_chains, with ld64. This option just
seems to be missing from our version, as it exists in later releases.

This is needed so we can disable fixup_chains in our security checks.
    3dca683cb7
  3. contrib: add macOS fixup_chains check to security-check 
    Followup to #27676.
    7f96638723
  4. DrahtBot commented at 11:12 am on June 29, 2023: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK theuni, hebasto, TheCharlatan

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #21778 (build: LLVM 15 & LLD based macOS toolchain by fanquake)

    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.

  5. DrahtBot added the label Scripts and tools on Jun 29, 2023
  6. fanquake commented at 12:00 pm on June 29, 2023: member
    cc @theuni
  7. theuni approved
  8. theuni commented at 5:10 pm on June 29, 2023: member

    utACK 7f96638723a08edf4341a2f4c06b2aa41378fcf7.

    The patch looks good. At first glance at git blame makes it looks like this is setting the wrong var, but when reading in-context I agree it’s correct.

    Another reasonable option would be to bump ld64, but since this is the last thing we should ever need from it, I agree patching makes more sense.

    Also, I checked lld for no-fixup-chains support and it works fine:

    /opt/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/bin/clang++ –target=x86_64-apple-darwin -mmacosx-version-min=11.0 -mlinker-version=609 -isysroot/home/cory/dev/bitcoin2/depends/SDKs/Xcode-12.2-12B45b-extracted-SDK-with-libcxx-headers -nostdlibinc -std=c++17 -fuse-ld=lld -Wl,-no_fixup_chains test.cpp -o testing

  9. hebasto commented at 7:20 pm on June 29, 2023: member

    Concept ACK.

    Another reasonable option would be to bump ld64, but since this is the last thing we should ever need from it, I agree patching makes more sense.

    So, what is the plan for bumping ld64 in the future?

  10. fanquake commented at 7:24 pm on June 29, 2023: member

    So, what is the plan for bumping ld64 in the future?

    Use LLD.

  11. hebasto commented at 7:31 pm on June 29, 2023: member

    So, what is the plan for bumping ld64 in the future?

    Use LLD.

    Ah, I can see that the first commit has been pulled from #21778.

  12. hebasto commented at 8:49 am on June 30, 2023: member

    Guix builds:

    00e17d462808f86aa7157e27a957da88fd1adeb491ad6c01138aca93e5ad1d018  guix-build-7f96638723a0/output/arm64-apple-darwin/SHA256SUMS.part
1ceb208e6374f5d7367b73128e90ca6eaeea15d50c69e49c8cf75b47212525ad7  guix-build-7f96638723a0/output/arm64-apple-darwin/bitcoin-7f96638723a0-arm64-apple-darwin-unsigned.dmg
2e31663554cfde8a37a9f3438c9c895dde94b90ff87e28f12f78be71ef6421d93  guix-build-7f96638723a0/output/arm64-apple-darwin/bitcoin-7f96638723a0-arm64-apple-darwin-unsigned.tar.gz
368a7bbc42418641eab391a85725b5c2f3c46d38a7acc07e7a8cef98909be07ec  guix-build-7f96638723a0/output/arm64-apple-darwin/bitcoin-7f96638723a0-arm64-apple-darwin.tar.gz
438d966ad93e7384f4f1ce16faded003a675ecce7be1987e6c4eee8e4b82c0432  guix-build-7f96638723a0/output/dist-archive/bitcoin-7f96638723a0.tar.gz
59d314f595d897a715a321a9fba0d552220fbd4bf69aff84eb8c0001cdb48234f  guix-build-7f96638723a0/output/x86_64-apple-darwin/SHA256SUMS.part
6c218ebfd0e96348c4912e6d522492b621bb043ef45b75105ff1fde979d1004d0  guix-build-7f96638723a0/output/x86_64-apple-darwin/bitcoin-7f96638723a0-x86_64-apple-darwin-unsigned.dmg
71c5ff7fa82f5c76d7d8b9582ad5202f4a82a917102ecafdc3c1fb7b783f6bc3e  guix-build-7f96638723a0/output/x86_64-apple-darwin/bitcoin-7f96638723a0-x86_64-apple-darwin-unsigned.tar.gz
815fb01e5afcc842db6a3e793b42c70c05ce07bec79e0d2d605e241901ff9f639  guix-build-7f96638723a0/output/x86_64-apple-darwin/bitcoin-7f96638723a0-x86_64-apple-darwin.tar.gz
  13. hebasto approved
  14. hebasto commented at 9:10 am on June 30, 2023: member
    ACK 7f96638723a08edf4341a2f4c06b2aa41378fcf7, I have reviewed the code and the patch, and they look OK.
  15. TheCharlatan approved
  16. TheCharlatan commented at 1:44 pm on June 30, 2023: contributor
    ACK 7f96638723a08edf4341a2f4c06b2aa41378fcf7
  17. fanquake merged this on Jun 30, 2023
  18. fanquake closed this on Jun 30, 2023
  19. fanquake deleted the branch on Jun 30, 2023
  20. sidhujag referenced this in commit 53b7270bae on Jul 1, 2023
  21. bitcoin locked this on Jun 29, 2024


fanquake DrahtBot theuni hebasto TheCharlatan

Labels
Scripts and tools

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-05 19:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me