Fix pre commit in container

[charlesroelli]

docker compose run --rm web python -m pre_commit run --all-files does not currently work. Here are the changes needed to get it running properly.

[ulgens]

I'm not sure this is desired. pre-commit hooks don't require any dependency that is handled with the container setup, and I believe the standard/common practice is to keep anything git related on the host env. Can you please give more details about the usecase?

[charlesroelli]

The use case is running the pre-commit checks in a local development environment using Docker, which a few of us tried to do at a recent sprint. Maybe these tweaks belong in a separate build stage, though, if there are cases where they should not be applied.

[ulgens]

which a few of us tried to do at a recent sprint

Sorry, I'm missing why. Hooks can be called separately, but the expected flow to use them is to trigger the hooks during a commit, automated via git. Making a commit in a container requires access to the hosts .git config, which is not easy or feasible to provide.

[charlesroelli]

I don't recall anyone committing from the container. The use case is to check that the hooks succeed independent of committing without having to first commit, go online, push, open this page, let the CI run and open the result. The container happens to be a convenient place to run the hooks since it seems to have the required dependencies, including e.g. git which doesn't work due to the safe directory issue.

[ulgens]

The container happens to be a convenient place to run the hooks since it seems to have the required dependencies

I'm not opposing the case, but just wanted to clarify, the hooks don't depend on any specific dependency in the container. Only dependency, afaik, is pre-commit.

[charlesroelli]

Right, after reading about pre-commit, it seems the intention of that project is that its user should not have to install any extra dependencies to get the hooks running. Is it a bug in pre-commit, then, that we have to install libatomic1 to make the hooks work?

[charlesroelli]

Aha, it looks like recent node installs now need libatomic.

[ulgens]

Aha, it looks like recent node installs now need libatomic.

I'm missing where do we need to install node to run pre-commit hooks.

[charlesroelli]

Probably for mirrors-prettier.

[ulgens]

I removed node from my system and tried to run hooks:

I'll try to replicate the issue if you have a way to reproduce it.

[charlesroelli]

You might have libatomic1 installed for other reasons, as most of us probably do, so it's unlikely this will reproduce outside of Docker. libatomic1 is probably also installed in the Github Actions runner image, although I don't know how to check that.

[ulgens]

That's possible, but I can't follow you, sorry. If it's required for node, but node is not required for hooks, why would we need libatomic1?

I checked my system to be sure that libatomic is not installed and I couldn't find any trace of it being installed.

All in all, I'd still recommend keeping your dev tooling out of the container for the best experience.

[charlesroelli]

Well, that's the twist: pre-commit installs its own copy of node via nodeenv, in order to run prettier, so it should not matter whether you have node installed beforehand. It's interesting though, that it works for you without libatomic1. Maybe this was already fixed somewhere upstream.

[ulgens]

Found this explanation in pre-commit docs:

Support: node hooks work without any system-level dependencies. It has been tested on linux, windows, and macOS and may work under cygwin.

https://pre-commit.com/#node

[charlesroelli]

Same thing happens with prek now:

~/Code/djangoproject.com$ podman compose run --rm web prek run
Creating djangoprojectcom_web_run ... done
Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg.
WARN[0000] Requested oom_score_adj=0 is lower than the current one, changing to 200 
Operations to perform:
  Apply all migrations: accounts, admin, aggregator, auth, blog, checklists, contenttypes, dashboard, docs, flatpages, foundation, fundraising, members, redirects, registration, releases, sessions, sites, subscriber, thumbnail, tracdb
Running migrations:
  No migrations to apply.
python -m pysassc djangoproject/scss/output.scss djangoproject/static/css/output.css --style=compressed

271 static files deleted, 274 static files copied to '/usr/src/app/data/static_root'.
error: Failed to install hook `prettier`
  caused by: Command `npm install` exited with an error:

[status]
exit status: 127

[stderr]
node: error while loading shared libraries: libatomic.so.1: cannot open shared object file: No such file or directory

ERROR: 2
Error: executing /usr/bin/docker-compose run --rm web prek run: exit status 2

See also the related issue in mirrors-prettier.

[ulgens]

@charlesroelli Thanks for the reproducible example. I tested it, and it results in the error you mentioned. I have a couple of comments:

podman compose run --rm web prek run

• This usage causes the container to be removed after the execution, which leads to prek/pre-commit caching being bypassed, ending up reinstalling hooks everytime from scratch. I personally wouldn't recommend this approach.
• Thanks for the issue ref from mirrors-prettier. The issue looks valid, but also

I have found, atleast for version rev: v3.5.3 (have not tried other versions), that setting language_version: 18.20.8 fixes the libatomic.so issue. So it seems like the dependency issue happens due to node version used.

seems to be working fine. I think this is a simpler fix and I'd recommend going with it for now.

• I'm still a bit lost about why you would want to run git hooks specifically in a container, considering the only dependency issue can be handled by the recommendation above. But if enough people agree that this is a common enough use case, and adding language_version is not enough, I'm okay with adding the dependency.

[charlesroelli]

Yes, setting language_version to a node version that doesn't require libatomic1 does seem like the least invasive fix for now. I will adjust the PR, thanks for the quick response!

[ulgens]

Added a comment about referencing the prettier issue, looks good otherwise.

[ulgens]

The failing test is unrelated, I think we can ignore it in the context of this PR.