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

ci: Fix "macOS native" job #29610

Merged
merged 2 commits into from Mar 12, 2024
Merged

ci: Fix "macOS native" job #29610

merged 2 commits into from Mar 12, 2024

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Mar 10, 2024

Homebrew promoted python@3.12 to the default python3. Now, our "macOS native" CI job is facing the following issues:

  1. Installing qt@5 requires re-installing python@3.12:
==> Fetching dependencies for qt@5: readline, python@3.12 and gettext
  1. Re-installing python@3.12 fails due to symbolic link conflicts on macOS x86_64:
==> Pouring python@3.12--3.12.2_1.ventura.bottle.tar.gz
Error: The `brew link` step did not complete successfully
  1. Homebrew's python@3.12 is marked as externally managed (according to PEP 668), necessitating different approaches for installing Python packages.

This pull request resolves all the issues mentioned above.

Promoting Homebrew's python@3.12 to the default python3 breaks symbolic
links on macOS x86_64.

This change adds a workaround for that issue.

Also see: actions/runner-images#9471 etc.
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 10, 2024

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK m3dwards

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

@DrahtBot DrahtBot added the Tests label Mar 10, 2024
@m3dwards
Copy link
Contributor

ACK eca024f

Don't have a x86 Mac to hand right now so ran mac cross CI job to test.

This works but two alternative approaches to the pip install problem could have been to either use a python virtual environment (venv) or to pass --break-system-packages when pip installing. As this is a throwaway build runner venv is probably overkill.

@maflcko
Copy link
Member

maflcko commented Mar 11, 2024

venv

I thought about this, too. However, I presume it would require activating the env twice. Once, when creating the container image (pip), and another time, when running the tests in the container.

--break-system-packages

If this causes a smaller diff, I'd say this is preferred.

@fanquake
Copy link
Member

It does seem not ideal to have to forcefully install one verison of Python, only to then not be able to use it for anything Python related, and then also refactor the CI to use a different (unclear which?) version of Python.

Homebrew's python@3.12 is marked as externally managed (PEP 668),
necessitating different approaches for installing Python packages.

For more details, please refer to https://github.com/orgs/Homebrew/discussions/3404.
@hebasto
Copy link
Member Author

hebasto commented Mar 11, 2024

@m3dwards @maflcko @fanquake

Thank you for your reviews!

--break-system-packages

If this causes a smaller diff, I'd say this is preferred.

Implemented.

It does seem not ideal to have to forcefully install one verison of Python, only to then not be able to use it for anything Python related, and then also refactor the CI to use a different (unclear which?) version of Python.

Reworked, so Homebrew's python is still using.

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

export PIP_PACKAGES="zmq"
# Homebrew's python@3.12 is marked as externally managed (PEP 668).
# Therefore, `--break-system-packages` is needed.
export PIP_PACKAGES="--break-system-packages zmq"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Seems fine to enable it globally in the CI, but this can be done in a follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. It's probably going to start coming up in other jobs as python and base containers get upgraded.

@m3dwards
Copy link
Contributor

reACK acc06bc to get the CI passing again.

Contrary to my previous comment, when making a more global change to CI perhaps the more permanent and proper thing would be to use a venv.

@maflcko maflcko requested a review from fanquake March 12, 2024 11:04
@fanquake
Copy link
Member

proper thing would be to use a venv.

Yea, it's pretty clear this is the direction we should be going, given we are going to start running into this with Python on other distros.

@fanquake fanquake merged commit bd55b7a into bitcoin:master Mar 12, 2024
16 checks passed
@hebasto hebasto deleted the 240309-homebrew branch March 12, 2024 18:56
hebasto added a commit to hebasto/bitcoin that referenced this pull request Mar 12, 2024
Add workaround for Homebrew's python link error.

Similar to bitcoin#29610.
hebasto added a commit to hebasto/bitcoin that referenced this pull request Mar 12, 2024
Add workaround for Homebrew's python link error.

Similar to bitcoin#29610.
hebasto added a commit to hebasto/bitcoin that referenced this pull request Mar 13, 2024
2de625d fixup! ci: Test CMake edge cases (Hennadii Stepanov)

Pull request description:

  Add workaround for Homebrew's python link error.

  Similar to bitcoin#29610.

ACKs for top commit:
  m3dwards:
    utACK 2de625d
  pablomartin4btc:
    cr ACK 2de625d

Tree-SHA512: 5f063138cbd0bdc7ba983c45662d6c30a7783c03b4ef21a1d4ddabc994334bb7eb5558666128308471b68524d93c0b7e03b1140e718ab38d6759ec934dff167b
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 14, 2024
Promoting Homebrew's python@3.12 to the default python3 breaks symbolic
links on macOS x86_64.

This change adds a workaround for that issue.

Also see: actions/runner-images#9471 etc.

