build(lint): consolidate linters under pre-commit

[MichaelYochpaz]

This PR migrates all linting tools to pre-commit for unified local and CI execution.

This allows developers to run hatch run lint:precommit (or pre-commit run) to check for lint issues locally and auto-fix them locally, or even install git pre-commit hooks that will auto-fix them before making a commit.

Notes:

• Super-Linter which had Markdown linting (already covered by markdownlint) and editorconfig validation (now covered by editorconfig-checker) was removed.
• The .markdownlint-config.yaml config file was renamed to .markdownlint.yaml to follow the official naming convention, which is auto-used by the CLI tool used (reference). Beforehand the name was a "made up" name that was referenced by using --config .markdownlint-config.yaml.
• markdownlint required Node.js to work (we install it in the CI beforehand). The pre-commit implementation here makes so that it's required on the CI, but for local runs the markdownlint check is skipped (with a warning) if Node.js isn't installed.
• There were some unrelated files that were slightly modified. Those are files that were picked up by the pre-commit and fixed. If not updated (or more accurately, fixed to follow or linting rules), they would change whenever pre-commit is ran.

[tiran]

FWIW, I'm not a fan of pre-commit. I prefer to have linters and tests in the same automation tool, so I can run all checks in parallel from the same tool with a single command. I'm still maintaining my own tox.ini for Fromager and run ruff, linters, doc builds, and unit tests in parallel with tox p.

[MichaelYochpaz]

I prefer to have linters and tests in the same automation tool, so I can run all checks in parallel from the same tool with a single command.

@tiran This is what pre-commit does... You don't have to install the hooks, you can run pre-commit run and it will run all configured tests and linters with a single command.

[LalatenduMohanty]

Pre-commit is a significant change in developer workflow, we can skip the pre-commit failures in the local workflow incase we want to focus on the code chnages initially i.e. git commit --no-verify -m "wip: work in progress". However I am not sure if we want to adopt this workflow. I need to use this workflow and findout more.

Developer Workflow WITHOUT Pre-commit:
┌──────────┐    ┌──────────┐    ┌──────────┐    ┌──────────┐
│  Write   │ ─► │  Commit  │ ─► │   Push   │ ─► │ CI Fails │ 😞
│  Code    │    │ (no check)│   │          │    │ (lint!)  │
└──────────┘    └──────────┘    └──────────┘    └──────────┘

Developer Workflow WITH Pre-commit:
┌──────────┐    ┌──────────┐    ┌──────────┐    ┌──────────┐
│  Write   │ ─► │  Commit  │ ─► │   Push   │ ─► │ CI Pass  │ ✅
│  Code    │    │ BLOCKED! │    │ (clean)  │    │          │
└──────────┘    └──────────┘    └──────────┘    └──────────┘
                     │
                Fix & retry

[MichaelYochpaz]

Just to make sure it's clear - you are not required to install pre-commit as a git hook. It's opt-in and you have to install the hook manually locally by running pre-commit install. Only then it run automatically before every commit, autofix issues, and if there are still issues remain, block you from commiting with an error message.

Another way to use it (which is what I use, and is written in the README) is just by running it manually with pre-commit run (or with hatch run lint:precommit in our repo). Just like how you'd run pytest or black or any other linter manually with a command. It just allows to unify them all under a single command that makes it easy and automatic for developers and contributors.

[tiran]

We need a compelling reason why we should introduce yet another automation tool to run linters. pre-commit is a new tool that we have not used in any project. We would need to train all engineers on the new tool. If hatch is not sufficient for our needs, then I would rather go back to tox (or maybe look into nox).

Also I'm worried about 321 insertions(+), 116 deletions(-). That's 200 more lines to accomplish the same task.

• Fromager was using tox until @LalatenduMohanty switched us to hatch in PR Start using hatch instead of tox #627
• https://github.com/python-wheel-build/elfdeps/ from this organization uses tox
• internally we are using tox in several places like the internal wheel builder repo
• no internal or public project uses pre-commit, yet

[MichaelYochpaz]

@tiran We already have pre-commit used here on Fromager. @dhellmann added it to the repo (Link #1, Link #2).
You can see I'm just modifying the existing pre-commit config file and not creating a new one.

I'm just consolidating the linters under the same tool, and making it easier to run the linters we only have on the CI, locally.
Running pre-commit and having the official linters fix linting issues for you locally is much more convenient over pushing, waiting for the CI to run, reading the failures, and manually fixing them.

I don't think line counts should be a metric. I've used a lot of comments and updated the docs/develop.md document to be more informative. I can easily deflate that number if that was a concern for some reason.

What training would developers need? It's literally just a single command to run.
If anything, using tox would require a lot more learning and training from developers.

[dhellmann]

I like the idea of this change as a way to standardize the tool configuration. When we introduced pre-commit we only brought some of the tests in, and this makes it easier to ensure they are all run. I find that very useful when working with agents that generate code, since you can't always get them to run the linter or unit tests but you can always get them to run the pre-commit hooks when committing changes.

The markdownlint stuff is a little confusing, mostly because I don't remember how that tool is installed. Is there a way to run it via an image or to configure npm to install the right thing when it's used locally?

The only other comment is that I might have staged the changes in multiple commits to make it easier to understand what each update is doing. It's not worth splitting this up into multiple commits, now, though.

[dhellmann]

Where does this hook get the linter?

[MichaelYochpaz]

@dhellmann Yeah this allows a full sync between linters running locally and on our CI.
This is good for AI agents (they can run the full linting and unit-testing process locally with a single command and not a shopping list of different tools to run sequentially), but also humans.
I opened this after I got a long list of Markdown list errors that just referred to specific lines that I had to manually find and fix. Now we can auto-fix these locally in seconds before even pushing.

To answer your question about markdownlint: It requires Node.js to run (we install Node in our CI).
This is tricky because I want pre-commit to also work for users who don't have Node.js installed.
We could make it an optional step, but then there wouldn't be a clear message for users to know they're missing out on this linter / how to fix it (they won't know they need to install Node.js), and also, the CI (which runs the same pre-commit) would skip this lint step in case of an error because it's optional.

I thought of using containers, but than we would just require Docker / Podman instead, and it would only work with one of them (e.g., if write it as a Podman command, it won't work for Docker users). Thought it's probably too much maintenance and overhead, with many possible issues and edge cases.

The solution I came up with (which also answers your review note) is to use a script - scripts/conditional_hook.py. This script will always run on pre-commit and is required. Then inside the script, if Node.js isn't installed, there are two cases:

1. If running on the CI (detected using environment variables) - running markdownlint is required and the script will fail if Node.js is missing.
2. Otherwise (running locally) - if Node.js isn't installed, it will print a nice warning message telling users to install Node.js, and then makdownlint-cli2 using npx, then it will continue to run the other linters.

However... I just found mdformat, which is a Python-based Markdown linter with native pre-commit hooks. If we're OK with switching to another Markdown linter (which also might change some Markdown files to slightly different formatting rules this linter follows), I think this could be a much simpler and cleaner solution.

[dhellmann]

I thought of using containers, but than we would just require Docker / Podman instead, and it would only work with one of them (e.g., if write it as a Podman command, it won't work for Docker users). Thought it's probably too much maintenance and overhead, with many possible issues and edge cases.

Eh, we could just pick podman.

However... I just found mdformat, which is a Python-based Markdown linter with native pre-commit hooks. If we're OK with switching to another Markdown linter (which also might change some Markdown files to slightly different formatting rules this linter follows), I think this could be a much simpler and cleaner solution.

That's definitely worth looking at. Is the project maintained? How different is the output?