Skip to content

MAINT: Create CircleCI job to test against wheels (binary distributions)#310

Open
mberz wants to merge 17 commits intomainfrom
ci/test_wheels
Open

MAINT: Create CircleCI job to test against wheels (binary distributions)#310
mberz wants to merge 17 commits intomainfrom
ci/test_wheels

Conversation

@mberz
Copy link
Copy Markdown
Member

@mberz mberz commented Mar 30, 2026

Which issue(s) are closed by this pull request?

Closes #309

Changes proposed in this pull request:

  • Add test against the binary distribution (wheels) to the CircleCI pipeline
  • Also re-organized the test pipeline a bit. Built products are now stored in the workspace and passed between runners to avoid duplicate steps.
  • Removed commented out installation of system dependencies.

Note: tests are expected to fail, since this PR helps to detect issues such as reported in #308, see #309 for further info.

@mberz mberz added the ci label Mar 30, 2026
@mberz mberz moved this from Backlog to Implementation in progress in Weekly Planning Mar 30, 2026
@mberz mberz added this to the v1.0.1 milestone Mar 30, 2026
@mberz mberz changed the title Create CircleCI job to test against wheels (binary distributions) MAINT: Create CircleCI job to test against wheels (binary distributions) Mar 30, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the CircleCI pipeline to add test coverage for installed wheel distributions and to reuse built dist/ artifacts across subsequent jobs via the workspace, helping detect packaging issues like missing non-Python files.

Changes:

  • Added a build_distribution job that builds sdist/wheel artifacts and persists dist/ to the workspace.
  • Added a test_wheel job that installs the built wheel into an isolated venv and runs the test suite against the installed distribution.
  • Rewired publish/check jobs and workflow dependencies to consume dist/ from the workspace; removed commented-out system dependency install steps.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@mberz mberz moved this from Implementation in progress to Require review in Weekly Planning Mar 30, 2026
Copy link
Copy Markdown
Member

@ahms5 ahms5 left a comment

Choose a reason for hiding this comment

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

I love it. I wonder if we really need to test_source, as test_wheel should also fail in case test_source fails.

at: /tmp/workspace
- run:
name: install dependencies
command: pip install ".[deploy]"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
command: pip install ".[deploy]"
command: pip install twine

woudnt this be sufficiant?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In theory yes, but I'd like to avoid managing dependencies in the CircleCI config and the pyproject.toml. This way it's always sufficient to extend and update in a single place.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see your point. My thinking here is that this job is only invoking twine, which doesn’t depend on our project’s dependencies or anything that could vary with the package—it’s just a standalone tool.
We could compare it to ruff, but in that case we pin a specific version and install it via the project configuration. Imo this makes sence here, but we dont have this requriedment for twine.
If we want to change the dependency of twine we also need to change this workflow here.
Right now, installing all packages takes about 29 seconds in CI. Installing only twine should be significantly faster. This also scales better, since this job will run for every package on each pull request.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry, I still don't really get the point. You could also pin to a specific version within the CircleCI config. My point is more about central management of dependencies in the toml, instead of individual management of dependencies in different places.

I realized though, that its probably more useful to check the binary distribution within the built job instead of creating a new job. This probably saves more time than the 29 secs you mentioned.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

To be honest, I think this job can be removed altogether.
It only checks if the meta-data provided in the pyproject.toml is according to what PyPI expects. More specifically, it only checks the content of the item long_description.
I think it's sufficient to check this before truly releasing a new version and remove the check in PRs, etc.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

that would also work for me

@@ -150,18 +190,17 @@ jobs:
command: pip install ".[deploy]"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
command: pip install ".[deploy]"
command: pip install twine

same here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

see comment above

Comment on lines +83 to +94
name: Install wheel in isolated venv and run tests
command: |
python -m venv .wheeltest
WHEEL=$(ls -1 /tmp/workspace/dist/*.whl | head -n 1)
.wheeltest/bin/python -m pip install "${WHEEL}[tests]"
TEST_PYTHON="$(pwd)/.wheeltest/bin/python"
rm -rf /tmp/spharpy-wheel-tests
mkdir -p /tmp/spharpy-wheel-tests
cp -r tests /tmp/spharpy-wheel-tests/
cd /tmp/spharpy-wheel-tests
"$TEST_PYTHON" -m pytest tests

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is not that easy to read, maybe we can spit this into 3 blocks, sth like create environment, move files, and test. In this way the maintainance would be easier.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think it's plenty easy to read 💩

... I'll update :)

@mberz
Copy link
Copy Markdown
Member Author

mberz commented Mar 31, 2026

I love it. I wonder if we really need to test_source, as test_wheel should also fail in case test_source fails.

In general I agree, my first draft of this did exactly that.
In the end I extended to both so that it's easier to spot where issues are coming from. But you're right, in probably 95% of the cases there's not really a difference. It's more the 5% might be harder to figure out why they actually fail.

at: /tmp/workspace
- run:
name: install dependencies
command: pip install ".[deploy]"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see your point. My thinking here is that this job is only invoking twine, which doesn’t depend on our project’s dependencies or anything that could vary with the package—it’s just a standalone tool.
We could compare it to ruff, but in that case we pin a specific version and install it via the project configuration. Imo this makes sence here, but we dont have this requriedment for twine.
If we want to change the dependency of twine we also need to change this workflow here.
Right now, installing all packages takes about 29 seconds in CI. Installing only twine should be significantly faster. This also scales better, since this job will run for every package on each pull request.

Comment on lines 185 to 187
- run:
name: Install System Dependencies
command: sudo apt-get update && sudo apt-get install -y libsndfile1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
- run:
name: Install System Dependencies
command: sudo apt-get update && sudo apt-get install -y libsndfile1

I think this is nor required here, isnt it?

- test_source

- test_wheel:
matrix:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

to reduce redandency we could also jsut run test_wheel on main and develop and not for each pull in a pr

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll update that at the end, to make sure it still triggers as long as work on this PR is ongoing

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

Labels

Projects

Status: Require review

Development

Successfully merging this pull request may close these issues.

MAINT: Tests against binary distributions missing

3 participants