-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Qml modules #13304
Qml modules #13304
Conversation
5a309af
to
2c9b9eb
Compare
arch tests are failing because the CI doesn't provide QtQml for qt 6 which is a new requirement for the test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! I've taken a cursory glance, and there's a few smallish things I've noticed. I'd like to have another look later when I have more time to get into more of the details
mesonbuild/modules/_qt.py
Outdated
for check in checklist: | ||
if check not in choices: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can simplify (and make a better error message) this to something like:
invalid = set(checklist).difference(choices)
if invalid:
return f"invalid selections {', '.join(sorted(invalid))}, valid elements are {', '.join(sorted(choices))}."
mesonbuild/modules/_qt.py
Outdated
preserve_paths: bool | ||
|
||
class HasToolKwArgs(kwargs.ExtractRequired): | ||
|
||
method: str | ||
tools: T.List[str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can make this a little more powerful (and maybe remove an assert later on) with:
tools: T.List[Literal['moc, 'lrelease', <insert reset of valid tools>]]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
mesonbuild/modules/_qt.py
Outdated
else: | ||
assert False, "unreachable" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to trust the type annotations for the final release, though this might be useful for bringup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this function quite a bit, I took inspiration from the one in gnome.py
mesonbuild/modules/_qt.py
Outdated
basename: str = os.path.basename(sourcepath) | ||
classname: str = basename.rsplit('.', maxsplit=1)[0] | ||
|
||
if not any(basename.endswith(ext) for ext in ['.qml', '.js', '.mjs']): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not any(basename.endswith(ext) for ext in ['.qml', '.js', '.mjs']): | |
if not basename.endswith(('.qml', '.js', '.mjs')): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
mesonbuild/modules/_qt.py
Outdated
if not any(basename.endswith(ext) for ext in ['.qml', '.js', '.mjs']): | ||
raise MesonException(f'unexpected file type declared in qml sources {s}') | ||
|
||
if len(classname) == 0 or '.' in classname or classname[0].islower(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if len(classname) == 0 or '.' in classname or classname[0].islower(): | |
if not classname or '.' in classname or classname[0].islower(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
mesonbuild/modules/_qt.py
Outdated
|
||
cmd.extend(kwargs['extra_args']) | ||
|
||
if namespace != '': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if namespace != '': | |
if namespace: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
mesonbuild/modules/_qt.py
Outdated
description=f'Qml type registration for {target_name}', | ||
) | ||
|
||
@FeatureNew('qt.qml_module', '1.0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FeatureNew('qt.qml_module', '1.0') | |
@FeatureNew('qt.qml_module', '1.6') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
mesonbuild/modules/_qt.py
Outdated
qrc_resouces: T.List[T.Union[FileOrString, build.CustomTarget, build.CustomTargetIndex, build.GeneratedList]] = [] | ||
all_qml_files: T.Sequence[T.Union[FileOrString, build.CustomTarget]] = list(kwargs['qml_sources']) + list(kwargs['qml_singletons']) + list(kwargs['qml_internals']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it would be beneficial to use the Interpreter.source_strings_to_files
method here to remove the strings and just have list[File | CustomTarget | CustomTargetIndex | GeneratedList]
here, and then not have to do string handling internally.
some self comments on the PR: I tried to follow Qt module directory structure
at the moment this is still a bit clumpsy:
|
Thank you for undertaking this work ! I wanted to give a try to registering QML modules but I am afraid I've hit against a regression introduced in this MR before I could get to it. Here's the reproducer: git clone --depth=1 https://github.com/AdelKS/ZeGrapher.git
cd ZeGrapher
meson setup build
cd build
meson compile which outputs this error
it looks like there's a naming convention that isn't respected between Or maybe it's related to the fact that I currently pass all headers that need a |
Hi, thanks for your interest
your repository doesn't use the feature from this MR, so I don't really see how this is a regression. I tried to compile you project but I ended up with a lot of compilation issues (template issues in your math classes, compiled on ubuntu 22.04 with GCC 11.4), So I can't really help you there
qt.qml_module will moc the file itself as it needs to pass specific options to moc to extract type information, so |
Hey, thanks for trying to look into it
Yeah I stopped trying to make "old" GCC or clang versions happy x)
Sorry, I haven't been clear enough: the master branch of my repo builds fine with latest release of meson 1.5.0, but not with this branch, this is before I make changes to
I understand. I have a noob suggestion on this aspect: I have this (maybe wrong ?) approach where I use a bash script to make a meson |
Okay I just tried on meson commit Would you be willing to install add-apt-repository -y ppa:ubuntu-toolchain-r/test
apt install -y gcc-13
git clone --depth=1 https://github.com/AdelKS/ZeGrapher.git
cd ZeGrapher
CXX=g++13 meson setup build
cd build
meson compile |
`outfiles` is the output list of the individual commands Bug: mesonbuild/pull/13304#issuecomment-2226398671
`outfilelist` is the output list of the target, while `outfiles` is the output list of the individual commands Bug: mesonbuild/pull/13304#issuecomment-2226398671
`outfilelist` is the output list of the target, while `outfiles` is the output list of the individual commands Bug: mesonbuild/pull/13304#issuecomment-2226398671
I updated the PR, it no longer requires the sources to be generated with a precise directory structure which should match meson philosophy. I also added a 'devel' install target to allow installing the module definition (for qmllint or and qml tools) |
3e31346
to
e8e8465
Compare
`outfilelist` is the output list of the target, while `outfiles` is the output list of the individual commands Bug: /pull/13304#issuecomment-2226398671
mesonbuild/modules/_qt.py
Outdated
module_prefix_full: str = os.path.join(*(kwargs['resources_prefix'].split('/') + module_prefix_list)) | ||
|
||
qrc_resouces: T.List[T.Union[FileOrString, build.GeneratedTypes]] = [] | ||
all_qml: T.Sequence[T.Union[FileOrString, build.GeneratedTypes]] = list(kwargs['qml_sources']) + list(kwargs['qml_singletons']) + list(kwargs['qml_internals']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't these already lists? Why cast them again to list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mypy fails otherwise
Unsupported left operand type for + ("Sequence[File | str | CustomTarget | CustomTargetIndex | GeneratedList]")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the original definition of kwargs should annotate these as T.List instead of the generic T.Sequence, if we need them to be a List type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it for a T.List, this seems to be enough
The next release is creeping closer so it would be nice to get this in. FWICT there are only some minor fixes + a rebase and this should be good to merge. |
I updated documentation to reflect @klokik & @AdelKS remarks
maybe some missing includes/dep ? moc tend to to be lenient about this. Q_MOC_INCLUDE may help about this also don't you need public inheritance rather than default (private)
not in short term, I'm more interested in providing shader compilation (qsb) support |
Thanks! Tried this
#pragma once
#include <QSyntaxHighlighter>
#include <QQuickTextDocument>
#include "Utils/state.h"
/// @brief backend that highlights parsing errors in math expressions in LineEdit
class Highlighter: public QSyntaxHighlighter
{
Q_OBJECT
QML_ELEMENT
// ...
Q_MOC_INCLUDE("QSyntaxHighlighter") same warning message, I don't know if this is a
That will also be very cool ! I may try to get the |
9876b77
to
bf2866c
Compare
@klokik I added the option I also renamed the |
Cygwin failure is unrelated. Dunno what is up with the image builder there, but the Arch failure at least is understandable. If that test needs to be skipped, then it needs to be whitelisted in the code (I don't remember exactly where, but grepping for the error message should find it). |
I tried to restart the cygwin test because it is supposed to be fixed now. Got an alternative fascinating error: https://github.com/mesonbuild/meson/actions/runs/12619860632/job/35164883111 works
https://github.com/mesonbuild/meson/actions/runs/12579978087/job/35171502458 fails
@jon-turney this seems odd somehow... By the way the vala error triggered by gcc 15 (really, gcc 14 is sufficient to trigger this) was fixed in https://gitlab.gnome.org/GNOME/vala/-/commit/1383ab9f78f81b4ef56d63b38df702253ac8469b But again, there is an out of date cygwin package. In this case, vala. Vala has some fixes for its terrible codegen, which has a general habit of breaking badly on the new GCC 14 defaults for "Modern C", but vala in cygwin is orphaned (again) and hasn't been updated since 2018... |
Indeed. I merged some PRs to the GitHub cygwin-install-action today, which I'm guessing is the cause of suddenly (and incorrectly) pulling in test packages... I think I've fixed it now. Sorry for the inconvenience.
Yeah. |
I cherry picked the CI commit to master. If you remove that and rebase, most of the errors should go away. |
something doesn't looks right with the fedora/suse docker images if i fetch the lastest docker image, the qt6 pacakges doesn't appeared as installed
if I install manually the packages (list taken from the install.sh) from there then it works as expected
regarding the unity build failure, the generated code can't be used with unity build, this will generate duplicate symbols, I'll override the unity flag in the tests |
well, master CI did fail, this explain why the image were not updated https://github.com/mesonbuild/meson/actions/runs/12642215836/job/35226093290 |
Yup, same problem that you would have seen happening in the image builder jobs from this PR. We try to rebuild images weekly, and only deploy them if tests still pass -- getting them to pass is then handled on a case by case basis when it fails, assuming the reason for failure is not transient, but actually requires updates for newer versions of that distro. So the general CI should always keep passing on git master and failures should always be because of issues introduced in a PR, not issues introduced by updating the environment. You can either try to fix those issues, or have this PR assume that the new test will be skipped for lack of qtdeclarative. If/when the errors go away we will update the test to stop being skipped. |
Master is green, so rebase should fix it? |
in `func_install_data`, `rename` parameter is `ContainerTypeInfo(list, str)` in `module/python.py`, rename is `None` `rename` is stored in `build.Data` object under the type `T.List[str]`
This aims to bring the support of QML modules to meson, the goal is to provide something similar to CMake `qt_add_qml_module` function provided by Qt (see https://doc.qt.io/qt-6/qt-add-qml-module.html ) Fixes: mesonbuild#6988, mesonbuild#9683
thanks for the fix, though the opensuse docker image doesn't seems updated yet
I guess I need to wait a couple of day, however the tests on fedora are passing now 🤔 |
version: '1.0', | ||
qml_sources: files('Basic.qml', 'subdir/Thing.qml'), | ||
qml_singletons: files('QmlSingleton.qml'), | ||
qml_internals: files('Internal.qml'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these are done with files()
. We should also support doing this:
qml_sources: ['Basic.qml', 'subdir/Thing.qml']
so some of the tests should be changed to this form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed a test to use sources as string an one to use generated targets
As I said, "have this PR assume that the new test will be skipped". There is no point waiting a couple of days. When I said "We try to rebuild images weekly" that was totally unrelated (we do that in addition to rebuilding them every time the image configs change, which you did here, and image failures are NOT a CI failure, so you must verify that your changes build successfully in this PR or not depend on them). Your test assumptions are incompatible with the OpenSUSE image until its unrelated failure is fixed, which means we need to mark it as skipped. This PR was merged with failing CI @jpakkane |
I opened #14107 for this |
This MR aims to bring the support of QML modules to meson, the goal is to
provide something similar to CMake
qt_add_qml_module
function provided by Qt(see https://doc.qt.io/qt-6/qt-add-qml-module.html)
fixes: #6988 #9683
what's provided
the QML module
what's missing:
the scope of this PR is already large, I voluntary put aside some aspect of the module generation.
tools (or options) are not available with older version of Qt,
specific Qml files, which is not very practical in terms of API
tooling, I haven't investigated this
VERSION
PAST_MAJOR_VERSIONS
STATIC / SHARED
PLUGIN_TARGET
OUTPUT_DIRECTORY
RESOURCE_PREFIX
CLASS_NAME
TYPEINFO
IMPORTS
OPTIONAL_IMPORTS
DEFAULT_IMPORTS
DEPENDENCIES
IMPORT_PATH
SOURCES
QML_FILES
RESOURCES
OUTPUT_TARGETS
DESIGNER_SUPPORTED
FOLLOW_FOREIGN_VERSIONING
NAMESPACE
NO_PLUGIN
NO_PLUGIN_OPTIONAL
NO_CREATE_PLUGIN_TARGET
NO_GENERATE_PLUGIN_SOURCE
NO_GENERATE_QMLTYPES
NO_GENERATE_QMLDIR
NO_LINT
NO_CACHEGEN
NO_RESOURCE_TARGET_PATH
NO_IMPORT_SCAN
ENABLE_TYPE_COMPILER
TYPE_COMPILER_NAMESPACE
QMLTC_EXPORT_DIRECTIVE
QMLTC_EXPORT_FILE_NAME