Skip to content

[CI] Add lint workflow running pre-commit#16275

Merged
straight-shoota merged 5 commits intocrystal-lang:masterfrom
straight-shoota:infra/ci/devenv
Oct 29, 2025
Merged

[CI] Add lint workflow running pre-commit#16275
straight-shoota merged 5 commits intocrystal-lang:masterfrom
straight-shoota:infra/ci/devenv

Conversation

@straight-shoota
Copy link
Member

This is a follow-up to #16263 which introduced a pre-commit configuration based on devenv.

It adds a new Lint workflow which runs pre-commit in CI. It uses differential testing that only tests files that have changes compared to the base ref of a PR, or master.
Workflow dispatch allows running pre-commit against all files in the repo.

We already have a couple of individual CI job that run some of these linter: lint-actionlint, lint-shellcheck, lint_spellcheck. The new setup replaced them. But I'm keeping them around for a while to ensure the new workflow functions properly.

@ysbaddaden
Copy link
Collaborator

ysbaddaden commented Oct 27, 2025

Nix and devenv may be nice, but they're painfully slow to setup:

  • 30s to install devenv;
  • 45.8s for devenv to build the shell;
  • the tests take less than 7s.

That's over 90% of the runtime spent on setup alone, repeated on every CI run 😱

(Aside: I replace the nix setup with homebrew to shove multiple minutes of setup time when I want to test something on macOS CI).

@straight-shoota
Copy link
Member Author

straight-shoota commented Oct 27, 2025

Yes, the setup overhead is significant. That's unfortunate. But it's also not too bad. It might be possible to speed that up with some optimizations.

Also the total duration is not that different compared to running each check in a separate job.
This workflow currently performs 9 different checks. The existing, individual linter actions (which this should eventually replace) take about 10s each. If we ran all of them we'd be spawning jobs with a total runtime of 90s as well.
This is not directly comparable, because the individual jobs can run in parallel and are only triggered on changes to specific paths.
We pay the nix install overhead regardless of what files the change affects.

But the best value from pre-commit is that it makes it trivial to add more hooks. And it's synced between CI and local (if you use devenv).

The current configuration already contains twice as many as tests as we have currently configured in custom workflows. And I'm planning to add more.

The main benefit of this setup is to avoid the complexity of managing individual CI jobs for different tests and ensuring consistency between local use and CI.

@ysbaddaden
Copy link
Collaborator

ysbaddaden commented Oct 27, 2025

It might be possible to speed that up with some optimizations.

Are you sure? We could use some cache, but nix/devenv quickly installs gigabytes, and "devenv shell" always tries to update and to install more...

@straight-shoota straight-shoota added this to the 1.19.0 milestone Oct 27, 2025
@straight-shoota straight-shoota merged commit 8583a3b into crystal-lang:master Oct 29, 2025
41 checks passed
@straight-shoota straight-shoota deleted the infra/ci/devenv branch October 29, 2025 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants