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

Add GH-Action-based CI #5

Merged
merged 17 commits into from
May 28, 2024
Merged

Add GH-Action-based CI #5

merged 17 commits into from
May 28, 2024

Conversation

j-s-ashley
Copy link
Owner

@j-s-ashley j-s-ashley commented May 24, 2024

This is the implementation using python -m build .. Resolves Issue #3.

@j-s-ashley j-s-ashley linked an issue May 24, 2024 that may be closed by this pull request
@j-s-ashley
Copy link
Owner Author

This does still upload the wheels, but I can take that out pretty easily.

I'm wrapping up for the day, but I plan to toy with this more on the weekend. Should I remove the "upload wheels" part?

Either way, I'll look to get started on implementing cibuildwheel in place of the Python build module!


- run: python -m pip install build

- run: python -m build .
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- run: python -m build .
- run: python -m build --wheel

(. optional)

You are already building the SDist above, so no need to repeat it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Though admittedly, you only upload the wheel in the next line, and build will make the SDist and then build the wheel from it rather than directly making the wheel, so this might not be a good suggestion to take. If you had broken SDists your version would catch it, and mine wouldn't.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a warning (you did it correctly here, but in case it ever matters), GitHub Actions will actually corrupt the binaries if you download several artifacts with any matching files. It's an annoying bug, as you have to make sure not to upload SDists from more than one job or wheels that match.

Very annoying bug to track down. ;)

Copy link
Contributor

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

Looking good. Just some suggestions.

You might also try to "squash and merge" this PR when you merge it in eventually, as it is atomic in scope and so a good candidate for that.

Comment on lines 10 to 52
make_sdist:
name: Make SDist
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4

- uses: actions/setup-python@v5
with:
python-version: "3.12"

- run: python -m pip install build

- run: python -m build .

- name: Upload SDist
uses: actions/upload-artifact@v4
with:
name: example
path: dist/*.tar.gz

build_wheels:
name: Wheel on ${{ matrix.os }}
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, windows-latest, macos-latest]
steps:
- uses: actions/checkout@v4

- uses: actions/setup-python@v5
with:
python-version: "3.12"

- run: python -m pip install build

- run: python -m build .

- name: Upload Wheels
uses: actions/upload-artifact@v4
with:
name: example-wheel-${{ matrix.os }}
path: dist/*.whl
Copy link
Contributor

@matthewfeickert matthewfeickert May 24, 2024

Choose a reason for hiding this comment

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

These can be combined into a single job and then you can just have a condition if statement on the upload of the sdist that checks to see if the OS is ubuntu-latest and if so then uploads the sdist.

Copy link
Contributor

Choose a reason for hiding this comment

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

Multiple jobs matches the future structure better, though. For a pure Python job you'd just have one ubuntu job that builds the wheel and SDist, and one upload. But for binaries you usually split it out rather than trying to just pick one to make the SDist on.

Copy link
Owner Author

Choose a reason for hiding this comment

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

So... should I combine them into one job? Or leave it as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Keeping them as 2 jobs is fine.

.github/workflows/main.yml Outdated Show resolved Hide resolved
j-s-ashley and others added 3 commits May 25, 2024 07:19
…low_dispath

Co-authored-by: Matthew Feickert <matthew.feickert@cern.ch>
Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
@matthewfeickert
Copy link
Contributor

As learning how to use cibuildwheel is a small task unto itself I would have kept the scope of this to just Issue #3 (so f6760e1) and then built off of that in a seperate PR to add cibuildwheel support to build wheels that are distributable.

@j-s-ashley
Copy link
Owner Author

As learning how to use cibuildwheel is a small task unto itself I would have kept the scope of this to just Issue #3 (so f6760e1) and then built off of that in a seperate PR to add cibuildwheel support to build wheels that are distributable.

Fixing that now!

@j-s-ashley j-s-ashley marked this pull request as ready for review May 25, 2024 19:10
Copy link
Contributor

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

I'd suggest squashing and merging this but instead of just taking every commit message that is in the squashed commit I'd write a single commit message in the text box before merge that summarizes what is done here.

@j-s-ashley j-s-ashley merged commit 1916382 into main May 28, 2024
8 checks passed
@j-s-ashley j-s-ashley deleted the add_GHA_based_CI branch May 28, 2024 19:41
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.

Add GitHub Actions based CI workflow to build the project
3 participants