Skip to content
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

Added a ci-wheels yml #135

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Added a ci-wheels yml #135

wants to merge 4 commits into from

Conversation

ESadek-MO
Copy link
Contributor

@ESadek-MO ESadek-MO commented Jan 22, 2024

Largely taken from python-stratify, with a couple continuity changes.

This might need a careful eye, it was largely trial and error.

@bjlittle bjlittle self-requested a review January 23, 2024 14:48
@bjlittle bjlittle self-assigned this Jan 23, 2024
@HGWright HGWright self-requested a review March 11, 2024 02:35
@bjlittle bjlittle removed their assignment Apr 9, 2024
Copy link
Contributor

@trexfeathers trexfeathers left a comment

Choose a reason for hiding this comment

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

Hi @ESadek-MO, I think this needs another look.

Largely taken from python-stratify, with a couple continuity changes.

python-stratify is not the right model for tephi, since stratify includes Cython and therefore needs to publish OS-specific builds (the job of cibuildwheel). I'd suggest looking at Iris, iris-esmf-regrid and nc-time-axis for an idea of how this workflow should be.

# - https://github.com/actions/checkout
# - https://github.com/actions/download-artifact
# - https://github.com/actions/upload-artifact
# - https://github.com/pypa/cibuildwheel
Copy link
Contributor

Choose a reason for hiding this comment

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

You're not using cibuildwheel

runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like Tephi is agnostic to OS - there are no OS-specific files here:

https://pypi.org/project/tephi/#files 1

So the matrix should be unnecessary. E.g. ci-wheels.yml in Iris does not have one.

Footnotes

  1. Compare this to stratify, which has many OS-specific files.

- "v*"

jobs:
build_bdist:
Copy link
Contributor

Choose a reason for hiding this comment

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

This job is not currently creating a built distribution ("bdist") - it only has 1 step - checking out the repo.

path: ${{ github.workspace }}/dist/*.tar.gz


show-artifacts:
Copy link
Contributor

Choose a reason for hiding this comment

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

Several other repos include a test stage. Looks like they download the artifacts and make sure that each provides a functional instance of the package.

Repos with this:

  • Iris
  • Iris-esmf-regrid
  • nc-time-axis

Repos without this:

  • Stratify
  • cf-units

It looks like the only times that we skip this are for repos that include Cython (and therefore build using cibuildwheel). Tephi is not in this camp, so could it include a testing stage?

name: "Publish to Test PyPI"
runs-on: ubuntu-latest
# upload to Test PyPI for every commit on main branch
if: github.event_name == 'push' && github.event.ref == 'refs/heads/main'
Copy link
Contributor

Choose a reason for hiding this comment

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

In other repos you have suggested a check for the repository owner, too: SciTools/iris#5180. Can that be included here too?

@@ -10,5 +10,4 @@ requires = [
"wheel",
]
# Defined by PEP 517
build-backend = "setuptools.build_meta"

build-backend = "setuptools.build_meta"
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks unintended

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants