ComPWA / taplo-pre-commit

A pre-commit hook for Taplo, a TOML formatter written in Rust
https://github.com/tamasfe/taplo
12 stars 3 forks source link

Offer a pre-commit hook with a faster install (e.g. binary / Docker) #1

Closed m-roberts closed 1 year ago

m-roberts commented 1 year ago

Problem description

Currently, it takes multiple minutes to add to pre-commit. Whilst wrapping in a Docker container in CI helps, it is still not ideal for local workflows. It would be more energy and time efficient to provide a pre-built binary where possible.

Proposed solution

redeboer commented 1 year ago

Thanks for your interest and for the feedback, @m-roberts! Indeed, it takes way too long for this pre-commit hook to compile (in fact, the reason for offering the Docker containers is that this hook is killed on pre-commit.ci).

I'm not a Rust programmer, so I am assuming that build time of Taplo itself cannot be reduced. This means a solution has to be found with the way pre-commit installs this hook. As you suggest, an idea would be to call a pre-built binary, but I'm not sure what's the best way forward here. Of course, the most straightforward solution is to require the user to install taplo-cli locally and just write something like this to your .pre-commit-config.yaml:

  - repo: local
    hooks:
      - id: taplo
        name: taplo-cli
        entry: taplo format
        language: system
        types:
          - toml

That would remove the need for this mirror altogether, but it's not a nice solution, because you don't want users to have to install Taplo separately. Another idea may be to somehow let the hook (or pre-commit) download the binary when the hook is installed or called for the first time. For that, the hook needs to know the OS etc. Not sure if pre-commit already has mechanisms for this. If not, I doubt that it will be easy to let that downloading happen through the hook itself.

Anyway, just some thoughts. Do you have other ideas?

m-roberts commented 1 year ago

I think the hook would need to handle downloading the binary when the hook is installed or called for the first time. You're right that the hook would need to know the OS etc. I don't think that pre-commit already has mechanisms for this.

It is possible, but I have not yet thought through the logistics to be able to implement this optimally.

m-roberts commented 1 year ago

I have thought more about this, and I think the way to go would be to build taplo's Docker image for all platforms, and then create a new hook that uses pre-commit's docker_image language type. As long as the host system has Docker running, then it will only need to download the image before running.

For reference, hadolint offers 2 hooks - 1 that uses the host and 1 that is Dockerized. The Dockerized one can follow the same approach, and have an id of taplo-docker.

What do you think?

m-roberts commented 1 year ago

Just found that the image is on Docker: https://taplo.tamasfe.dev/cli/installation/docker.html However, it's only for linux/amd64: https://hub.docker.com/r/tamasfe/taplo/tags

If this were available for other architectures (e.g. linux/arm64) then it would be useful on more machines.

m-roberts commented 1 year ago

I have managed to get this to work:

  - repo: local
    hooks:
      - id: format-toml
        name: format toml
        entry: tamasfe/taplo:latest
        language: docker_image
        types: [toml]
        args: [format]

I do get a platform warning from an M1 MacBook (arm64):

format toml..............................................................Failed
- hook id: format-toml
- files were modified by this hook

WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested
 INFO taplo:format_files:collect_files: found files total=1 excluded=0 cwd="/src"
redeboer commented 1 year ago

Just found that the image is on Docker: https://taplo.tamasfe.dev/cli/installation/docker.html

Thanks! That may be of help for local runs of pre-commit. The Docker that this repo offers is actually just the pre-commit cache for this hook 😅

https://github.com/ComPWA/mirrors-taplo/blob/eac908688265f359aa45b06e5f0f79ccadf5654e/Dockerfile#L18

However, it's only for linux/amd64: https://hub.docker.com/r/tamasfe/taplo/tags

That's fine for a Docker, right?

redeboer commented 1 year ago

I have managed to get this to work:

  - repo: local
    hooks:
      - id: format-toml
        name: format toml
        entry: tamasfe/taplo:latest
        language: docker_image
        types: [toml]
        args: [format]

Indeed, that should also work if you for instance build and install Taplo locally. Still, it would be better if the pre-commit hook handles that installation.

m-roberts commented 1 year ago

The solution would be to take this config and add it to your hooks. See #2 for my PR

m-roberts commented 1 year ago

Unfortunately, I am unable to get this working:

Updating https://github.com/ComPWA/mirrors-taplo ... Cannot update because the update target is missing these hooks:
taplo-docker
redeboer commented 1 year ago

Did you set the rev to a specific tag before calling autoupdate? I just tried the instructions on the readme and that seems to work:

Add this to your .pre-commit-config.yaml

repos:
  - repo: https://github.com/ComPWA/mirrors-taplo
    rev: ""
    hooks:
      - id: taplo-docker

then run

pre-commit autoupdate --repo https://github.com/ComPWA/mirrors-taplo