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

build: easier containerized pre-commit execution for friends without nix #980

Merged
merged 1 commit into from
Feb 20, 2023

Conversation

yajo
Copy link
Member

@yajo yajo commented Feb 15, 2023

poe lint now uses nix if available. Otherwise, it will try to use docker or podman, by downloading the nix image and running pre-commit nix-based from there.

Fixes #931.

`poe lint` now uses nix if available. Otherwise, it will try to use docker or podman, by downloading the nix image and running pre-commit nix-based from there.

Fixes #931.
@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Merging #980 (a6063b4) into master (37388f9) will decrease coverage by 0.47%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #980      +/-   ##
==========================================
- Coverage   96.66%   96.20%   -0.47%     
==========================================
  Files          42       42              
  Lines        3122     3137      +15     
==========================================
  Hits         3018     3018              
- Misses        104      119      +15     
Flag Coverage Δ
unittests 96.20% <0.00%> (-0.47%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
devtasks.py 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@yajo yajo merged commit c6b5728 into master Feb 20, 2023
@yajo yajo deleted the poe-lint-container branch February 20, 2023 19:47
@rafalkrupinski
Copy link
Contributor

@yajo I'm sorry, I didn't communicate to well. By 👍 I meant that I will test it soon, so I don't leave you without an answer for a week.
I don't like docker desktop so I'm trying podman. I'll let you know how it worked once I figure out how to configure it.

@rafalkrupinski
Copy link
Contributor

I have docker client installed and I think it's interfering with poe.

 % which docker
docker: aliased to podman
% docker ps
CONTAINER ID  IMAGE       COMMAND     CREATED     STATUS      PORTS       NAMES
% poe lint
Poe => lint
Nix not found; fallback to a container
Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
Traceback (most recent call last):
...

@yajo
Copy link
Member Author

yajo commented Feb 25, 2023

That's because aliases only work within bash, but we're executing a process directly.

I guess it makes sense to priorize podman over docker.

@yajo
Copy link
Member Author

yajo commented Feb 25, 2023

Please check with #1005.

@rafalkrupinski
Copy link
Contributor

After removing docker:

% poe lint            
Poe => lint
Nix not found; fallback to a container
Error: creating container storage: the container name "copier-lint-v1" is already in use by a1093ee368e5ce414ac034fee3d2c0864a6f33e74947370b6b933c55b1dd9109. You have to remove that container to be able to reuse that name: that name is already in use
Error: unable to start container a1093ee368e5ce414ac034fee3d2c0864a6f33e74947370b6b933c55b1dd9109: preparing container a1093ee368e5ce414ac034fee3d2c0864a6f33e74947370b6b933c55b1dd9109 for attach: lsetxattr /Users/user/Documents/Projects/copier/.envrc: operation not supported
Traceback (most recent call last):
  File "/Users/user/Documents/Projects/copier/devtasks.py", line 64, in lint
    local["nix"].with_cwd(HERE)[args] & TEE
  File "/Users/user/Library/Caches/pypoetry/virtualenvs/copier-U7tILSPN-py3.10/lib/python3.10/site-packages/plumbum/machines/local.py", line 242, in __getitem__
    return LocalCommand(self.which(cmd))
  File "/Users/user/Library/Caches/pypoetry/virtualenvs/copier-U7tILSPN-py3.10/lib/python3.10/site-packages/plumbum/machines/local.py", line 202, in which
    raise CommandNotFound(progname, list(cls.env.path))
plumbum.commands.processes.CommandNotFound: (CommandNotFound(...), 'nix', [<LocalPath /Users/user/Library/Caches/pypoetry/virtualenvs/copier-U7tILSPN-py3.10/bin>, <LocalPath /Users/user/Library/Caches/pypoetry/virtualenvs/copier-U7tILSPN-py3.10/bin>, <LocalPath /Users/user/.pyenv/shims>, <LocalPath /Users/user/Library/pnpm>, <LocalPath /Users/user/.poetry/bin>, <LocalPath /Users/user/.nvm/versions/node/v16.13.1/bin>, <LocalPath /opt/local/bin>, <LocalPath /opt/local/sbin>, <LocalPath /Library/Frameworks/Python.framework/Versions/3.8/bin>, <LocalPath /usr/local/bin>, <LocalPath /System/Cryptexes/App/usr/bin>, <LocalPath /usr/bin>, <LocalPath /bin>, <LocalPath /usr/sbin>, <LocalPath /sbin>, <LocalPath /opt/X11/bin>, <LocalPath /Users/user/.local/bin>])

@rafalkrupinski
Copy link
Contributor

I guess that's the problem:
lsetxattr /Users/user/Documents/Projects/copier/.envrc: operation not supported

/Users is a directory mounted from the host system

@rafalkrupinski
Copy link
Contributor

I'm just a small contributor, so I don't thing I have any influence over this project, but I think using Nix is already an overkill for running a linter, and adding more components, like docker/podman is making the environment even more complex and less portable.
I know some people prefer using Nix or docker for development, but requiring it is IMO not a good practice, especially in a small open source project.
I'd rather KISS

@yajo
Copy link
Member Author

yajo commented Feb 27, 2023

What I was trying to do with this enhancement was to make it simpler for those that don't have nix but have podman or docker.

Maintaining pre-commit configurations is a complex topic. It's so complex that my first approach to fix the problem was to create Autopretty which, as you see, was under the umbrella of the copier org. It fixed that problem for me for awhile.

Copier is a project I maintain in my free time. However, in my professional time I've stumbled a huge lot of times with the problem that pre-commit hook installations fail randomly every now and then. Somebody will publish a new version of a dependency you use on your hooks that introduces some incompatibility and... 💣 I don't know why, but they usually do it in the day you most need your CI working like a charm because there's an emergency.

That's something that most devs accept because they don't know any other option, or because just yes. Recently I've been gathering some of these problems in OCA/oca-addons-repo-template#175 (comment). As of today, it's counting 11 public issues linked there, but the total number of times I've fixed this is probably more than the triple. FWIW Autopretty is now archived because the last time I tried to use it, after some time without touching it, everything was broken yet again! 🤦🏼‍♂️

When I discovered nix (and https://github.com/cachix/pre-commit-hooks.nix in particular), it was mind-blowing to learn that I can write code in a way that will survive the pass of time. You will be able to develop Copier after 20 years if I don't touch it. Also you'll be able to build it and run it. If I have 30 minutes to dedicate to Copier, I don't want to waste them fighting with some random thing that suddenly crashed on CI.

So, although I'm the biggest fan of KISS, there are different definitions of Simple and Short. For me, something simple is also something that I know for certain it will keep working. Recently the https://devenv.sh/ project was born, as an attempt to make nix simpler for non-nix people. That's what I'm using in Copier, to KISS. IMHO, tools like devenv are going to make Nix more widespread. It just solves too many problems as to ignore it. Be ready to see more flake.nix and devenv.nix files here and there, each time in more projects.

Apart from all these reasons, Copier is also something I treat with lot of ❤️ . Sometimes I use it to experiment, and sometimes to try to share with the reader the wonderful things I've learned. Nix is one of the best things ever invented in IT, so I simply love to have it around. Makes development funnier!

I appreciate a lot the humility in your words and I hope you understand why nix is here to stay.

OTOH I dedicated some time to make sure all this new tooling works in gitpod. So if you want to do small contributions to Copier, probably the best way is to click the Gitpod badge on the README, wait for a minute until all dependencies are pulled, and give back your code. Also, you can always do pipx run black . and it should mostly work.

@rafalkrupinski
Copy link
Contributor

I hope you understand why nix is here to stay.

Sure, it's your project and you can do with it whatever you want.

So if you want to do small contributions to Copier on the README

I actually have a refactoring to push, and I'm not sure if it qualifies as small.

Also, you can always do pipx run black . and it should mostly work.

Um, I did, and it changed files I didn't touch :D

Also running pytest outside of nix makes some tests fail. Now, how do you know copier will work on another users' machines, outside this carefully crafted environment?

What you say sounds to me like pre-commit itself is broken. If you compare it to poetry or node.js tools, they all have lock files that lock all runtime and dev dependencies versions. Seems pre-commit runs with whatever version is the last one, so if one of the hooks uses another packages internal code (black & click in one of the bugs you've linked), and it changes, than too bad for you. If that's the case, I see here a lot of effort to go around a single broken tool.

@yajo
Copy link
Member Author

yajo commented Mar 11, 2023

Yes, that's why pre-commit is so broken.

@sisp
Copy link
Member

sisp commented Mar 11, 2023

@yajo: Did you also experience pre-commit issues when most of the tools were installed as dev-dependencies via Poetry? I know that some pre-commit hooks were installed via pre-commit directly, so that's certainly a problem. For Prettier, it would have been possible to manage it via package.json and a lock file of one of the common package managers, expect developers to install them using the corresponding package manager, and configure it in .pre-commit-hooks.yaml in the same way as the Poetry-installed Python pre-commit hooks. But then there were still the plenty of pre-commit hooks from https://github.com/pre-commit/pre-commit-hooks. Were they problematic? Just trying to understand your pain.

I've not used Nix or devenv yet, contributing to Copier with just Poetry and manually installed Flake8 + plugins has worked sufficiently, but you seem so convinced and fascinated by Nix and devenv that I'll actually look into it. 😄

@yajo
Copy link
Member Author

yajo commented Apr 7, 2023

I've had plenty of problems in the past with prettier installation. I actually get one or two installation random failures per week on my company's CI (in the projects that still don't use nix).

Poetry gave no problems back then, but its design tries to be kinda pure. However, in reality, it lacks important pieces. Example: python-poetry/poetry#6154 (comment)

I think asking people to use npm to install dev dependencies is at least as bad as asking them to install nix. However it's much worse in many other aspects.

So, once I've found a solution that is good and covers any possible use case and is easy to use, it's just too hard to go back 😅

FWIW you don't need to "learn" nix to use it for copier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

poe lint should be faster if user doesn't have nix installed
3 participants