Skip to content

Conversation

sentrivana
Copy link
Contributor

@sentrivana sentrivana commented Oct 7, 2025

Description

Removing the Check CI Config step altogether as well as associated parts of the toxgen script (fail_on_changes). Added a BIG ALL CAPS WARNING to tox.ini instead. Also updated the toxgen readme a bit.

Removing the check should be fine because we haven't actually seen cases of people trying to edit tox.ini directly -- if this happens in the future it's easy to notice in the PR. If we don't notice it then, we can notice it during the weekly toxgen update. And if don't notice it then, the file simply gets overwritten. 🤷🏻‍♀️

The Problem With Checking tox.ini: The Long Read

In order to check manual changes to tox.ini on a PR, we hash the committed file, then run toxgen, hash the result, and compare. If the hashes differ, we fail the check. This works fine as long as there have been no new releases between the two points in time when tox.ini was last committed and when we ran the check.

This is usually not the case. There are new releases all the time. When we then rerun toxgen, the resulting tox.ini is different from the committed one because it contains the new releases. So the hashes are different without any manual changes to the file.

One solution to this is always saving the timestamp of the last time tox.ini was generated, and then when rerunning toxgen for the purposes of the check, ignoring all new releases past the timestamp. This means any changes we detect were actually made by the user.

However, the explicit timestamp is prone to merge conflicts. Anytime master has had a toxgen update, and a PR is made that also ran toxgen, the PR will have a merge conflict on the timestamp field that needs to be sorted out manually. This is annoying and unnecessary.

(An attempt was made to use an implicit timestamp instead in the form of the commit timestamp, but this doesn't work since we squash commits on master, so the timestamp of the last commit that touched tox.ini is actually much later than the change was made. There are also other problems, like someone running toxgen but committing the change much later, etc.)

Solutions considered

  • using a custom merge driver to resolve the timestamp conflict automatically (doesn't work on GH PRs)
  • running toxgen in CI on each PR and committing the change (would work but we're essentially already doing this with the cron job every week)
  • not checking in tox.ini at all, but running toxgen on each PR (introduces new package releases unrelated to the PR, no test setup committed -- contributors and package index maintainers also need to run our tests)
  • finding a different commit to use as the implicit timestamp (doesn't work because we squash commits on master)
  • ...

In the end I decided to just get rid of the check. If people modifying tox.ini manually becomes a problem, we can deal with it then. I've added a big warning to tox.ini to discourage this.

Issues

Closes #4886

Reminders

@sentrivana sentrivana marked this pull request as ready for review October 7, 2025 10:22
@sentrivana sentrivana requested a review from a team as a code owner October 7, 2025 10:22
cursor[bot]

This comment was marked as outdated.

@sentrivana sentrivana merged commit a879d82 into master Oct 8, 2025
112 checks passed
@sentrivana sentrivana deleted the ivana/toxgen/remove-check branch October 8, 2025 06:32
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.

Toxgen false-positive change detected

2 participants