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 build and documentation build workflows. #49

Merged
merged 16 commits into from
May 24, 2021

Conversation

hodgestar
Copy link
Contributor

No description provided.

Copy link
Member

@BoxiLi BoxiLi left a comment

Choose a reason for hiding this comment

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

Thanks, @hodgestar !

I tried the build on my own fork. (https://github.com/BoxiLi/qutip-qip/actions/runs/870993503) Ignoring the deploying part, it looks good. RTD also seems to work fine (https://boxi-qutip-qip.readthedocs.io/en/latest/index.html).

Needs to set the github.secret up seperately.

@BoxiLi
Copy link
Member

BoxiLi commented May 24, 2021

I have set up the PyPI token.

@hodgestar
Copy link
Contributor Author

I have set up the PyPI token.

Nice!

Let's merge. I asked Jake for a review, but if he finds anything we can always fix it in a follow up PR.

@BoxiLi BoxiLi merged commit a1e0b9d into master May 24, 2021
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

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

I had a couple of comments, but nothing I saw looks wrong - just a couple of things I might have done differently if I did a second attempt at the qutip workflows.

Comment on lines +71 to +88
# Zip files are not part of PEP 517, so we need to make our own.
- name: Create zipfile from tarball
shell: bash
working-directory: dist
run: |
# First assert that there is exactly one tarball, and find its name.
shopt -s failglob
tarball_pattern="*.tar.gz"
tarballs=($tarball_pattern)
[[ ${#tarballs[@]} == 1 ]]
tarball="${tarballs[0]}"
# Get the stem and make the zipfile name.
stem="${tarball%.tar.gz}"
zipfile="${stem}.zip"
# Extract the tarball and rezip it.
tar -xzf "$tarball"
zip "$zipfile" -r "$stem"
rm -r "$stem"
Copy link
Member

Choose a reason for hiding this comment

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

qutip-qip probably doesn't need to do this (probably qutip doesn't even need to do this any more...). The only place this is accessible from for the main repo is qutip.org, so if you've not got anything like this for qip, it's probably not necessary.

Comment on lines +129 to +133
- uses: actions/upload-artifact@v2
with:
name: wheels
path: ./wheelhouse/*.whl

Copy link
Member

Choose a reason for hiding this comment

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

You maybe also want to specify if-no-files-found: error on this one as well.

Comment on lines +111 to +113
with:
# This is about the build environment, not the released wheel version.
python-version: '3.7'
Copy link
Member

Choose a reason for hiding this comment

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

Notionally the same argument about minimum Python version applies here as well as on the sdist (i.e. you want to build using the minimum supported version), but really I don't think it matters at all as long as it's done with the lowest in at least one place.

Copy link
Member

Choose a reason for hiding this comment

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

(the reason it doesn't apply here for qutip/qutip is that the build is delegated to cibuildwheel, which then sets up a bunch of docker containers or something with each supported Python version)

Comment on lines +35 to +39
make html SPHINXOPTS="-W --keep-going -T"
# Above flags are:
# -W : turn warnings into errors
# --keep-going : do not stop after the first error
# -T : display a full traceback if a Python exception occurs
Copy link
Member

Choose a reason for hiding this comment

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

I think there's a way to add this into pyproject.toml so that these flags are always done, even for users. I didn't know about it at the time I wrote the qutip/qutip workflow, but if it does exist, it might be an idea to add them there. I think pytest also supports that.

If not pyproject.toml, then you could also change the Sphinx Makefile to put them in there. I don't know why I didn't do that with qutip/qutip - I think that's the right way to do it, and I just got tunnel vision on writing the workflow.

def qutip_qip_version():
""" Retrieve the qutip-qip version from ``../../VERSION``.
"""
src_folder_root = pathlib.Path(__file__).absolute().parent.parent.parent
Copy link
Member

Choose a reason for hiding this comment

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

pathlib also lets you do pathlib.Path.parents[-3], which is a bit simpler.

@jakelishman
Copy link
Member

oh, you already merged it! Haha, no problem - my stuff was just minor comments anyway.

Comment on lines +144 to +148
env:
TWINE_USERNAME: __token__
TWINE_PASSWORD: ${{ secrets.PYPI_TOKEN }}
TWINE_NON_INTERACTIVE: 1
TWINE_REPOSITORY: pypi
Copy link
Member

Choose a reason for hiding this comment

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

Oh, a comment here - I see in the settings of this repo that you've called the token PYPI_API_TOKEN, which is a mismatch with this line.

Also, we should probably move the env specification on to the "Upload sdist and wheels to PyPI" step, not the entire job, just for the purposes of really minimising the time that our secrets are available in the environment. It's not a big deal at all, since only QuTiP maintainers can run this workflow, but still, just seems like the right thing to do (similar on qutip/qutip).

@BoxiLi
Copy link
Member

BoxiLi commented May 24, 2021

oh, you already merged it! Haha, no problem - my stuff was just minor comments anyway.

Oh sorry, haha, didn't know you were reviewing at the very moment. We'll address them in a separate PR :)

BoxiLi added a commit to BoxiLi/qutip-qip that referenced this pull request Jul 11, 2021
Add build and documentation build workflows.
This was referenced Jul 12, 2021
@BoxiLi BoxiLi deleted the feature/add-build-workflows branch November 25, 2021 08:39
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.

None yet

3 participants