Fix dependency typo and unpin cibuildwheel version in wheel building …

[nicholasjng]

…action.

Apologies for abandoning the Python wheels, I got sidetracked by some things over the last few months. In its current form, the wheel building action did not work due to a careless typo by me, that is fixed with this commit.

I started a first hail-mary workflow run in my fork just a few minutes ago. The results can be found here. In short, as a summary:

• macOS builds succeed (Python 3.6-3.9, macOS 11.0, x86 64-bit)
• Linux builds fail, presumably due to an environment misconfiguration (it fails to locate Bazel in the first place).
• Windows builds fail due to a linker error coming from pybind11 (?).

So that is currently 1/3. If someone has experience with setting up Bazel inside GitHub Actions (specifically Linux and Windows), I would love to get some direction on this.

cc @yashk2810 (who caught the typo here)

[dmah42]

brilliant! thank you :)

there's some bazel actions here that you can copy, and maybe this action will help with the pybind issue?

[dmah42]

did you want to land this change as is while you work through the other issues?

[nicholasjng]

did you want to land this change as is while you work through the other issues?

Thank you @dominichamon! I wanted to submit a working solution, so I will continue fixing the other stuff and rebasing until it works.

there's some bazel actions here that you can copy, and maybe this action will help with the pybind issue?

Hmmm ok, I'll try that out then. I'll focus on fixing the Linux bazel problem first, because that seems "in reach". Hope I won't run out of Github Actions Free Tier minutes along the way though :)

[dmah42]

did you want to land this change as is while you work through the other issues?

Thank you @dominichamon! I wanted to submit a working solution, so I will continue fixing the other stuff and rebasing until it works.

there's some bazel actions here that you can copy, and maybe this action will help with the pybind issue?

Hmmm ok, I'll try that out then. I'll focus on fixing the Linux bazel problem first, because that seems "in reach". Hope I won't run out of Github Actions Free Tier minutes along the way though :)

for the purposes of this PR if you set the action to trigger on pull request you should be able to use my quota.

[nicholasjng]

The failure confuses me even more considering that the ubuntu-latest image apparently contains Bazel 4.2.1:

https://github.com/actions/virtual-environments/blob/ubuntu20/20211017.0/images/linux/Ubuntu2004-README.md#tools

I created another branch, and the discovery fails even with a preceding successful Bazel cache mounting, so it seems something else is going on. Maybe Bazel does not show up on PATH (probably unlikely), or cibuildwheel does not have access to PATH.

Latest attempt: https://github.com/nicholasjng/benchmark/runs/4020132378

[dmah42]

The failure confuses me even more considering that the ubuntu-latest image apparently contains Bazel 4.2.1:

https://github.com/actions/virtual-environments/blob/ubuntu20/20211017.0/images/linux/Ubuntu2004-README.md#tools

I created another branch, and the discovery fails even with a preceding successful Bazel cache mounting, so it seems something else is going on. Maybe Bazel does not show up on PATH (probably unlikely), or cibuildwheel does not have access to PATH.

Latest attempt: https://github.com/nicholasjng/benchmark/runs/4020132378

i'm a bit out of my depth here but is it possible you're building from within a docker container that doesn't have bazel installed?

[nicholasjng]

i'm a bit out of my depth here but is it possible you're building from within a docker container that doesn't have bazel installed?

That was right! I fiddled around and it turns out that you have to install Bazel into the container itself. After some tries, I succeeded in building x86 wheels using the manylinux2014 image, summary here.

Apparently in this form cibuildwheel also wants to build aarch64, which I'm going to disable for now. That leaves Windows and the pybind11 linkage error.

Consider the failure for aarch64 after the x86_64 run; there, the bazel installation fails due to a failure to curl the bazel CentOS7 repo metadata. I am frequently getting 404s from curl in the runner there (not in the browser or on my own machine, though), so maybe there should be a retry policy set (otherwise the job will fail due to missing bazel)?

Let me know what you think.

[dmah42]

