Skip to content

fix(crc32c): escape the version dot and anchor the pyenv check regex#17555

Draft
chalmerlowe wants to merge 5 commits into
mainfrom
feat/fix-crc32c-osx-pyenv-build
Draft

fix(crc32c): escape the version dot and anchor the pyenv check regex#17555
chalmerlowe wants to merge 5 commits into
mainfrom
feat/fix-crc32c-osx-pyenv-build

Conversation

@chalmerlowe

@chalmerlowe chalmerlowe commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Problem

The OS X python wheel build script (build_python_wheel.sh) checks if a Python version is installed in pyenv before building:

if [ -z "$(pyenv versions --bare | grep $version)" ]

When $version is 3.14, this expands to grep 3.14. Because the dot (.) is not escaped, it is treated as a wildcard and will also match the substring 3.13.14 (which contains 3.14 at the end). This results in a false positive, causing the script to skip compiling Python 3.14. The script then fails when executing pyenv shell 3.14 with a "version not installed" error.

Solution

Escape the version dots in bash: ${version//./\.}.
Anchor the grep match with line start (^) and word boundaries (\b) to ensure it only matches the exact version line (e.g. ^3\.14\b instead of matching 3.13.14).
Applied this fix to both build_python_wheel.sh and publish_python_wheel.sh.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request improves Python version matching in macOS build and publish scripts by escaping dots and using anchors/word boundaries in grep patterns. The feedback suggests refactoring the bash conditional checks from [ -z "$(pipeline | grep ...)" ] to a more idiomatic and efficient if ! pipeline | grep -q ...; then pattern to avoid unnecessary subshells and string comparisons.

Comment thread packages/google-crc32c/scripts/osx/build_python_wheel.sh Outdated
Comment thread packages/google-crc32c/scripts/osx/publish_python_wheel.sh Outdated
@chalmerlowe chalmerlowe reopened this Jun 25, 2026
Comment thread packages/google-crc32c/scripts/osx/build_python_wheel.sh
@chalmerlowe

Copy link
Copy Markdown
Contributor Author

Re: the failing cover test:

When google-crc32c gets a change to core code, it triggers as a diff in CI/CD and attempts to run unittests in the unit job in workflows/unittest.yml.
Within that job the unit nox session is run via a call to ci/run_conditional_tests.sh.

However... in the crc32c noxfile.py it uses session.skip for unit tests because they are not implemented.

This means NO coverage results are produced.
Which means when the unittest.yml file rolls down to the step that runs coverage, it blows up.

Note

running coverage is entirely managed in the unittest.yml file.
It does NOT run as a nox session defined in the crc32c noxfile.py. In fact there is no cover session, such as we have in other packages.
Thus we can't just go in and tell nox to skip the cover session the way we did with the unit session.

OPTIONS under consideration:

  1. Add a filter in the unittest.yml to ignore crc32c when trying launch coverage. We don't yet know how that works in terms of publishing a "success" result so that the CI/CD check succeeds.
  2. Add a dummy test (with dummy output) in the crc32c noxfile unit test to create coverage files that reflect 100% coverage for crc32c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant