Build wheels for Python package

[redeboer]

As discussed in ComPWA/taplo-pre-commit#13 (comment), here is an attempt to build Python package wheels with Maturin. That would make it way easier for the Python community to integrate Taplo into their workflow and would also make it possible to host a fast Taplo pre-commit hook (#535).

A test taplo pip package is available here:
https://pypi.org/project/taplo-test

And you can use this as a test pre-commit hook:

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

Warning

This is an archived fork of ComPWA/taplo-pre-commit. Please use that repository instead!

OpenSSL dependency

Building wheels for MacOS and Windows was easy, but I ran into trouble with Linux because of the apparent need for OpenSSL. I thought that OpenSSL is not a dependency anymore (#302), but it is still pulled in indirectly through reqwest:

taplo/crates/taplo-cli/Cargo.toml

Lines 31 to 33 in 4bf7b53

	reqwest = { version = "0.11.9", default-features = false, features = [
	"json",
	] }

It seems that reqwest is used in several of the taplo crates, so I presume we have to deal with OpenSSL as a dependency. Manylinux does not offer OpenSSL however, which resulted in broken wheels. I therefore added openssl as an optional dependency with the vendored flag, i.e. cargo build --features vendored-openssl (open to suggestions for a better name).¹

Even though that solution solution seems to work², it only 'bakes' openssl into the wheels for Linux x86_64: the 32-bit version is still built with shared openssl lib, because its manylinux still runs into problems building with vendored OpenSSL. And I haven't managed to test whether those wheels actually work.

Important

Who can help test whether the x86 wheels work? You can try the taplo-test package or the artifacts from the job.

PyPI release

I'm happy to help maintain this repository (#403), especially the Python release. For now, however, we need to somehow share a MATURIN_PYPI_TOKEN for the workflow. Who of the Taplo maintainers is also on PyPI?

Pre-commit hook

To address #535, it will be necessary to put the .pre-commit-hooks.yaml under a different config (see example repo here). The reason is that (1) it speeds up the pre-commit clone and (2) this repo already has a pyproject.toml for the Maturin build. This would be an argument for creating a 'Taplo organization' #403 (comment). ³

Footnotes

1. 55c7e96 upgrades the lock files to keep the diff of 2ef0d97 small; you can revert that lock update if preferred. ↩

2. This is the job that published the taplo-test package. ↩

3. An organization would also make it easier to extract the VSCode extension to separate repo. ↩

[flying-sheep]

Python itself links against system OpenSSL, so we shouldn’t bundle it unless we can commit to frequent security updates: https://peps.python.org/pep-0513/#security-implications

The manylinux project itself only bundles it when the system OpenSSL version is too old: https://github.com/pypa/manylinux/blob/ea3724609ced892adb65f4d091cc01f1231d0235/docker/build_scripts/build-openssl.sh#L28

[redeboer]

Python itself links against system OpenSSL, so we shouldn’t bundle it unless we can commit to frequent security updates: https://peps.python.org/pep-0513/#security-implications

The manylinux project itself only bundles it when the system OpenSSL version is too old: https://github.com/pypa/manylinux/blob/ea3724609ced892adb65f4d091cc01f1231d0235/docker/build_scripts/build-openssl.sh#L28

Ah okay thanks. Then I don't get why the wheels without vendored OpenSSL cannot run on Linux. See this failed job or try installing the wheels from its artifacts, e.g.:

gh run download 7814055668 -n wheels -D wheels --repo redeboer/taplo
pip install --no-index --find-links wheels/ taplo
taplo --help

[flying-sheep]

The job failure happens because the maturin action runs in some ancient container with OpenSSL 1.0.x (libssl.so.10), whereas modern systems have OpenSSL 3 (libssl.so.3). The built library then links against the ancient ABI instead of the modern one.

I think the way the manywheels action works is that it first builds against a newer OpenSSL and then removes the OpenSSL library. That way it links against the OpenSSL 3 ABI.

But maybe I’m wrong and for adequate compatibility one needs to bundle OpenSSL like you do. In that case, we’re stuck with having to do frequent releases whenever there’s some OpenSSL security patch 😞

[redeboer]

The job failure happens because the maturin action runs in some ancient container with OpenSSL 1.0.x (libssl.so.10), whereas modern systems have OpenSSL 3 (libssl.so.3).

Afaics the maturin actions just uses the manylinux_2014 container and that comes with OpenSSL 1.0.x:

$ docker run --rm -it quay.io/pypa/manylinux2014_x86_64 openssl version
OpenSSL 1.0.2k-fips  26 Jan 2017

In fact, I wonder why openssl-devel needs to be installed at all. If you don't, Taplo won't compile in manylinux at all.

taplo/.github/workflows/python.yml

Lines 45 to 46 in ebdb3e3

	before-script-linux: |
	yum install -y openssl-devel

But maybe I’m wrong and for adequate compatibility one needs to bundle OpenSSL like you do. In that case, we’re stuck with having to do frequent releases whenever there’s some OpenSSL security patch 😞

Indeed, this is the Catch-22 :S convenience (and insecurity?) of bundling OpenSSL, or pointing to shared libraries...

[flying-sheep]

Is there anything blocking this from being merged?

[redeboer]

From my side:

• support for x86 (needed?)
• bundling with with OpenSSL, as this PR does now (with a regular release flow this should be fine imho)

Details in the comments above

[flying-sheep]

Yeah, I think there’s no way around bundling at the moment, and having only 64 bit wheels is probably fine for now.

If someone wants 32 bit, they can start complaining I guess.

[bollwyvl]

This would be lovely! Is it anticipated the PyPI package would carry the taplo-cli version number, or something else? For downstreams that already have a taplo package, e.g. conda-forge, would there be a preferred name for the python package when not installed from PyPI, e.g. python-taplo?

A big thing for making this an easily-adoptable tool for the broader python ecosystem would be if taplo (even the existing fat binary CLI) could be coaxed into looking in pyproject.toml#/tool/taplo to reduce dotfile proliferation.

[redeboer]

Is it anticipated the PyPI package would carry the taplo-cli version number, or something else?

The version number of the PyPI package is the same as that of the taplo-cli crate, e.g. 0.9.0. That is, what taplo --version returns.

taplo/pyproject.toml

Line 29 in 3487ebe

manifest-path = "crates/taplo-cli/Cargo.toml"

For downstreams that already have a taplo package, e.g. conda-forge, would there be a preferred name for the python package when not installed from PyPI, e.g. python-taplo?

Good point. Not sure if a name clash should be avoided here, it's still the same CLI that is installed, right? (Or does conda-forge provide all the crates?)

I'm thinking about https://anaconda.org/conda-forge/graphviz vs https://anaconda.org/conda-forge/python-graphviz. These are actually different packages, one being the actual DOT parser, the other the Python interface. I don't think we have the same situation with taplo on PyPI and taplo on conda-forge. Both are just a CLI tool afaiu.

A big thing for making this an easily-adoptable tool for the broader python ecosystem would be if taplo (even the existing fat binary CLI) could be coaxed into looking in pyproject.toml#/tool/taplo to reduce dotfile proliferation.

I agree, would also love to see that. Haven't found an issue on this in this repo, could you post one? So pyproject.toml ad an alternative configuration file for taplo.

[panekj]

Is there anything blocking this from being merged?

I don't understand why is it required for pre-commit tool and I'm not keen on supporting additional language ecosystem that isn't familiar to me.
Additionally it introduces a whole new workflow just to build a python package that isn't integrated with release workflow.

[mrijken]

A Python variant is in a lot of use cases easier to install than a Rust variant and the number of possible users will grow with huge numbers when a Python variant is supported. I'm sure the community can and will help you to support the (small) Python part.

In our company we want to integrate a toml formatter in our ci/cd pipeline. Without a Python package, it is hard for us to use it.

[flying-sheep]

that isn't integrated with release workflow.

are you sure?

    tags:
      - release-*cli-*

looks to me like the workflow is triggered by pushing release tags, which seems like integration with the release process to me.

[panekj]

are you sure?

I'm sure

[redeboer]

Additionally it introduces a whole new workflow just to build a python package that isn't integrated with release workflow.

That's true, at the time I created a separate workflow, but over the past half year the existing workflows were extended quite a bit.

I hope ffd444a is more agreeable to the intended release flow. What I like less about it, is that the Maturing build process is now only tested on releases.

A Python variant is in a lot of use cases easier to install than a Rust variant and the number of possible users will grow with huge numbers when a Python variant is supported.

I agree, see also issues like #603 and other issues related to pyproject.toml.

[redeboer]

What I like less about it, is that the Maturing build process is now only tested on releases.

Oww okay, nvm 😅
https://github.com/tamasfe/taplo/actions/runs/10459526952/job/28964054546?pr=549
Fixed by 2ed2723

[panekj]

This will also require a token configured in secrets and until project owner becomes active again, that's impossible to implement.

[redeboer]

This will also require a token configured in secrets and until project owner becomes active again, that's impossible to implement.

That's right, see PR description

PyPI release

I'm happy to help maintain this repository (#403), especially the Python release. For now, however, we need to somehow share a MATURIN_PYPI_TOKEN for the workflow. Who of the Taplo maintainers is also on PyPI?

[panekj]

That's right, see PR description

I've seen that, I'm telling you why I won't merge it

[FlavioAmurrioCS]

My two cents.

• The owner of the pypi project should be able to add @tamasfe as a owner
• @tamasfe should then remove the original owner.
• @tamasfe should then remove all other tokens and create a project token
• finally add the new token as a repo secret.

I believe the only owner of the pypi project should be the owner of taplo so that we only need to trust one person.

[FlavioAmurrioCS]

Alternatively, you could try adding the project as a trusted publisher so that a token is not needed.

https://docs.pypi.org/trusted-publishers/adding-a-publisher/

[s-weigand]

Two more cents

Using trusted publisher is the recommended way by PyPA and the default of pypa/gh-action-pypi-publish.

[redeboer]

Thanks for the tips/cents! ;)
Ah yes, I was overthinking with Maturin, you can indeed just use PyPI's recommended publish action.

I'm happy to transfer ownership of https://pypi.org/project/taplo/ to @tamasfe. Could you confirm which PyPI account to transfer it to?

[panekj]

Could you confirm which PyPI account to transfer it to?

I can take it, as there currently isn't anyone else who maintains taplo.
https://pypi.org/user/panekj/

[redeboer]

Awesome, thanks for merging!