maybe you can make your life simpler using https://cibuildwheel.readthedocs.io/en/stable/options/#archs to get just a couple of basic ones working first.

[nicholasjng]

Good news, I have been able to get all wheels to build on all platforms after some tweaks.

Completed run: https://github.com/nicholasjng/benchmark/actions/runs/1434685479

Featured wheels (all including Python 3.7-3.10)

• manylinux2014 wheels
• macOS x86 and ARM64 wheels with 11.0 as deployment target
• Win AMD64 wheels.

Left TODO are:

• Testing the created wheels by running the Python bindings test.
• Emulating bazel inside the Docker container to allow for aarch64/ppc64le/i686 manylinux builds.
• win32 builds (these fail due to an MSVC linking error, no idea how to fix this yet).

[dmah42]

nice! is it ready for me to review?

[nicholasjng]

Yes, please!

[dmah42]

did you want to have this run on push instead of manually?

[dmah42]

maybe "build_windows" to match the others?

[nicholasjng]

TBH, I am not sure: The wheels should be built for every new release, since PyPI also groups them by release (and the release numbers are part of the wheel names).

I have seen repos with the trigger set to published release, but given that the Python bindings version and the Google Benchmark version are not the same, this would fail if a new GBM version gets published without a new Python bindings version. One could prevent this, though, by checking the release tag against the current. What do you think?

[dmah42]

TBH, I am not sure: The wheels should be built for every new release, since PyPI also groups them by release (and the release numbers are part of the wheel names).

gotcha.

I have seen repos with the trigger set to published release, but given that the Python bindings version and the Google Benchmark version are not the same,

i'm missing something, why are they not the same?

this would fail if a new GBM version gets published without a new Python bindings version. One could prevent this, though, by checking the release tag against the current. What do you think?

i don't know exactly how this would work but i would like to see this automated if at all possible. if not possible, then could you add instructions to the releasing doc? https://google.github.io/benchmark/releasing.html

[nicholasjng]

i'm missing something, why are they not the same?

That I am not sure about, either.

i don't know exactly how this would work but i would like to see this automated if at all possible. if not possible, then could you add instructions to the releasing doc? https://google.github.io/benchmark/releasing.html

This is definitely possible. I did this on a hobby project previously, I think I will use the logic from there.

Regarding the versioning, I see two possibilities (largely):

1. Sync the Python bindings version to match GBM, which would produce a gap (0.2.0 -> 1.6.x), but alas. Then, trigger wheel building on a new release, and add a reminder to bump the Python bindings version as well to the releasing doc.
2. Keep the version as is. Then, depending on the latest PyPI version, wheels may or may not have to be built upon release, which can probably be determined by a clever if-clause in the GitHub Action (TBA).

I personally favor option 1), but ultimately it is of course your call. Also, I should add a wheel pushing step to upload the built wheels to PyPI (using twine). Does this repo have a PyPI API Token as a GitHub secret? (That is usually how these uploading jobs work).

[dmah42]

option 1 sounds great and would make a lot of sense to users.

no, we've had no need for a pypi api token. let's tackle that as a separate PR maybe :)

[nicholasjng]

I added wheel test commands and a published release trigger in b1e8848. Every built wheel (except macOS ARM, which cannot yet be tested on GHA runners) passes the tests (one run through the Python bindings).

Also, I edited the setup.py to build wheels for minimum macosx_10_9 on x86, which supports a broader range of OSX releases - with the previous setup, everyone on anything older than macOS 11 would have been left out.

[dmah42]

happy for me to merge?

[nicholasjng]

Yeah, I guess we can do the PyPI publishing job separately (I would need your input on that again).

Would you like me to add a note about bumping the Python bindings version before tagging/release?

[dmah42]

oh, yes if the version needs to be bumped somewhere new then that should be in the docs/releasing.md doc.

[nicholasjng]

oh, yes if the version needs to be bumped somewhere new then that should be in the docs/releasing.md doc.

Done in 62e5038.

[dmah42]

thanks!