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

CI: update github action workflow after sphinx-theme-builder switch, remove Python 3.6 #519

Merged
merged 6 commits into from
Dec 3, 2021

Conversation

jorisvandenbossche
Copy link
Member

Some various updates (xref #514 (comment))

Comment on lines 37 to 43
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-
Copy link
Contributor

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

Copy link
Member Author

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"
Copy link
Contributor

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. 🤷🏽

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep good idea

Copy link
Member Author

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

Copy link
Collaborator

@choldgraf choldgraf left a 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.

@choldgraf
Copy link
Collaborator

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 :-)

@choldgraf choldgraf merged commit cdf72d7 into pydata:master Dec 3, 2021
@choldgraf choldgraf changed the title CI: update github action workflow after sphinx-theme-builder switch CI: update github action workflow after sphinx-theme-builder switch, remove Python 3.6 Dec 3, 2021
@jorisvandenbossche jorisvandenbossche deleted the stb-follow-ups branch December 3, 2021 20:50
@@ -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"]
Copy link
Collaborator

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
Copy link
Collaborator

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?

Copy link
Contributor

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. :)

https://github.com/pradyunsg/sphinx-theme-builder/blob/main/src/sphinx_theme_builder/_internal/cli/package.py

Copy link
Collaborator

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!

@damianavila
Copy link
Collaborator

damianavila commented Dec 3, 2021

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 😜 ).

@choldgraf
Copy link
Collaborator

@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.

@damianavila
Copy link
Collaborator

maybe we should open up an issue that tracks the major things we want to clean up?

Yep, that makes sense to me 😉 .

@jorisvandenbossche
Copy link
Member Author

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) :)

@damianavila
Copy link
Collaborator

maybe we should open up an issue that tracks the major things we want to clean up?

Btw, I opened a new issue to track improvements at #525.

@jorisvandenbossche
Copy link
Member Author

Thanks!

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.

4 participants