macOS: swap docs & CI from pkg-config to pkgconf #31335

pull fanquake wants to merge 2 commits into bitcoin:master from fanquake:pkg_config_to_pkg_conf changing 2 files +3 −2
  1. fanquake commented at 3:19 pm on November 20, 2024: member

    Migrate the macOS build docs and CI from pkg-config to pkgconf. As the former now just redirects to the later.

    Upstream is currently mass-migrating its formula. i.e https://github.com/Homebrew/homebrew-core/pull/198317.

    Fixes #31334.

  2. DrahtBot commented at 3:19 pm on November 20, 2024: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31335.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko, hebasto
    Stale ACK hodlinator

    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:

    • #30975 (Add multiprocess binaries to release build by Sjors)
    • #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
    • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
    • #10102 (Multiprocess bitcoin by ryanofsky)

    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.

  3. fanquake force-pushed on Nov 20, 2024
  4. fanquake force-pushed on Nov 20, 2024
  5. hebasto commented at 3:43 pm on November 20, 2024: member
    Concept ACK.
  6. fanquake commented at 3:49 pm on November 20, 2024: member
    Given that the CI is now working here, (and #31334 wasn’t one-off, it’s happening for all new pushes (i.e #29881)), have updated the commit message and PR description.
  7. fanquake force-pushed on Nov 20, 2024
  8. hodlinator approved
  9. hodlinator commented at 11:24 pm on November 20, 2024: contributor

    utACK fa9e7fb167d7a1c476af931610ccbae39e29b964

    Don’t have a valid Mac to test on but change looks very straightforward and is in a similar vane to the linked upstream change.

  10. DrahtBot requested review from hebasto on Nov 20, 2024
  11. hodlinator commented at 11:28 pm on November 20, 2024: contributor

    Add a workaround for #31334.

    Sounds like a TODO? Why not “Fixes #31334”?

  12. fanquake commented at 9:43 am on November 21, 2024: member

    Sounds like a TODO? Why not “Fixes #31334”?

    Sure, changed to fixes. However it is a TODO in the sense that we shouldn’t need to do any of this for a working CI, and ideally, these changes should be removed at some point, as they are all workaround for other problems.

  13. in .github/workflows/ci.yml:112 in fa9e7fb167 outdated 
    108@@ -109,7 +109,8 @@ jobs:
109         run: |
110           # A workaround for "The `brew link` step did not complete successfully" error.
111           brew install --quiet python@3 || brew link --overwrite python@3
112-          brew install --quiet coreutils ninja pkg-config gnu-getopt ccache boost libevent zeromq qt@5 qrencode
113+          brew unlink pkg-config
    maflcko commented at 9:53 am on November 21, 2024:
    Looks like https://github.com/bitcoin/bitcoin/pull/31337/checks passed CI recently, so maybe this was just a temporary issue?
    fanquake commented at 10:11 am on November 21, 2024:
    Dropped the workaround, and it seems like using pkgconf is still broken, so we’d need to retain the workaround. We could also just update only the docs, and change the CI later if it starts breaking again.
    maflcko commented at 10:17 am on November 21, 2024:
    I’d presume you could only update the CI after GHA has updated their image?
    fanquake commented at 10:33 am on November 21, 2024:
    Probably. Added a note for that, dropping the package change, but now the build is failing again, with the same as #31334.
    hebasto commented at 10:35 am on November 21, 2024:
    Or we can brew update. However, it might be time consuming.
    hebasto commented at 10:50 am on November 21, 2024:
    It seems that unlink is still required because the pkg-config package is preinstalled in the GHA image.
    maflcko commented at 11:05 am on November 21, 2024:

    Though, I am wondering if brew unlink pkg-config will start failing once pkg-config is removed from the GHA image?

    Wouldn’t a better fix be to drop the unlink and the install and add a comment to add install pkgconf in the future?

    fanquake commented at 11:06 am on November 21, 2024:

    Wouldn’t a better fix be to drop the unlink and the install and add a comment to add install pkgconf in the future?

    See #31335 (review).

    maflcko commented at 11:20 am on November 21, 2024:

    Wouldn’t a better fix be to drop the unlink and the install and add a comment to add install pkgconf in the future?

    See #31335 (comment).

    IIRC you still tried to install. However, my recommendation is to do neither. Otherwise, either can fail, depending on the state of the GHA image.

    I tested this in https://github.com/bitcoin/bitcoin/commit/4191e4a34c7f91fe1dfc68baa91998067a144619 and it passed. What am I missing?

    fanquake commented at 11:21 am on November 21, 2024:
    Nothing, the suggested change is already in this PR.
  14. fanquake force-pushed on Nov 21, 2024
  15. doc: migrate from pkg-config to pkgconf in macOS build docs 
    Brew has migrated to using the later:
```bash
brew info pkg-config
==> pkgconf: stable 2.3.0 (bottled), HEAD
Package compiler and linker metadata toolkit
https://github.com/pkgconf/pkgconf
```
    8d203480b3
  16. fanquake force-pushed on Nov 21, 2024
  17. fanquake referenced this in commit 278687dcc0 on Nov 21, 2024
  18. fanquake force-pushed on Nov 21, 2024
  19. fanquake commented at 10:50 am on November 21, 2024: member
    This is the upstream issue https://github.com/actions/runner-images/issues/10984 and related fix: https://github.com/actions/runner-images/pull/11015.
  20. hebasto approved
  21. hebasto commented at 11:05 am on November 21, 2024: member
    ACK 278687dcc010d0a826e55b864164cf42beddff46.
  22. DrahtBot requested review from hodlinator on Nov 21, 2024
  23. ci: note that we should install pkgconf in future 
    As pkg-config is now just redirected to the later, but changing this likely
depends on the GHA image being updated first.
    fe3457ccff
  24. fanquake force-pushed on Nov 21, 2024
  25. maflcko approved
  26. maflcko commented at 11:23 am on November 21, 2024: member

    ACK fe3457ccfff9a022d9f183e18217422e2e1f7689 🍭

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK fe3457ccfff9a022d9f183e18217422e2e1f7689 🍭
3MAKSEnsFhDvAE/jAJfIhu71YebqfW2OqEWxmRpz6tOmZNLY5GlcCOrkV3BP1SqjgTdTSF89PyBEstFltNJrqAQ==
  27. DrahtBot requested review from hebasto on Nov 21, 2024
  28. hebasto approved
  29. hebasto commented at 12:01 pm on November 21, 2024: member
    re-ACK fe3457ccfff9a022d9f183e18217422e2e1f7689.
  30. fanquake merged this on Nov 21, 2024
  31. fanquake closed this on Nov 21, 2024
  32. fanquake deleted the branch on Nov 21, 2024
  33. hebasto commented at 12:40 pm on November 25, 2024: member

    From the macOS 14 arm64 (20241125) Image Update:

    Added: pkgconf 2.3.0 Deleted: pkg-config 0.29.2


fanquake DrahtBot hebasto hodlinator maflcko


hodlinator

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-12-03 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me