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

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.

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.

fanquake force-pushed on Nov 20, 2024

hebasto commented at 3:43 pm on November 20, 2024: member

Concept ACK.

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.

fanquake force-pushed on Nov 20, 2024

hodlinator approved

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.

DrahtBot requested review from hebasto on Nov 20, 2024

hodlinator commented at 11:28 pm on November 20, 2024: contributor

Add a workaround for #31334.

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

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.

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.

fanquake force-pushed on Nov 21, 2024

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

fanquake force-pushed on Nov 21, 2024

fanquake referenced this in commit 278687dcc0 on Nov 21, 2024

fanquake force-pushed on Nov 21, 2024

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.

hebasto approved

hebasto commented at 11:05 am on November 21, 2024: member

ACK 278687dcc010d0a826e55b864164cf42beddff46.

DrahtBot requested review from hodlinator on Nov 21, 2024

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

fanquake force-pushed on Nov 21, 2024

maflcko approved

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==

DrahtBot requested review from hebasto on Nov 21, 2024

hebasto approved

hebasto commented at 12:01 pm on November 21, 2024: member

re-ACK fe3457ccfff9a022d9f183e18217422e2e1f7689.

fanquake merged this on Nov 21, 2024

fanquake closed this on Nov 21, 2024

fanquake deleted the branch on Nov 21, 2024

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

TheCharlatan referenced this in commit bd64a555d7 on Dec 3, 2024

TheCharlatan referenced this in commit 15319cfcd5 on Dec 4, 2024

bradmock84 approved

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

Code Coverage & Benchmarks

Reviews

Conflicts