gh-101981: Build macOS as recommended by the devguide

[erlend-aasland]

• Issue: CI: intermittent macOS failures for make checksharedmods #101981

Automerge-Triggered-By: GH:erlend-aasland

[erlend-aasland]

Installing the Homebrew dependencies takes between 10 to 20 seconds of CI time. We can consider caching those deps (cc. @hugovk, as our CI/GitHub/DevOps oracle).

[erlend-aasland]

Apparently Tools/build/check_extension_modules.py is too fragile. We should harden it and ensure that it dumps enough debug info on failure, so we won't end up with a week of red CI crosses again.

[erlend-aasland]

I intend to merge this as soon as the CI turns green. If there are objections, we can always revert or create a follow-up PR.

[corona10]

@erlend-aasland What's difference between #101991?

[corona10]

lgtm

[erlend-aasland]

@erlend-aasland What's difference between #101991?

Oh, sorry, I missed you had a similar PR!

[erlend-aasland]

@erlend-aasland What's difference between #101991?

AFAICS, my PR is slightly cleaner in that it still uses multiple build steps (first set environment variables, then run configure, etc.); it makes debugging the CI easier. Also, I did not touch any of the tests.

[miss-islington]

Status check is done, and it's a success ✅.

[miss-islington]

Thanks @erlend-aasland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

[erlend-aasland]

Thanks for the review, Dong-hee!

[corona10]

it makes debugging the CI easier. Also, I did not touch any of the tests.

It looks like the GHA performance issue is resolved.
During the weekend it was not.

[corona10]

AFAICS, my PR is slightly cleaner in that it still uses multiple build steps (first set environment variables, then run configure

see: #101991 (comment)

[hugovk]

Re: caching Homebrew:

https://stackoverflow.com/a/65056232/724176 shows how to do it, but warns:

You should note that this cache is unlikely to speed up your workflow. Installing Homebrew bottles (the default) will usually have similar performance to downloading from GitHub/Azure's own storage cache since the bottle files are served on bintray's CDN.

https://stackoverflow.com/a/63681598/724176 has 3 options, which aren't great: 1 is caching everything; 2 means identifying and copying binaries; 3 is Docker.

---

Looking at the log:

==> Running `brew cleanup pkg-config`...
Disable this behaviour by setting HOMEBREW_NO_INSTALL_CLEANUP.

We can set that if it makes things quicker, we don't need to do any cleanup, the whole CI run is disposed of after the run. And the first SO answer above sets HOMEBREW_NO_AUTO_UPDATE, that may save time too.

Here's a run of main with this PR, brew install takes 25s:

• https://github.com/hugovk/cpython/actions/runs/4223815148/jobs/7333994448

With HOMEBREW_NO_AUTO_UPDATE it drops to 15s:

• https://github.com/hugovk/cpython/actions/runs/4223831861/jobs/7334030713

And with HOMEBREW_NO_INSTALL_CLEANUP it's 16s, basically the same:

• https://github.com/hugovk/cpython/actions/runs/4223836429/jobs/7334040607

So let's add them.

     needs: check_source
     if: needs.check_source.outputs.run_tests == 'true'
     env:
+      HOMEBREW_NO_AUTO_UPDATE: 1
+      HOMEBREW_NO_INSTALL_CLEANUP: 1
       PYTHONSTRICTEXTENSIONBUILD: 1
     steps:
     - uses: actions/checkout@v3

Edit: Well, these are all within the expected range anyway:

Installing the Homebrew dependencies takes between 10 to 20 seconds of CI time.

But these env vars shouldn't add time :)

[erlend-aasland]

@hugovk, yeah adding the HOMEBREW_* variables makes sense. @corona10, can you update your PR to include those?

[hugovk]

And they're documented at https://docs.brew.sh/Manpage#environment

[ned-deily]

Sorry that I'm a bit late to this discussion but I have to point out that there appears to be a bit of a misunderstanding here of what configure does and, thus, this PR is not doing what its title asserts :)

configure goes to great lengths to ensure that the necessary environment specified at the time configure is run is preserved in the generated Makefile such that whenever make is run the necessary environment variable values are created in the make environment. That means that, once you have run configure, you can go back later, in a different terminal session, say, and run make without having to define the essential environment variables (like CFLAGS and LDFLAGS) as they were set at configure time.

However, the Makefile does allow for augmenting those environment variables if they are defined in the environment when running make. So, if those environment variables have been defined externally when running make, you may get different results of a build. That's fine if that's what you want to do. But, by default, you should not have to define those variables for a normal build after running configure. That's the way things have worked for a very long time.

Note that the devguide recipe for 3.10+ deliberately does not export any environment variables in the shell environment when running configue and make; by placing the environment variables definitions on the configure command line, they are local to the configure process and its children (and preserved by it in the Makefile). With the github workflow as merged here, the CFLAGS and LDFLAGS values are effectively unnecessarily being exported into each subsequent build step by using GITHUB_ENV. This isn't just a stylistic change; it bypasses a small but important test of the build process that those variables are being properly preserved and recreated by make steps so that we are confident that the Makefile stands alone and is not dependent on externally defining any configuration-related variables.

[erlend-aasland]

@ned-deily, this is true (I noticed my error post merge, see my comments on Discord). Dong-hee had a fixup PR ready at some point, but he reverted that specific change. I definitely agree that we should provide the environment variables only when configure is run!

[miss-islington]

Thanks @erlend-aasland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

[miss-islington]

Thanks @erlend-aasland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

[miss-islington]

Sorry, @erlend-aasland, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 27136310414965a3ea7f835e416cf74b91cefb48 3.9

[miss-islington]

Sorry @erlend-aasland, I had trouble checking out the 3.8 backport branch.
Please retry by removing and re-adding the "needs backport to 3.8" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 27136310414965a3ea7f835e416cf74b91cefb48 3.8

[erlend-aasland]

I'll create backports for 3.9 and 3.8 with the fixup in #102131 applied.

[bedevere-bot]

GH-102186 is a backport of this pull request to the 3.9 branch.

[bedevere-bot]

GH-102187 is a backport of this pull request to the 3.8 branch.