[RFC] Migrate from `qmlformat` to `qml_formatter` #10929

Holzhaus · 2022-09-29T09:42:58Z

Although qml_formatter is much less mature, we can at least pin a specific version via pre-commit (in contrast, qmlformat is distributed with Qt, so in order to update it we need to update our Qt version). This is especially annoying since qmlformat seems to change its formatting rules on every version bump, so version incompatibilities between local computer and CI happen very often.

There are still a few bugs blocking that prevent us from using it though:

daschuer · 2022-09-29T16:16:52Z

This seems to pull in the requirements of the whole rust build system into Mixxx. Probably not a good fit for us.
Is there a way to use a binary distribution? Or an alternative based on Python?

2022-09-29T13:18:48.5045588Z [INFO]�[m This may take a few minutes...
2022-09-29T13:18:48.5058361Z An unexpected error has occurred: CalledProcessError: command: ('cargo', 'install', '--bins', '--root', '/github/home/.cache/pre-commit/repo1h5bsi4x/rustenv-default', '--path', '.')
2022-09-29T13:18:48.5058813Z return code: 1
2022-09-29T13:18:48.5059021Z expected return code: 0
2022-09-29T13:18:48.5060569Z stdout:
2022-09-29T13:18:48.5061033Z     Executable `cargo` not found
2022-09-29T13:18:48.5062221Z stderr: (none)
2022-09-29T13:18:48.5091236Z Check the log at /github/home/.cache/pre-commit/pre-commit.log
2022-09-29T13:18:48.5280645Z ##[error]Process completed with exit code 3.
2022-09-29T13:18:48.5334581Z ##[group]Run git diff-index -p HEAD > "${PATCH_FILE}"
2022-09-29T13:18:48.5334914Z �[36;1mgit diff-index -p HEAD > "${PATCH_FILE}"�[0m
2022-09-29T13:18:48.5335239Z �[36;1m[ -s "${PATCH_FILE}" ] && echo "UPLOAD_PATCH_FILE=${PATCH_FILE}" >> "${GITHUB_ENV}"�[0m
2022-09-29T13:18:48.5335764Z shell: sh -e {0}

Holzhaus · 2022-09-29T22:13:32Z

This seems to pull in the requirements of the whole rust build system into Mixxx. Probably not a good fit for us. Is there a way to use a binary distribution? Or an alternative based on Python?

No, i don't think there is a binary distribution, and there's also no Python implementation that I know of. I found a 2 years old alpha-stage plugin for prettier (node JS), but I could not make it work (always fails to parse our QML).

I don't think the rust dependency is a big problem. It's neither a compile-time nor a runtime dependency. It's only needed for running pre-commit locally, and you can still skip the hook and rely on CI if you don't want to install cargo.

Also, @Swiftb0y and me are working on rekordcrate which is also written in Rust with the goal to replace the kaitai-based Rekordbox parser implementation and add support for serialization. So an actual compile-time rust dependency would be on the horizon anyway...

The tool itself is still under development, but the devs of qml_formatter seem to be quite responsive (fixes were pushed within a few hours after opening the bug reports).

daschuer

It's only needed for running pre-commit locally, and you can still skip the hook and rely on CI if you don't want to install cargo.

After merging this, it will fail every commit with the obscure error message like above. This happens even without touching any qml file. I have no idea how many files are pulled in by cargo, but for now it feels a bit overdone and necessary hassle for a new contributor, just want to alter come c++ code.

Can we make this somehow less mandatory, like one of these ideas:

disable it by default locally
installing qml_formatter only if qml files have changed
Skip hook if cargo is not installed
Fail early with a descriptive error message
add the cargo installation to all our environment scripts.
Add qml_formatter to our vcpkg environment

daschuer · 2022-09-30T08:23:10Z

Is it correct that cargo requires ~2 GB ?

Holzhaus · 2022-09-30T08:23:21Z

I think Executable "cargo" not found is pretty clear, but I agree that it would be better if the user wouldn't have to care about this.
I'll look into working on pre-commit/pre-commit#1863 (comment).

Holzhaus · 2022-09-30T08:25:24Z

Is it correct that cargo requires ~2 GB ?

No, it uses 550 MiB (excluding stuff like gcc/llvm-libs/curl that you need anyway, even without rust):

Name            : rust
Version         : 1:1.64.0-1
Description     : Systems programming language focused on safety, speed and concurrency
Architecture    : x86_64
URL             : https://www.rust-lang.org/
Licenses        : MIT  Apache
Groups          : None
Provides        : cargo  rustfmt
Depends On      : gcc-libs  llvm-libs  curl  libssh2  gcc
Optional Deps   : lldb: rust-lldb script
                  gdb: rust-gdb script [installed]
Required By     : rust-src
Optional For    : None
Conflicts With  : cargo  rustfmt  rust-docs<1:1.56.1-3
Replaces        : cargo  rustfmt  cargo-tree  rust-docs<1:1.56.1-3
Installed Size  : 551.29 MiB
Packager        : Jan Alexander Steffens (heftig) <heftig@archlinux.org>
Build Date      : Thu 22 Sep 2022 05:27:10 PM CEST
Install Date    : Tue 27 Sep 2022 10:42:40 PM CEST
Install Reason  : Explicitly installed
Install Script  : No
Validated By    : Signature

daschuer · 2022-09-30T08:28:26Z

How about "disable it by default locally" and wait until rust is settled in Mixxx anyway. Luckily the GitHub workflow runners have rust installed by default ..

Holzhaus · 2022-09-30T14:00:18Z

I think Executable "cargo" not found is pretty clear, but I agree that it would be better if the user wouldn't have to care about this.
I'll look into working on pre-commit/pre-commit#1863 (comment).

Done, have a look at: pre-commit/pre-commit#2534
With this PR, you don't need to install rust yourself. It's automatically bootstrapped by pre-commit (just like node or ruby).

daschuer · 2022-09-30T15:41:06Z

Cool, now we just need to wait for a release.
Is the required memory smaller now?

Can we disable this by default and carry on here?
Or make it dependent to the next version number of pre-commit that it is solved automatically?

Holzhaus · 2022-09-30T19:17:08Z

Is the required memory smaller now?

It installs rust locally, compiles and then deletes the rustup distribution. It could theoretically also delete the entire tooling and only keep the compiled binaries, but I wanted to get some feedback on the implementation first. I'm also unsure if this makes hook version bumps too annoying if it always reinstalls all over.

Can we disable this by default and carry on here?
Or make it dependent to the next version number of pre-commit that it is solved automatically?

No, I don't think this is possible with pre-commit. I think it's not that much of an issue that it really prevents merging (I have the impression that most contributors are not using pre-commit for whatever reason anyway), but if you insist we can also wait for the next pre-commit release.

daschuer · 2022-09-30T20:38:41Z

At the moment this hook is only relevant for a handful of contributors. The others will face the error message above and annoyed by a > 500 MB download after figured out the solution for no reason.
So I think it is the best to merge this as disabled hook, an enable it on GitHub. This way we have a reference for formatin in our CI, but no one is annoyed.

Holzhaus · 2022-10-20T10:10:16Z

I'm guesstimating that a new pre-commit version will be released soon-ish, which will feature support for bootstrapping rust: pre-commit/pre-commit#2534 (comment)

I've already set the minimum pre-commit version to 2.21.0, so that this PR should start working once the next pre-commit version is released.

Swiftb0y · 2022-10-20T12:42:50Z

At the moment this hook is only relevant for a handful of contributors. The others will face the error message above and annoyed by a > 500 MB download after figured out the solution for no reason. So I think it is the best to merge this as disabled hook, an enable it on GitHub. This way we have a reference for formatin in our CI, but no one is annoyed.

Well if we only run it on CI but not locally, then people will get annoyed that that pre-commit passes locally but not on CI...

github-actions · 2023-01-19T00:08:01Z

This PR is marked as stale because it has been open 90 days with no activity.

Although `qml_formatter` is much less mature, we can at least pin a specific version via pre-commit. This is especially important since `qmlformat` seems to change its formatting rules on every version bump, so version incompatibilities between local computer and CI happen very often.

Mixxx is a C project, and requiring contributors to set up a rust installation on their systems just to be able to use `qml_formatter` is a bit tedious. Fortunately, pre-commit 2.21.0+ features support for bootstrapping Rust toolchains using rustup, so that no preexisting system install is necessary. See pre-commit/pre-commit#2534 for details. We set pre-commit 2.21.0 as the minimum required version, to prevent users that use an older pre-commit version and don't have rust installed from running into problems and to inform them that they should update. If someone uses an pre-commit version < 2.21.0, the following error message will be shown: $ pre-commit run An error has occurred: InvalidConfigError: ==> File .pre-commit-config.yaml ==> At Config() ==> At key: minimum_pre_commit_version =====> pre-commit version 2.21.0 is required but version 2.20.0 is installed. Perhaps run `pip install --upgrade pre-commit`. Check the log at /home/user/.cache/pre-commit/pre-commit.log

daschuer

pre-commit has been released as V2.21.0 including pre-commit/pre-commit#2534 see:
https://github.com/pre-commit/pre-commit/releases/tag/v2.21.0

This means that the next commit will require a 116 MB download including configuration it takes here 4 min.

Since it is a one time task, it is kind of OK now.

Thank you.

Holzhaus added the code quality label Sep 29, 2022

Holzhaus force-pushed the qmlformat-pre-commit-pinned branch from d276d65 to 19e7743 Compare September 29, 2022 13:16

Holzhaus mentioned this pull request Sep 29, 2022

Fix automatic QML type registration for Qt 6.3.x #10928

Merged

Holzhaus force-pushed the qmlformat-pre-commit-pinned branch from 19e7743 to 30520b2 Compare September 29, 2022 23:28

Holzhaus marked this pull request as ready for review September 30, 2022 07:28

daschuer requested changes Sep 30, 2022

View reviewed changes

daschuer mentioned this pull request Nov 19, 2022

Precommit qmlformat crash fixes #11078

Merged

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Jan 19, 2023

Holzhaus force-pushed the qmlformat-pre-commit-pinned branch from 1f5a473 to aa71f70 Compare January 19, 2023 09:42

Holzhaus added 3 commits January 19, 2023 11:21

style(qml): Apply formatting changes from qml_formatter

2f756b7

Holzhaus force-pushed the qmlformat-pre-commit-pinned branch from aa71f70 to 7ec6d21 Compare January 19, 2023 10:21

Holzhaus removed the stale Stale issues that haven't been updated for a long time. label Jan 19, 2023

github-actions bot added library ui labels Jan 19, 2023

Holzhaus changed the base branch from main to 2.4 January 19, 2023 10:22

Holzhaus requested a review from daschuer January 19, 2023 10:22

daschuer approved these changes Jan 19, 2023

View reviewed changes

daschuer merged commit fa06c46 into mixxxdj:2.4 Jan 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Migrate from `qmlformat` to `qml_formatter` #10929

[RFC] Migrate from `qmlformat` to `qml_formatter` #10929

Holzhaus commented Sep 29, 2022

daschuer commented Sep 29, 2022 •

edited

Loading

Holzhaus commented Sep 29, 2022 •

edited

Loading

daschuer left a comment

daschuer commented Sep 30, 2022

Holzhaus commented Sep 30, 2022

Holzhaus commented Sep 30, 2022 •

edited

Loading

daschuer commented Sep 30, 2022

Holzhaus commented Sep 30, 2022

daschuer commented Sep 30, 2022

Holzhaus commented Sep 30, 2022

daschuer commented Sep 30, 2022

Holzhaus commented Oct 20, 2022

Swiftb0y commented Oct 20, 2022

github-actions bot commented Jan 19, 2023

daschuer left a comment

[RFC] Migrate from qmlformat to qml_formatter #10929

[RFC] Migrate from qmlformat to qml_formatter #10929

Conversation

Holzhaus commented Sep 29, 2022

daschuer commented Sep 29, 2022 • edited Loading

Holzhaus commented Sep 29, 2022 • edited Loading

daschuer left a comment

Choose a reason for hiding this comment

daschuer commented Sep 30, 2022

Holzhaus commented Sep 30, 2022

Holzhaus commented Sep 30, 2022 • edited Loading

daschuer commented Sep 30, 2022

Holzhaus commented Sep 30, 2022

daschuer commented Sep 30, 2022

Holzhaus commented Sep 30, 2022

daschuer commented Sep 30, 2022

Holzhaus commented Oct 20, 2022

Swiftb0y commented Oct 20, 2022

github-actions bot commented Jan 19, 2023

daschuer left a comment

Choose a reason for hiding this comment

[RFC] Migrate from `qmlformat` to `qml_formatter` #10929

[RFC] Migrate from `qmlformat` to `qml_formatter` #10929

daschuer commented Sep 29, 2022 •

edited

Loading

Holzhaus commented Sep 29, 2022 •

edited

Loading

Holzhaus commented Sep 30, 2022 •

edited

Loading