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

[RFC] Replace pre-commit with precommix or pre-commit-hooks.nix #175

Closed
yajo opened this issue Nov 28, 2022 · 6 comments
Closed

[RFC] Replace pre-commit with precommix or pre-commit-hooks.nix #175

yajo opened this issue Nov 28, 2022 · 6 comments
Labels
enhancement New feature or request stale PR/Issue without recent activity, it'll be soon closed automatically.

Comments

@yajo
Copy link
Member

yajo commented Nov 28, 2022

Is your feature request related to a problem?
Here at OCA we use pre-commit extensively to make it more comfortable to share the same style of programming. It also bundles a lot of tools that help the coding and reviewing process.

However, pre-commit itself is divided into 3 parts:

  1. Install the hooks.
  2. Find the hooks.
  3. Run the hooks.

While it's perfect for running the hooks, and pretty OK for finding them, it's not perfect for installing them. How can that be, if we pin the hook tags? Because everything that's below the hook (python version, or node version, or stdlib version, OS version, etc.) is not pinned.

The obvious outcome of this problem is that every now and then, some hook dependency starts failing, and then the CI and dev experiencie starts melting across all OCA repos. It becomes a maintenance burden with ease. Here I've collected some examples (related but not exclusive to OCA):

  1. Fix prettier installation yet again #41
  2. pre-commit: Black < 22.3.0 is incompatible with click 8.1.0 #133
  3. [UPD] pre-commit config for 13.0: fix python version to 3.9 #143
  4. [FIX] mis_builder: let non-admins print reports mis-builder#476 (comment)
  5. Repo versions <=13.0: pylint in pre-commit broken on Python 3.11 #174
  6. Pin python version for <= 13.0 #182
  7. [15.0][FIX] l10n_es_aeat_mod390_2022: 2022 format l10n-spain#2783 (comment)
  8. [UPD] Upgrde isort, which does not build with the latest poetry release #183
  9. [FIX] isort: started breaking the world today #184
  10. feat: add --exclude-core-addons flag acsone/manifestoo#53 (comment)
  11. feat(click-odoo-listdb): handy script for listing odooable databases acsone/click-odoo-contrib#126 (comment)
  12. [13.0] Pre commit fails with "AttributeError: module 'collections' has no attribute 'MutableSet'" #80
  13. [FIX] pre-commit isort version Tecnativa/doodba-copier-template#370
  14. [FIX] pre-commit: Enforce last version of pre-commit for 14 #188
  15. 15.0 Pre-commit fails #193
  16. [FIX] pre-commit: bump nodejs version for Odoo 15 #194
  17. [FIX] Make Python version explicit in pre-commit workflow #196
  18. Support Python 2.7 #199
  19. Drop Python 2.7 #200
  20. build(pylint-odoo): bump for v16 #198 (comment)
  21. With a M1 mac, pre-commit run fails: urllib.error.HTTPError: HTTP Error 404: Not Found #119
  22. [IMP] Use specific python version for a set of hooks #282
  23. [FIX][15.0] branches Update flake8 autoflake #243
  24. Build dependency conflicts with setuptools and setuptools-scm maintainer-tools#647

Describe the solution you'd like
Here at @moduon I've developed a definitive solution for this problem. It's https://gitlab.com/moduon/precommix. The base idea is that you use pre-commit for running the hooks (where it's perfect), but you use nix to install the hooks (where it's perfect too) and direnv to make them available for both pre-commit and your terminal/IDE (where it's perfect too).

Since Nix is the world's most obsessively perfect packager ever invented, once you package something there, it will keep working forever. Guaranteed. And every app bundles every dependency in a granular manner. So we will never have this problem again.

The "bad" parts of this approach:

  1. You need to set up nix.
  2. You need to set up direnv.
  3. None of these work natively on Windows. You need to use WSL there.

None of these is particularly hard, but it can be annoying to bundle yet another tool in your belt for some people.

Describe alternatives you've considered
We could also bundle all current hooks in a docker image and use the docker_image language for pre-commit.

That would make you have to set up docker or podman instead of nix + direnv. This is more familiar for some people, although you lose some extra benefits of those tools.

FWIW, we also provides docker images for precommix. Although the hooks are not currently tailored for using them (that shouldn't be too hard to add).

Additional context
I've used it for months. It's a total pleasure.

@sbidoul
Copy link
Member

sbidoul commented Dec 11, 2022

I'll try to find some time to try out pre-commitx and form myself an opinion - at first sight it may look like a bit of a steeper learning curve than pipx install pre-commit for the OCA contributors.

@carmenbianca
Copy link
Member

I like the idea a lot, and I'm very charmed by Nix, but I don't think this is presently a workable solution due to the aforementioned learning curve. I wish Nix were easier than it is.

Just learning enough Nix to get the linters to run wouldn't be exactly sufficient, either. Inevitably, the user will want to reproduce some error that appears in the linter but not in their own Python virtualenv, and they'll need enough know-how of Nix to navigate that.

@yajo
Copy link
Member Author

yajo commented Dec 19, 2022

Well, you don't really need to know nix to use precommix. Just follow some instructions to setup your dev env. The magic is actually done by direnv. You can even use pre-commit installed with pipx or apt if you prefer, and they'll be compatible.

OTOH if you find it easier, we can use the same backend, but with docker_image. Users will just need to have installed podman or docker and hooks will keep working forever, which is the end goal. Yes, it's also another dependency. At least, more widespread.

@yajo yajo changed the title [RFC] Replace pre-commit with precommix [RFC] Replace pre-commit with precommix or pre-commit.nix Feb 28, 2023
@yajo
Copy link
Member Author

yajo commented Mar 1, 2023

Updating the list of issues in the description is becoming kinda fun... I have 14 already! 😸

@yajo yajo changed the title [RFC] Replace pre-commit with precommix or pre-commit.nix [RFC] Replace pre-commit with precommix or pre-commit-hooks.nix May 14, 2023
@len-foss
Copy link

@yajo
Well, outside of the 14 issue you listed, I can tell that I never created one but I think 10 hours wasted on pre-commit breaking is a rather low estimation (most of it when I was at Acsone).

Way too often you have a project that is mostly dormant, and when you come back a few months later some dependency is broken, and you have to find the right chain of dependency to get to a working solution again. And you have to be careful that after the version change it's using the exact same setting and doesn't start polluting all new commits by rewriting unrelated stuff in the file.

These dependencies, as you have noticed, might break in one setup but not in another (I saw how frequent it is as I changed from Ubuntu to Arch repos about one year ago).
Which means in some case you might want to have a local version of the pre-commit to be able to work, diverging from the project one. This difference meant in some case Gitlab's pre-commit would rewrite something differently, so it required to manually making the change before pushing again (again, happened a few times at Acsone).

With regards to the solution, I think it's a great idea, a great choice of tools and a very sane approach.
I tested it, install was really simple since both nix and direnv are packaged, the whole setup took less than 10 minutes.
So I really hope this can go forward!

@sbidoul you should definitely give it a try!

Copy link

There hasn't been any activity on this issue in the past 6 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this issue to never become stale, please ask a PSC member to apply the "no stale" label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

No branches or pull requests

4 participants