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

A combo of CI unblocks via PRs #2106 #2134 #2141 #2142 #2146 #2145

Conversation

webknjaz
Copy link
Member

This is necessary to merge as a combo because of how the branch protection is configured.

Contributor checklist
  • Included tests for the changes.
  • PR title is short, clear, and ready to be included in the user-facing changelog.
Maintainer checklist
  • Verified one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

webknjaz and others added 7 commits December 4, 2024 15:58
This is necessary because:
* `pypy-3.8` is EOL
* `pypy-3.8` has flaky SEGFAULTs on import [[1]] [[2]] [[3]] due to a
  bug in their GC that is fixed in `pypy-3.9`

[1]: https://github.com/jazzband/pip-tools/actions/runs/12162197242/job/33918558133?pr=2106#step:8:59
[2]: pytest-dev/pytest#11771 (comment)
[3]: https://pypy.org/posts/2024/03/fixing-bug-incremental-gc.html
This better reflects that the pip version being pulled in is the
lowest tested/supported.
This is meant to be separate from "latest" and is pinned to the known
working released version of pip. It will run on merges to `main` and
in pull requests. And the cron runs will additionally test the latest
version on PyPI and the `main` branch of the pip repo.
This fixes a bug that build deps compilation would get the latest
version of an unconstrained build requirements list, not taking into
account the restricted/requested one.

The regression test is implemented against `setuptools < 70.1.0` which
is known to inject a dependency on `wheel` (newer `setuptools` vendor
it). The idea is that `pyproject.toml` does not have an upper bound for
`setuptools` but the CLI arg does. And when this works correctly, the
`wheel` entry will be included into the resolved output.

Cleaning up `PIP_CONSTRAINT` is implemented manually due to the corner
case of a permission error on Windows when accessing a file that we
hold a file descriptor to from another subprocess[[1]]. It can be
further simplified once the lowest Python version `pip-tools` supports
is 3.12 by replacing `delete=False` with `delete_on_close=False` in the
`tempfile.NamedTemporaryFile()` context manager initializer.

[1]: https://docs.python.org/3/library/tempfile.html#tempfile.NamedTemporaryFile

Co-authored-by: Sviatoslav Sydorenko <[email protected]>
The `pre-commit.ci` service dropped support for Python 3.8 as it's
gone EOL. This change makes the check runnable again. It is suboptimal
as we haven't yet dropped support for Python 3.8. jazzband#2144 will address
the underlying issue more thoroughly.

Resolves jazzband#2133.

Co-Authored-By: Sviatoslav Sydorenko <[email protected]>
@webknjaz webknjaz added bug Something is not working tests Testing and related things maintenance Related to maintenance processes skip-changelog Avoid listing in changelog labels Dec 12, 2024
@webknjaz webknjaz requested review from hugovk and chrysle December 12, 2024 09:29
@webknjaz webknjaz enabled auto-merge December 12, 2024 09:29
@webknjaz webknjaz force-pushed the maintenance/reproducible-ci-fixes-pr2106-pr2141-pr2142 branch from 2741e7c to 51ec5ed Compare December 12, 2024 09:33
@webknjaz webknjaz changed the title A combination of changes to unblock CI via PRs #2106 #2141 #2142 A combo of changes to unblock CI via PRs #2106 #2134 #2141 #2142 Dec 12, 2024
@webknjaz
Copy link
Member Author

webknjaz commented Dec 12, 2024

@hugovk @chrysle @Mr-Sunglasses @macro1 as soon as somebody approves this, everybody should get unblocked. This contains a true/natural merge commit of individual branches in 4 PRs. They are the same commits (hashes and everything) so when merging this, GitHub is supposed to auto-detect that it can mark those other PRs as merged automatically.

Copy link
Member

@WhyNotHugo WhyNotHugo left a comment

Choose a reason for hiding this comment

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

The parts which I can understand look good to me.

Notably, I have no idea what fac71fc does.

51ec5ed seems to be an internal merge commit between your branches which duplicates al changes? I think that rebasing will making it disappear entirely, but I didn't check all of it for any discrepancies.

@webknjaz
Copy link
Member Author

webknjaz commented Dec 12, 2024

Notably, I have no idea what fac71fc does.

Basically, it turned out that the variable we relied on was always unavailable and so CI was always evaluating it the same way.

51ec5ed seems to be an internal merge commit between your branches which duplicates al changes? I think that rebasing will making it disappear entirely, but I didn't check all of it for any discrepancies.

No, merge commits don't contain duplicates by their nature. We mustn't rebase, this is done on purpose so that the actual flow of history is preserved. If you rebase, you'll basically disregard the original commits, replacing them with copies in a weird order and no link to reality. This is an intentional “octopus” merge in hopes that the merge queues mechanism would just fast-forward main to it.

@webknjaz
Copy link
Member Author

The PyPy failure is rather weird. I saw similar problems on Windows but CPython. And I thought that was gone. I'll have to debug this last thing.

@webknjaz
Copy link
Member Author

The PyPy failure is rather weird. I saw similar problems on Windows but CPython. And I thought that was gone. I'll have to debug this last thing.

So this is actually a bug in PyPy. Also seen in tox: tox-dev/tox#3284.

PyPy maintainers recommend testing under PyPy 3.10 instead: pypy/pypy#4958 (comment). So I'm going to add another PR with that bump.

This is necessary because:
* `pypy-3.8` is EOL
* `pypy-3.8` has flaky SEGFAULTs on import [[1]] [[2]] [[3]] due to a
  bug in their GC that is fixed in `pypy-3.9`
* `pypy-3.9` has a bug that does not have a backport with the fix [[4]].
  Furthermore, PyPy maintainers recommend using 3.10 [[5]].

[1]: https://github.com/jazzband/pip-tools/actions/runs/12162197242/job/33918558133?pr=2106#step:8:59
[2]: pytest-dev/pytest#11771 (comment)
[3]: https://pypy.org/posts/2024/03/fixing-bug-incremental-gc.html
[4]: tox-dev/tox#3284
[5]: pypy/pypy#4958 (comment)
@webknjaz webknjaz changed the title A combo of changes to unblock CI via PRs #2106 #2134 #2141 #2142 A combo of CI unblocks via PRs #2106 #2134 #2141 #2142 #2146 Dec 16, 2024
@webknjaz webknjaz force-pushed the maintenance/reproducible-ci-fixes-pr2106-pr2141-pr2142 branch from 51ec5ed to fb0f69b Compare December 16, 2024 03:10
@webknjaz
Copy link
Member Author

I was considering pinning to pypy3.9-v7.3.14 like build did: pypa/build@a44675b (#778).

But this implies that pypy-3.9 is EOL so I won't bother: pypy/pypy#4958 (comment).

@webknjaz webknjaz added this pull request to the merge queue Dec 16, 2024
@webknjaz
Copy link
Member Author

in hopes that the merge queues mechanism would just fast-forward main to it.

Nope, it still created another merge commit. Though, it's fine with me since it still is supposed to work, backtracking the PRs to mark as merged.

Merged via the queue into jazzband:main with commit e604dec Dec 16, 2024
33 checks passed
@WhyNotHugo
Copy link
Member

Thanks for picking up this one and stabilising CI! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working maintenance Related to maintenance processes skip-changelog Avoid listing in changelog tests Testing and related things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants