Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable qmllint in qt6 builds. #4664

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Feb 5, 2022

I am still struggling to build Mixxx wit Qt 6 on ubuntu focal.
It turns out that the issues at runtime are with the qml files reported by qmllint.

Has One an idea how to fix it, or is the failure expected?

@Holzhaus
Copy link
Member

Holzhaus commented Feb 6, 2022

Has One an idea how to fix it, or is the failure expected?

Yes, it is expected. I didn't have time to fix every qmllint warning.

@Holzhaus
Copy link
Member

Holzhaus commented Feb 6, 2022

Btw, I don't think this should be merged. There is no reason to add the linter to the all target IMHO.

@daschuer
Copy link
Member Author

daschuer commented Feb 6, 2022

Any Idea why CI is not running? I will try a rebase.

@daschuer
Copy link
Member Author

daschuer commented Feb 6, 2022

Ci is working now:

Warning: res/qml/Mixxx/Controls/Knob.qml:113:21: Type not found in namespace
            value = Mixxx.MathUtils.clamp(value, 0, 1);
                    ^^^^^^^^^^^^^^^
Warning: res/qml/Mixxx/Controls/WaveformOverview.qml:12:5: Cannot assign to non-existent default property
    Mixxx.ControlProxy {
    ^^^^^
Warning: res/qml/Mixxx/Controls/WaveformOverview.qml: QQuickPaintedItem was not found. Did you add all import paths?

I can confirm the same issue here on Ubuntu Focal. Any ideas?

Do you know which parent in the QmlWaveformOverview class defines the "DefaultProterty"? It looks like qmllint does not find the parent QQuickPaintedItem which should be part of Qt6::Quick.

I expect the Mixxx.MathUtils issue to be a similar one.

@Holzhaus
Copy link
Member

Holzhaus commented Feb 6, 2022

I expect the Mixxx.MathUtils issue to be a similar one.

That issue is a Qt bug: https://bugreports.qt.io/browse/QTBUG-100326

@Holzhaus Holzhaus marked this pull request as draft February 12, 2022 16:13
@daschuer
Copy link
Member Author

What are the pros and cons to have qmllint running with the all target?

Compared to the qmllint during the pre commit hook, this one is called from the correct toolset and using the correct include paths. It only runs like a compiler whenever a qml file has been changed.

On the contra side we have the issue that it currently produces false positives that unnecessary fail a build.

I think If we like to rely on qmllint, we should make it happy all the time anyway.

Would it be an option to run the qmllint make target during the commit hook? Sounds scary ...

What do you think?

@daschuer
Copy link
Member Author

It turns out that the pre-commit hook calls qmllint from qt5 which produces false positives.
qmllint form qt6 needs to be called with the correct include paths. They are only known when building with QT6=1.
So I think it is just the right solution to run qmllint with the build process.

@daschuer
Copy link
Member Author

Arch CI failure is fixed here: #4680

@Holzhaus
Copy link
Member

So I think it is just the right solution to run qmllint with the build process.

This won't work. Only files that are built as qml modules are linted, but the majority of the qml code belongs to the skin which is not a module.

@daschuer
Copy link
Member Author

What would be the solution? I think it would be straight forward to add a module for the skin as well. We can skip it during linking and use the plain qml files when installing Mixxx. This way qt/cmake will take care that the correct call to qmllint is issued.

@github-actions
Copy link

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

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Jun 15, 2022
@daschuer
Copy link
Member Author

QT6 build fails now with:

Warning: /__w/mixxx/mixxx/res/qml/Mixxx/Controls/WaveformOverview.qml:10:13: Cannot assign binding of type QObject to mixxx::qml::QmlPlayerProxy
    player: Mixxx.PlayerManager.getPlayer(root.group)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Warning: /__w/mixxx/mixxx/res/qml/Mixxx/Controls/WaveformOverview.qml:33:18: Cannot assign binding of type double to bool
        visible: trackLoadedControl.value
                 ^^^^^^^^^^^^^^^^^^^^^^^^
make[2]: *** [CMakeFiles/mixxx-qml-mixxxcontrols_qmllint.dir/build.make:76: CMakeFiles/mixxx-qml-mixxxcontrols_qmllint] Error 255

is this expected?

@github-actions github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants