[zacharyburnett]

we can move these specific cibuildwheel configurations into pyproject.toml, and it will run them for each platform / arch

also, cibuildwheel can use emulation to build macOS Intel x86 wheels on the Apple Silicon platform

also also, we can run the wheels.yml workflow on every pull request to build wheels (with on: pull_request) and use an if conditional to only attempt publishing to PyPI on an actual release

[rmjarvis]

Why? What was wrong with the previous version? This is a pretty big overhaul with no (apparent to me) benefit. I'll let you make your case though.

also also, we can run the wheels.yml workflow on every pull request to build wheels (with on: pull_request) and use an if conditional to only attempt publishing to PyPI on an actual release

This in particular seems purely wasteful. Why would we want to build wheels on every random PR?

[zacharyburnett]

Why? What was wrong with the previous version? This is a pretty big overhaul with no (apparent to me) benefit. I'll let you make your case though.

This reduces the amount of jobs actually running, and GitHub runners spun up, from 4 (glibc Linux, muslc Linux, macOS x86, macOS ARM, plus the Linux containers that cibuildwheel starts) to 2 (GitHub-supplied ubuntu-latest and macos-latest, plus manylinux and musllinux spun up by cibuildwheel) by letting cibuildwheel handle the build matrix and also build the macOS x86 wheel with emulation alongside the macOS ARM64 wheel. We also don't need to install fftw on the final publish job in order to build the source dist, because we can build it alongside the wheel in the build jobs

Additionally, moving the meaningful configuration to pyproject.toml makes it a lot more explicit on what is actually different from defaults, and makes it much easier to maintain. In my experience configuration should be in one place or it will diverge and people will forget to update it. For instance, setting requires_python once in the pyproject.toml is much better IMO than copying CIBW_SKIP: cp36* cp37* cp38* pp* four times buried in the workflow YAML; if GalSim ever needs new syntax features that aren't backported and needs to update the minimum Python, there's much less configuration change required

also also, we can run the wheels.yml workflow on every pull request to build wheels (with on: pull_request) and use an if conditional to only attempt publishing to PyPI on an actual release

This in particular seems purely wasteful. Why would we want to build wheels on every random PR?

I personally like to run a build job on every pull request so I know about build failures early, otherwise I'm blindsided by build failures when it comes to release time and have to scramble. I can take that out though if you prefer; there's always the workflow_dispatch that can be run manually

[rmjarvis]

This reduces the amount of jobs actually running, and GitHub runners spun up, from 4 (glibc Linux, muslc Linux, macOS x86, macOS ARM, plus the Linux containers that cibuildwheel starts) to 2

This IMO is not particularly desirable. The reason for having 4 there was to reduce the human wait time for them all to finish building.

[rmjarvis]

I seem some benefits to the simplification here. But I have some significant concerns with the current implementation.

[rmjarvis]

What is the point of this? AFAIK, we only need permissions later when publishing. And specifying none here seems vaguely redundant.

[zacharyburnett]

the default permissions given to GITHUB_TOKEN are very broad and have been misused in security breaches where someone added malicious code to an action. We can mitigate the amount of access given by setting permissions: {} globally and then adding permissions to individual jobs (see https://docs.zizmor.sh/audits/#excessive-permissions)

[rmjarvis]

Pretty sure these are unnecessary. And fetch-depth=0 is surely a waste of bandwidth.

[zacharyburnett]

You're right, I usually work with projects that use setuptools_scm to retrieve the version from the Git tags in which case fetch-tags is necessary, and sometimes with projects that use release_branch_semver with release branches diverging from main, which needs fetch-depth: 0. But those are not needed here, I'll take them out

persist-credentials: false is recommended by zizmor to mitigate artifact packing that may exfiltrate credentials, either accidental or maliciously from upstream (see https://docs.zizmor.sh/audits/#remediation_2)

[rmjarvis]

What is the point of attestations?

[zacharyburnett]

https://docs.pypi.org/attestations/ attestations gives strong cryptographic proof that a binary was created by and signed by a specific source, in this case this github actions workflow. I don't think most users are actually checking attestations currently, but it's part of PyPI now so it doesn't hurt to have, especially if that ever changes. I can remove the attestation step if desired though

[rmjarvis]

I don't think this is doing anything useful. Including the sha here means the cache won't be reused I think. We only run this workflow once per commit, so I don't see the benefit of this.