Skip to content

Conversation

@hugovk
Copy link
Member

@hugovk hugovk commented Jan 20, 2023

I wanted to bump 3.11-dev to 3.12-dev on the CI, but https://github.com/lkiesow/python-feedgen depends on https://lxml.de/, which is difficult to build from source, even for Ubuntu.

We're writing out fairly simple RSS, so we can remove the feedgen/lxml dependencies, and it takes less code to do so.

In addition to testing 3.12-dev on all operating systems, this means we can also now test 3.11/Windows.


I also updated the generate_rss.py script to take an optional output directory (like build.py does), so we can also generate RSS on the Read the Docs previews.


And let's bump RTD to use 3.11 instead of 3.10 for build previews.

@hugovk hugovk added the infra Core infrastructure for building and rendering PEPs label Jan 20, 2023
@hugovk hugovk requested review from a team, AA-Turner and CAM-Gerlach as code owners January 20, 2023 22:57
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM otherwise (I'd been meaning to update the CI Python versions, but hadn't quite gotten around to it yet).

One thing, though, that's bothered me for a while. It would likely be confusing and frustrating for PEP authors and contributors if one of our CI check jobs run on all PRs and our main branch suddenly started failing due to a change in an alpha Python version outside of their control. Furthermore, it also means that we're executing (or at least a slightly hacky skips on) several unnecessary steps (clean up, deploy, purge CDN cache) twice, and we're also running both jobs on every single PR (as opposed to e.g. just those that actually touch infra/rendering code). Conversely, it means we're not actually checking that the PEPs can actually be build on the full range of Python versions and platforms we nominally support, only Linux with the very latest stable and development Python versions.

To avoid all of this, what about we remove the 3.12-dev job from the render workflow matrix, as well as the matrix.python-version check from the Deploy step, and instead add a Render PEPs step to the test workflow, running python -b -X dev -W error build.py (for cross platform support and warning checks)? This means:

  • We ensure PEP rendering works on all supported Python versions and platforms
  • The CI checks on PEP author/contributor facing PRs don't suddenly start failing due to an alpha Python version issue unrelated to their change
  • We're not running extra testing jobs on changes that actually touch any infra (we should also expand the paths to cover more relevant files, too, like requirements.txt, build.py, conf.py, generate_rss.py, and pytest.ini)
  • The CIs on infra PRs will detect and report Python-level warnings (Deprecation, etc), as is considered a best practice for CI runs.

Or, in hindsight, perhaps best that I make this a separate followup PR?

@JelleZijlstra
Copy link
Member

I feel we use dev Python in order to be good citizens and help catch regressions in main faster. It's true that this could confuse some contributors, but this repo's contributors tend to be rather sophisticated Python users, so I think the tradeoff is probably worth it.

@CAM-Gerlach
Copy link
Member

Well, perhaps, but we're still touching some aspects of infra semi-regularly so any issue would still get found relatively promptly, considering that the 3.12-dev version just follows the released alphas, which have a ≈monthly cadence.

If we want more certainty than that, a much better solution than adding it to the PEP content checks on every commit is to just run a GitHub Action against main at regular intervals (e.g. every week) running on 3.12-dev, in addition to running against every commit of any PR/main that touches any infra. That ensures we're checking regularly against the actual thing that's meaningfully changing for these purposes (3.12-dev, rather than PEP content), instead of just whenever someone happens to push to a PR, and should notify all of us subscribed to the repo immediately rather than the PR author if it does fail. It also avoids the checks continually failing on main and on authors' PRs for something that they (and, generally, this repo) has no direct control over, and may not resolve itself for a month or more.

Also, even if were to not remove it from the render workflow, the other changes mentioned besides removing it from this workflow are all independently worthwhile, as they provide more thorough testing of both our own code and the Python versions, including the development one(s), on all platforms instead of just one, and checking for warnings (as is recommended practice for good open source citizens) instead of just hard errors.

@hugovk
Copy link
Member Author

hugovk commented Jan 21, 2023

Ah, I think your main point is that render.yml is primarily a deploy workflow, and we can more fully exercise its "Render PEPs" step by adding it to test.yml and get the full Python/OS matrix. And do render.yml with a single stable 3.x version.

That sounds fine to me.

About the paths, I'd probably remove them from test.yml so we check for any change, including PEP file changes.

And I don't think we need a cron.

Or, in hindsight, perhaps best that I make this a separate followup PR?

Yeah, I think this would be good as a followup, and I'll merge this.

Thank you!

@hugovk hugovk merged commit b173099 into python:main Jan 21, 2023
@hugovk hugovk deleted the rm-feedgen branch January 21, 2023 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

infra Core infrastructure for building and rendering PEPs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants