Clean up check.sh, move stuff to pre-commit #3157

jakkdl · 2024-12-17T11:51:46Z

Ruff, Black, Codespell and ~~gen-exports~~ are all run in pre-commit, so there's no need to also run them in check.sh
~~moving uv pip-compile to pre-commit was easy~~

The only remaining tools in check.sh now are:

pyright pyright+pre-commit+CI does not play well, as pyright ~requires an internet connection and pre-commit in CI isolates the environment not to have it. In flake8-async we have a dedicated CI action for pyright that uses an unofficial github action.
EDIT: Though we also have check_type_completeness.py that won't play well with that pyright-action.
mypy I think doing 3 full runs (all platforms) would be overly slow for local development, but I'm not sure it's possible to specify manual stages to run with the pre-commit github action. So best solution here is perhaps to configure a mypy pre-commit hook (using current system?) and once again add a dedicated CI action.

Addresses some of #2699

…re now run in pre-commit

codecov · 2024-12-17T11:53:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00000%. Comparing base (92b0480) to head (2bc884c).

Additional details and impacted files

@@               Coverage Diff               @@
##                 main        #3157   +/-   ##
===============================================
  Coverage   100.00000%   100.00000%           
===============================================
  Files             124          124           
  Lines           18427        18427           
  Branches         1215         1215           
===============================================
  Hits            18427        18427

…ng deps

jakkdl · 2024-12-17T13:17:48Z

ah right, of course pip-compile also requires internet. There's maybe a configuration with repo: local and language: system but eh. Unclear if it's worth running locally

A5rocks · 2024-12-17T20:27:54Z

check.sh

-echo "::group::Generate Exports"
-python ./src/trio/_tools/gen_exports.py --test \
-    || EXIT_STATUS=$?
-echo "::endgroup::"


I probably should do this in another PR but I think we should move gen exports out of pre-commit and back here. Unfortunately, as things currently are, if you modify a file that the pre-commit hook says it uses then commit with a git client (ie not just git commit -m "..." in the venv you have for trio), gen_exports doesn't get the dependencies it needs.

So far I've been getting that error then going to my terminal to run git there, but that's a bit much.

Pre-commit with a external git client doesn't install required dependencies?

Nope, because we don't specify them. We don't specify them because pre-commit doesn't let you install from a requirements file :(

I specified them in the latest commit:

trio/.pre-commit-config.yaml

Line 46 in 17d3d05

additional_dependencies: ["astor", "attrs", "black", "ruff"]

but that doesn't let us pin the versions (in a non-awful way), so yeah if we don't want that then it has to go outside of pre-commit

trio/.pre-commit-config.yaml

Line 46 in 17d3d05

additional_dependencies: ["astor", "attrs", "black", "ruff"]

Keep in mind that the pre-commit cache is virtually immortal. You can clean it locally, but not in the pre-commit.ci service. For that, changing the deps list or the hook version is required. This may be problematic sometimes.

So I've found that the best thing to do here is to keep the version pins in the list and bump them periodically. Just so that the cache doesn't grow too old.

I think that means we need to keep it in check.sh and skip it in ci for now. I'll keep the changes to the pre-commit hook to alleviate @A5rocks's problem, and maybe use tox or something later on.

Tox sounds wonderful. I've been meaning to suggest it. I've actually been working on some reusable workflows for tox-based projects + experimented with some cool GHA integrations.
When you get to it, I may offer some help / experimentation in simplifying things here.

Yeah I'm personally a big fan of tox. #2790 discussed hatchling too but we ended up bikeshedding too much to actually do anything in the end.

A5rocks · 2024-12-17T20:29:02Z

check.sh

@@ -94,8 +56,6 @@ if git status --porcelain | grep -q "requirements.txt"; then
    echo "::endgroup::"
 fi

-codespell || EXIT_STATUS=$?


(not the right line because github isn't letting me comment there)

Should you remove the uv pip compile lines above? And also update the error message + add a pre-commit run -a line in check.sh so people can run check.sh locally? (I'm not sure people do run check.sh locally but...)

Realized we can't do uv pip compile in pre-commit in CI since it needs internet, so it needs to be here (or somewhere).

My goal is not to require/expect people to run check.sh locally as it's unintuitive and clunky, but given the limitations of pre-commit we do need to stash these somewhere and make it easy for users to run them locally if they want.
I'd personally vote for tox (due to familiarity), and set up environments for the different tests.

I usually wrap the pre-commit invocation in tox for local run anyway + run it in CI too, which covers the case of the service not having the internet during testing.

CoolCat467

Moving things to pre-commit looks great, just wondering about how this will work with CI. Will CI check to make sure pre-commit run succeeds before allowing merge, or do we have to change what is required in the "all greens" workflow?

.pre-commit-config.yaml

jakkdl · 2024-12-22T11:57:23Z

[once again registering my displeasure with not being able to squash on merge]
I'll manually squash and force push after giving y'all a chance to review, since force pushing breaks review history on github.

I've scaled back the PR to:

remove redundant formatting/linting from check.sh (although I'm not sure if pre-commit is 'required' to merge? so not strictly 100% redundant)
change pre-commit entry for regenerate-files to have dependencies instead of relying on the local environment having them.
Add pip-compile checks to pre-commit, which are skipped in CI.

webknjaz · 2024-12-23T02:10:32Z

remove redundant formatting/linting from check.sh (although I'm not sure if pre-commit is 'required' to merge? so not strictly 100% redundant)

No, it's not required, but I'd ask @A5rocks to make it mandatory in the ruleset.

Add pip-compile checks to pre-commit, which are skipped in CI.

To address such cases, I like duplicating the pre-commit invocation in conventional CI, which I'd recommend here as well. If you want to, you can select/skip some checks to make sure that only those that can't run in the pre-commit.ci platform would run on GHA. But you can also not care and run everything additionally — it's usually inexpensive.

graingert · 2024-12-23T10:17:10Z

check.sh

@@ -79,8 +67,7 @@ Problems were found by static analysis (listed above).
 To fix formatting and see remaining errors, run

    uv pip install -r test-requirements.txt
-    black src/trio
-    ruff check src/trio
+    pre-commit run --all-files


hook stage manual?

?
We don't have any manual hooks anymore

jakkdl · 2024-12-23T10:30:22Z

Hrm, I just noticed this warning

warning: The requested Python version 3.11 is not available; 3.13.1 will be used to build dependencies instead.

https://github.com/python-trio/trio/actions/runs/12465050038/job/34790209503?pr=3157
which is also happening on main/ atm @A5rocks

warning: The requested Python version 3.9 is not available; 3.13.1 will be used to build dependencies instead.

https://github.com/python-trio/trio/actions/runs/12446716072/job/34749424898#step:5:1098

I think the easiest fix will be tox, although it doesn't seem to have mattered so far

(3.9 vs 3.11 is because we compile test-requirements on 3.9 and docs-requirements on 3.11)

jakkdl · 2024-12-23T10:40:16Z

running it through pre-commit I appear to have bumped uv from 5.5 to ~5.11, so I suppose they started sorting the version/platform constraints at some point (though not seeing it from a quick skim of the changelogs https://github.com/astral-sh/uv/releases).

graingert · 2024-12-23T10:58:07Z

Can we use '3.x' for dists and code checks, instead of 3.11?

jakkdl · 2024-12-23T11:04:57Z

Can we use '3.x' for dists and code checks, instead of 3.11?

if the python version doesn't matter for uv pip-compile we could simply opt out of passing --python-version

jakkdl added 2 commits December 17, 2024 12:22

remove ruff, black, codespell, and gen-exports from check.sh as they'…

9b3783f

…re now run in pre-commit

move pip-compile tests to pre-commit

84433f4

jakkdl requested a review from CoolCat467 December 17, 2024 11:51

jakkdl added 2 commits December 17, 2024 14:16

restore pip-compile in check.sh, skip in CI

7557558

make regenerate-files runnable in CI, with the downside of not freezi…

b38faba

…ng deps

fix skip

17d3d05

A5rocks reviewed Dec 17, 2024

View reviewed changes

CoolCat467 reviewed Dec 18, 2024

View reviewed changes

webknjaz reviewed Dec 18, 2024

View reviewed changes

.pre-commit-config.yaml Show resolved Hide resolved

webknjaz reviewed Dec 18, 2024

View reviewed changes

.pre-commit-config.yaml Show resolved Hide resolved

jakkdl mentioned this pull request Dec 22, 2024

yaml formatting/linting #3164

Open

jakkdl added 2 commits December 22, 2024 12:48

Merge remote-tracking branch 'origin/main' into cleanup_checksh

4633591

restore gen_exports in check.sh, skip in pre-commit+ci

22215d0

make check.sh call pre-commit

b4d08e1

graingert reviewed Dec 23, 2024

View reviewed changes

it seems uv pip-compile is sorting the requirements now

8f787d6

Merge branch 'main' into cleanup_checksh

2bc884c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up check.sh, move stuff to pre-commit #3157

Clean up check.sh, move stuff to pre-commit #3157

jakkdl commented Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

jakkdl commented Dec 17, 2024

A5rocks Dec 17, 2024

CoolCat467 Dec 18, 2024

A5rocks Dec 18, 2024

jakkdl Dec 18, 2024

webknjaz Dec 18, 2024

jakkdl Dec 22, 2024

webknjaz Dec 23, 2024

jakkdl Dec 23, 2024

A5rocks Dec 17, 2024 •

edited

Loading

jakkdl Dec 18, 2024 •

edited

Loading

webknjaz Dec 23, 2024

CoolCat467 left a comment

jakkdl commented Dec 22, 2024 •

edited

Loading

webknjaz commented Dec 23, 2024

graingert Dec 23, 2024

jakkdl Dec 23, 2024

jakkdl commented Dec 23, 2024 •

edited

Loading

jakkdl commented Dec 23, 2024

graingert commented Dec 23, 2024 •

edited

Loading

jakkdl commented Dec 23, 2024

Clean up check.sh, move stuff to pre-commit #3157

Are you sure you want to change the base?

Clean up check.sh, move stuff to pre-commit #3157

Conversation

jakkdl commented Dec 17, 2024 • edited Loading

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

jakkdl commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

A5rocks Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

jakkdl Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoolCat467 left a comment

Choose a reason for hiding this comment

jakkdl commented Dec 22, 2024 • edited Loading

webknjaz commented Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakkdl commented Dec 23, 2024 • edited Loading

jakkdl commented Dec 23, 2024

graingert commented Dec 23, 2024 • edited Loading

jakkdl commented Dec 23, 2024

jakkdl commented Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

A5rocks Dec 17, 2024 •

edited

Loading

jakkdl Dec 18, 2024 •

edited

Loading

jakkdl commented Dec 22, 2024 •

edited

Loading

jakkdl commented Dec 23, 2024 •

edited

Loading

graingert commented Dec 23, 2024 •

edited

Loading