-
Notifications
You must be signed in to change notification settings - Fork 328
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
CI: update github action workflow after sphinx-theme-builder switch, remove Python 3.6 #519
Conversation
.github/workflows/tests.yml
Outdated
with: | ||
path: ~/.cache/pip | ||
key: | | ||
${{ runner.os }}-pip-${{ matrix.python-version }}-${{ hashFiles('setup.py', 'docs/requirements.txt') }} | ||
${{ runner.os }}-pip-${{ matrix.python-version }}-${{ hashFiles('pyproject.toml') }} | ||
restore-keys: | | ||
${{ runner.os }}-pip-${{ matrix.python-version }}- | ||
${{ runner.os }}-pip- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're touching this, you can probably drop this whole thing and use setup-python v2 and provide cache: pip
to it. https://github.com/actions/setup-python#caching-packages-dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes! I saw that passing by on twitter :)
pyproject.toml
Outdated
@@ -16,7 +16,7 @@ description = "Bootstrap-based Sphinx theme from the PyData community" | |||
dynamic = ["version"] | |||
readme = "README.md" | |||
|
|||
requires-python = ">=3.5" | |||
requires-python = ">=3.6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump to 3.7+?
3.6 goes EoL in ~30 days. 🤷🏽
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually also don't test it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all seems good to me - we should make sure to track that we've officially dropped 3.6 support in the next release notes.
This is all relatively minor under the hood stuff, improves the codebase, and has a net-negative number of lines, so I'm just gonna merge this one :-) |
@@ -19,9 +19,9 @@ jobs: | |||
runs-on: ubuntu-latest | |||
strategy: | |||
matrix: | |||
python-version: [3.7, 3.8, 3.9] | |||
python-version: [3.7, 3.8, 3.9, "3.10"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we should quote all the versions (that seems to be the "recommendation" from action authors: https://github.com/actions/setup-python#usage).
python -m pip install -U pip setuptools wheel | ||
python setup.py sdist bdist_wheel | ||
python -m pip install -U pip build | ||
python -m build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are using stb now, we should be using stb package
, right?
https://sphinx-theme-builder.readthedocs.io/en/latest/tutorial/#packaging-the-theme
I guess this is a more general question... do we want to use stb commands here or the underlying machinery?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more "correct" since it's portable across whatever build-backend is used here.
Plus, at the cost of sharing how the sausage is made, stb package
is... uhm... a very dumb and thin wrapper around this command anyway. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the answer I needed, thanks for the feedback @pradyunsg!
As a general comment, it seems there is still some old stuff on this workflow file... so the idea to merge this one as quickly was because we wanted to migrate iteratively here as well? I am a little bit concerned with the fact we now have a mix of things (see the audit section) that could be very difficult to interpret for possible contributors. This is not a general pushback (my latest messages could be interpreted in that way), I am just trying to "sync" myself with the usual practices on this repo so I can accommodate myself to the community standards (before actually pushing for some changes 😜 ). |
@damianavila definitely agree, maybe we should open up an issue that tracks the major things we want to clean up? I just didn't want to block an iterative improvement PR because it's not solving the whole problem. |
Yep, that makes sense to me 😉 . |
Yeah, and I intentionally didn't touch the other audit workflows, as initially started this PR as small scoped fix for the things that were actually broken (because of setup.py or requirements.txt files not existing anymore) :) |
Btw, I opened a new issue to track improvements at #525. |
Thanks! |
Some various updates (xref #514 (comment))