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

Migration to GitHub actions and readthedocs #108

Merged
merged 61 commits into from
Oct 27, 2020
Merged

Conversation

sauln
Copy link
Member

@sauln sauln commented Oct 10, 2020

  • Use GitHub actions for cross-platform testing so we can drop travis and appveyor for testing on linux, mac, and windows.
  • Deploy script to auto deploy the package when a new release is cut. (currently deploying to testpypi: https://test.pypi.org/project/ripser/#files)
    • Linux build
    • Windows build
  • Readthedocs config so docs get automatically built. (https://ripserpy.readthedocs.io/en/saul-new-deploy-process/)
  • Update custom domains to point to readthedocs this will have to happen later and will not be part of the PR process

@ctralie can you confirm the files uploaded to pypi (https://test.pypi.org/project/ripser/#files) are all the one's we need?

@codecov
Copy link

codecov bot commented Oct 10, 2020

Codecov Report

Merging #108 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #108   +/-   ##
=======================================
  Coverage   96.75%   96.75%           
=======================================
  Files           3        3           
  Lines         154      154           
  Branches       26       26           
=======================================
  Hits          149      149           
  Misses          4        4           
  Partials        1        1           
Impacted Files Coverage Δ
ripser/_version.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 258ad0a...3933463. Read the comment docs.

@sauln sauln requested a review from bdice October 10, 2020 21:13
@sauln sauln requested a review from ctralie October 10, 2020 21:47
Copy link
Collaborator

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Hey @sauln I saw you've been pushing on this -- I have some comments that might be helpful. Also you might want to use pyproject.toml for this repository since Cython is a requirement of setup.py. You can see an example here for freud, which has similar requirements: https://github.com/glotzerlab/freud/blob/master/pyproject.toml

pyproject.toml is discussed further here: https://www.python.org/dev/peps/pep-0518/

.github/workflows/python-app.yml Outdated Show resolved Hide resolved
.github/workflows/python-publish.yml Outdated Show resolved Hide resolved
TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }}
TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }}
run: |
python setup.py sdist
Copy link
Collaborator

Choose a reason for hiding this comment

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

If possible, I would try to release source distributions and wheel distributions at the same time. I think you could generate source dists and wheels on one platform (e.g. Linux) and then only upload wheels for Windows/other platforms. Here are some example scripts that might be useful.

https://github.com/glotzerlab/freud/blob/master/.circleci/deploy-linux.sh
https://github.com/glotzerlab/signac/blob/master/.circleci/deploy.bash

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like pypi doesn't support wheels for linux without using the manylinux package.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, there are compatibility requirements to make things work on the majority of Linux distros. You can use one of the manylinux container images for deployment if you want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Previous versions didn't have linux wheels, so I'm fine passing on it right now.

@sauln sauln requested a review from bdice October 11, 2020 20:38
Copy link
Collaborator

@bdice bdice left a comment

Choose a reason for hiding this comment

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

@sauln Looks good to me!

@sauln sauln merged commit fc90fb9 into master Oct 27, 2020
@sauln sauln deleted the saul/new-deploy-process branch October 27, 2020 17: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.

2 participants