Github-Pull: bitcoin#29610
Rebased-From: ae5f720
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 14, 2024
Homebrew's python@3.12 is marked as externally managed (PEP 668),
necessitating different approaches for installing Python packages.

For more details, please refer to https://github.com/orgs/Homebrew/discussions/3404.

Github-Pull: bitcoin#29610
Rebased-From: acc06bc
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 15, 2024
Promoting Homebrew's python@3.12 to the default python3 breaks symbolic
links on macOS x86_64.

This change adds a workaround for that issue.

Also see: actions/runner-images#9471 etc.

Github-Pull: bitcoin#29610
Rebased-From: ae5f720
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 15, 2024
Homebrew's python@3.12 is marked as externally managed (PEP 668),
necessitating different approaches for installing Python packages.

For more details, please refer to https://github.com/orgs/Homebrew/discussions/3404.

Github-Pull: bitcoin#29610
Rebased-From: acc06bc
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 18, 2024
Promoting Homebrew's python@3.12 to the default python3 breaks symbolic
links on macOS x86_64.

This change adds a workaround for that issue.

Also see: actions/runner-images#9471 etc.

Github-Pull: bitcoin#29610
Rebased-From: ae5f720
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Mar 18, 2024
Homebrew's python@3.12 is marked as externally managed (PEP 668),
necessitating different approaches for installing Python packages.

For more details, please refer to https://github.com/orgs/Homebrew/discussions/3404.

Github-Pull: bitcoin#29610
Rebased-From: acc06bc
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Mar 21, 2024
Promoting Homebrew's python@3.12 to the default python3 breaks symbolic
links on macOS x86_64.

This change adds a workaround for that issue.

Also see: actions/runner-images#9471 etc.

Github-Pull: bitcoin#29610
Rebased-From: acc06bc
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Mar 21, 2024
Homebrew's python@3.12 is marked as externally managed (PEP 668),
necessitating different approaches for installing Python packages.

For more details, please refer to https://github.com/orgs/Homebrew/discussions/3404.

Github-Pull: bitcoin#29610
Rebased-From: acc06bc
glozow pushed a commit to glozow/bitcoin that referenced this pull request Mar 25, 2024
Promoting Homebrew's python@3.12 to the default python3 breaks symbolic
links on macOS x86_64.

This change adds a workaround for that issue.

Also see: actions/runner-images#9471 etc.

Github-Pull: bitcoin#29610
Rebased-From: ae5f720
glozow pushed a commit to glozow/bitcoin that referenced this pull request Mar 25, 2024
Homebrew's python@3.12 is marked as externally managed (PEP 668),
necessitating different approaches for installing Python packages.

For more details, please refer to https://github.com/orgs/Homebrew/discussions/3404.

Github-Pull: bitcoin#29610
Rebased-From: acc06bc
@fanquake
Copy link
Member

Backported to 27.x in #29693 and 26.x in #29719.

fanquake added a commit that referenced this pull request Mar 25, 2024
cc0553d [doc] add manual pages for 26.1 (glozow)
785242d [doc] update release notes 26.1 (glozow)
5f06dcf [build] bump version to 26.1 final (glozow)
b53bf22 ci, macos: Use `--break-system-packages` with Homebrew's python (Hennadii Stepanov)
324e562 ci: Add workaround for Homebrew's python link error (Hennadii Stepanov)

Pull request description:

  Final changes for `v26.1`.

  Bins for rc2 have been available for 10 days and I haven't seen any bug reports or new things to add.
  Includes #29610 backport for the CI, which has no effect on what goes into the release.
  Website PR: bitcoin-core/bitcoincore.org#1009

ACKs for top commit:
  hebasto:
    ACK cc0553d.
  fanquake:
    ACK cc0553d
  stickies-v:
    ACK cc0553d (modulo CI passing)

Tree-SHA512: d032157c7cdf07a474e40b947f7e51bfc6a8e280e43345522bad67b6ad449d473f29bf03ee845b2e403693c1c81078589d042337c895eceb8a59cb4340432887
fanquake added a commit that referenced this pull request Mar 26, 2024
a7116c8 ci: Bump msan to llvm-18 (MarcoFalke)
05f69b3 ci, macos: Use `--break-system-packages` with Homebrew's python (Hennadii Stepanov)
603f036 ci: Add workaround for Homebrew's python link error (Hennadii Stepanov)
5d381cf serfloat: improve/simplify tests (Pieter Wuille)
f4be4d7 serfloat: do not test encode(bits)=bits anymore (Pieter Wuille)

Pull request description:

  Currently:
  * #29192
  * #29610
  * #29676

ACKs for top commit:
  stickies-v:
    ACK a7116c8 - all clean test backports

Tree-SHA512: f3508a2c20d336c8647ba16886859d6a070584c4739fc8b5cfce2041a0662794775fb0ce89c9bf848a29e70089bae05ad1c921bbe45afe3fd5cac2a5c6b76baf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants