pre-commit-ci / issues

public issues for https://pre-commit.ci
17 stars 3 forks source link

Support for manual dependency management for local repos, system or script entry-points #13

Open lorenzwalthert opened 3 years ago

lorenzwalthert commented 3 years ago

I know there is no explicit dependency management for script or system hooks or local repos. This is not a problem when used locally, because people could install the dependencies once. However, there is no way to install these dependencies for https://pre-commit.ci to my knowledge. This means that any pre-commit.ci reun will fail if it has at least one local, system or script hook if I am not mistaken. An example is this run.

I saw there is #11 which would provide maximum flexibility. Maybe there is also a way to limit the scope and run custom code only before the hook check?

If not, maybe it would be helpful to indicate in .pre-commit-config.yaml if a hook should run on pre-commit.ci. Or set an env variable that can be checked in a system, script or local hook.

Looking forward for this to reach production readiness 🥳

cc: @katrinleinweber. Maybe we should anyways aim to support R as a language, as discussed in https://github.com/pre-commit/pre-commit/issues/926. There is support for conda now, maybe we can follow the PR and use renv: https://github.com/pre-commit/pre-commit/pull/1232. If you are interested, let's discuss in https://github.com/lorenzwalthert/precommit/issues/215. In any case, this would not solve the problem for local hooks and system or script hook in general.

asottile commented 3 years ago

arbitrary installation won't be supported for the free tier as it exposes too many risks and system / script are the escape hatch

lorenzwalthert commented 3 years ago

Ok. So are third level languages the only ones not supported by pre-commit.ci and the service will install required global system requirements of second level languages like conda?

asottile commented 3 years ago

for alpha currently the only languages I've added are python, node, and ruby -- but I'll be accepting PRs to add the other supported languages in the runner-image (which is public for this reason!)

but yes, by the time it's out of alpha it should have support for second class languages (maybe without docker / docker_image because I'm not sure how I can do that efficiently / safely)

Cielquan commented 3 years ago

Couldn't one e.g. for python stuff write a python script which gets called as a script hook at the top of .pre-commit-config.yaml which then determines if it runs inside pre-commit.ci or not and if so uses pip to install dependencies for other local hooks?

I did not look into the runner-image source yet.

asottile commented 3 years ago

the free tier intentionally does not allow network access at runtime so that wouldn't work

Cielquan commented 3 years ago

Ah ok thanks for the info. But if I interpret your answer correctly it would be a possibility for the paid plan than?

asottile commented 3 years ago

yes or something like it

eggplants commented 2 years ago

I know this problem is avoidable by using https://github.com/pre-commit/action instead. But I would like to use even if I had to pay for it...

asottile commented 2 years ago

@eggplants is there a specific tool you'd like made available? I can probably design a system for providing them

eggplants commented 2 years ago

Thank you! Here is: https://github.com/pecigonzalo/pre-commit-shfmt

asottile commented 2 years ago

good news, you can use shfmt without needing special system tools installed \o/ (since it is written in go)

repos:
-   repo: local
    hooks:
    -   id: shfmt
        name: shfmt
        entry: shfmt -w -i 4
        language: golang
        additional_dependencies: [mvdan.cc/sh/v3/cmd/shfmt@v3.4.3]
        types: [shell]
eggplants commented 2 years ago

This workaround looks good, thank you so much.

eggplants commented 2 years ago

@asottile Could you create PR for pecigonzalo/pre-commit-shfmt?

asottile commented 2 years ago

@eggplants feel free to take that yaml block above and dedent it and add it to .pre-commit-hooks.yaml in that repo! (though I would perhaps remove -i 4 from args if it's going to be extended?)

asottile commented 1 year ago

I've just released a brand new "lite" version of pre-commit.ci which may be of interest for non-managed hooks (such as those requiring special unsupported setup to make language: system or language: script work). you can read more about it at https://pre-commit.ci/lite -- the tl;dr is adds autofixing to GitHub Actions

gilesw commented 1 year ago

I love that pre-commit-ci is so fast and my company would pay for private repos. I tried out github super-linter and it's really slow because it has to pull a huge docker image. It's a shame that the pre-commit-ci team can't add all the common dependencies themselves to their image. I'm assuming it's fast because the image doesn't have to be pulled down every time it runs.

It's not clear what this mechanism does under the hood:-

additional_dependencies: [mvdan.cc/sh/v3/cmd/shfmt@v3.4.3]

If it works for any go project then maybe install actionlint.

If I can't then I'm going to go back to running pre-commit in gha and installing dependencies with adsf and using caching to speed that up.

asottile commented 1 year ago

It's a shame that the pre-commit-ci team can't add all the common dependencies themselves to their image

that misses the entire point of pre-commit -- the whole idea is that the tool installs and manages exactly the tools you specify in the configuration. if your contributors need to set up a bunch of manual stuff outside the tool they're unlikely to do it. the configuration above for shfmt uses the framework to manage a language: golant dependency

gilesw commented 1 year ago

I can agree that it's strange that for such a mature system that pre-commit hooks often don't install the dependencies they need.

asottile commented 1 year ago

fwiw this seems to work in-framework (though, also highlights a bug in actionlint!):

$ pre-commit  run actionlint --all-files
actionlint...............................................................Failed
- hook id: actionlint
- exit code: 1

.github/workflows/main.yml:6:10: string should not be empty [syntax-check]
  |
6 |     tags:
  |          ^
repos:
-   repo: local
    hooks:
    -   id: actionlint
        name: actionlint
        language: golang
        entry: actionlint
        additional_dependencies: [github.com/rhysd/actionlint/cmd/actionlint@v1.6.23]
        files: ^\.github/workflows/.*\.(yml|yaml)$
gilesw commented 1 year ago

Well I'm happy that pre-commit-ci now runs it and does it fast. So no blockers unless I find another strange hook that needs something unusual. I might do a pr on the actionlint repo so at least they can include an example of running it that way but it sounds like you're going to get this question asked again.

asottile commented 1 year ago

it's kinda odd that they don't have a language: golang integration directly -- seems they have a docker and system one but not the one that would ~generally be best for users