-
Notifications
You must be signed in to change notification settings - Fork 88
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
chore: bump cibuildwheel version, use action #309
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Henry! This is great 😄
Had a couple questions below 🙂
- name: Build wheel | ||
run: | | ||
python -m cibuildwheel --output-dir wheelhouse | ||
- uses: pypa/[email protected] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really like this simplification 😄
@@ -53,7 +43,7 @@ jobs: | |||
python-version: '3.9' | |||
|
|||
- name: Build sdist | |||
run: python setup.py sdist | |||
run: pipx run build --sdist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add pipx
as a requirement somewhere? Or is that already installed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's built into the default runners as a supported package manager. See https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-Readme.md or any other platform under https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-software . It does not require setup-python (that could be moved below this line). (pipx is included in manylinux images, too, which is really handy when you want to run something like nox/tox on many Pythons ;) )
@@ -13,7 +13,7 @@ jobs: | |||
env: | |||
CIBW_TEST_COMMAND: pytest --pyargs numcodecs | |||
CIBW_TEST_REQUIRES: pytest | |||
CIBW_SKIP: "*27* pp* *35*" | |||
CIBW_SKIP: "pp*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT cibuildwheel still supports Python 3.6, correct? Is there anyway for us to state here that we still support it? Just want to make sure if an update to cibuildwheel drops Python 3.6 from cibuildwheel that we catch it (admittedly maybe we should be looking to drop Python 3.6 anyways just want to make sure we do that explicitly as opposed to implicitly due to tooling changes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we plan to support 3.6 until manylinux drops it, which is mid next year, according to pypa/manylinux#1260 (Ubuntu 18.04 EOL). Most of the time I split jobs based on Python version, like https://github.com/scikit-hep/boost-histogram/blob/develop/.github/workflows/wheels.yml (and you can see the toml config at https://github.com/scikit-hep/boost-histogram/blob/develop/pyproject.toml) - that will end up with an empty matrix element if a version was missing. You could just add a check after this for a file containing cp36 in the wheelhouse, though.
cibuildwheel will pick up python_requires
, by the way, ideally if you set it in your setup.cfg
, but also if it's very simply set in setup.py
. It uses this for skipping though, not really checking to see if it's missing anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sounds like there isn't a good way to do this, but that's ok since Numcodecs will most likely drop Python 3.6 before cibuildwheel does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again Henry! LGTM 😄
@joshmoore would you be able to review? 🙂 |
LGTM as well. We're currently at |
We probably want to add Python 3.10 to CI after this just to see what happens. Not expecting any surprises. Then should be good to try an alpha I think |
In that case, let me try it here. |
All the new 3.10 builds passed. Just waiting on the slower wheels builds now. |
Looks green. Sounds like we are happy here. So went ahead and merged. Hope that is ok 🙂 Thanks Henry and Josh! 😀 |
v0.10.0.a2 pushed pushed (Edit: pushed pushed pushed pushed) |
Followup to #308. General recommendations:
setup_requires
is deprecated and produces a warning.I'd also recommend testing on 3.10. You could add 3.10 and drop 3.6, if you want to keep to 4 versions (or non-EOL) versions supported.