Docker hook was removed by bot

flying-sheep commented 9 months ago

What happened?

in 01c57019c8bf32670d7b795f2ed852f2db8d7b86, the docker hook was removed.

Relevant log output

No response

Is it possible to reproduce the bug with a small code snippet?

$ pre-commit autoupdate
[https://github.com/ComPWA/mirrors-taplo] Cannot update because the update target is missing these hooks: taplo-docker

Additional steps to reproduce the bug

No response

Which Python version were you using?

None

Python dependencies

No response

redeboer commented 9 months ago

Hmm curious indeed... Also how did the rev end up there in the .pre-commit-config.yaml? 😬

At any rate, there are some upstream problems as well with v0.9.0 (https://github.com/tamasfe/taplo/issues/542) that seem to be similar to problems with the Docker build for this mirror repo.

redeboer commented 9 months ago

Tbh I haven't been happy with this mirror since the beginning and would have preferred to see a taplo Python package with prebuilt wheels, which would make it trivial to host a (much faster) pre-commit hook. Have played around a bit with building wheels for Taplo with Maturin and I'll make a PR to Taplo once I have time to get back to that.

If you're interested, you can already try a test pip package: https://pypi.org/project/taplo-test

And here's the corresponding (temporary!) pre-commit hook:

  - repo: https://github.com/redeboer/taplo-pre-commit
    rev: v0.9.1rc1
    hooks:
      - id: taplo

Interested to hear if that hook works for you, but don't check it in to your CI ;)

flying-sheep commented 9 months ago

That’s exactly the solution I thought about, awesome job of getting it working!

Taplo upstream is currently in a bit of chaos since new maintainers are taking over and haven’t figured out all the processes, but eventually we could upstream your workflows so all the package building is centralized.

/edit: your changes are pretty neat! I’d not add the kitchen sink to .gitignore, otherwise that looks like a great solution.

My .gitignores usually look like:

__pycache__/
/.*cache/
/dist/

# and if others work on it too:
/.vscode/
/.idea/
/.python-version
/.venv/

maresb commented 9 months ago

Hey, this looks awesome!!!

A few days ago I was asking about an official pre-commit hook here: https://github.com/tamasfe/taplo/issues/535

maresb commented 9 months ago

Just had a closer look at the commits. That looks like it was way easier that I could have imagined, up until the OpenSSL stuff.

I was wondering why you'd need OpenSSL, and found the answer here.

Taplo depends on OpenSSL in order to fetch schemas via HTTPS, you will most likely need the openssl development files to be installed (openssl-dev or openssl-devel on most Linux-based systems).

But then it looks like the dependency was removed in 0.7.0. I suppose it was readded later? I'm not familiar with the taplo codebase or rust programming, so my explorations and questions may be quite naive.

redeboer commented 9 months ago

Thanks a lot for the feedback, great to hear it works!

Just had a closer look at the commits. That looks like it was way easier that I could have imagined, up until the OpenSSL stuff.

Well spotted indeed 😅 That was the painful part and actually what held me back from making a PR yet. Two things:

I also thought openssl was still in the dependencies by mistake, but it seems to be pulled in to the lock file through the reqwest (see direct dependency here, or with cargo tree: request -> hyper-tls -> native-tls -> openssl).
I did not manage to 'bake' openssl into the x86 build, hence the if-else in the workflow. Just started two builds that (hopefully) illustrated the problem: https://github.com/redeboer/taplo/actions/runs/7814054400 (static) https://github.com/redeboer/taplo/actions/runs/7814055668 (shared) Note that building openssl with a shared library seems to work, but once you install the wheels from the artifacts, I got missing library errors (even if openssl is installed), which made me look into those static builds with openssl-src. That makes me think the wheels from the package I described in https://github.com/ComPWA/mirrors-taplo/issues/13#issuecomment-1930749688 do not work on x86 architecture

redeboer commented 2 months ago

Removed the Docker hook, see https://github.com/ComPWA/mirrors-taplo/pull/18.

ComPWA / taplo-pre-commit