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

DBus description: Add annotations for Qt support #3112

Merged
merged 2 commits into from Nov 8, 2021

Conversation

travier
Copy link
Member

@travier travier commented Sep 6, 2021

Add an initial set of simple annotations to help with interface code generation for Qt base applications.

@openshift-ci
Copy link

openshift-ci bot commented Sep 6, 2021

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@travier
Copy link
Member Author

travier commented Sep 6, 2021

The objective is to drop the copy we have in https://invent.kde.org/plasma/discover/-/blob/master/libdiscover/backends/RpmOstreeBackend/org.projectatomic.rpmostree1.xml with the annotations.

@travier
Copy link
Member Author

travier commented Sep 6, 2021

Looks like I have to figure out how to make it work for gdbus-codegen first:

[2021-09-06T18:10:02.103Z] gdbus-codegen \
[2021-09-06T18:10:02.103Z] 	--interface-prefix org.projectatomic.rpmostree1 \
[2021-09-06T18:10:02.103Z] 	--c-namespace RPMOSTree \
[2021-09-06T18:10:02.103Z] 	--c-generate-object-manager \
[2021-09-06T18:10:02.103Z] 	--generate-c-code rpm-ostreed-generated \
[2021-09-06T18:10:02.103Z] 	--generate-docbook rpm-ostreed-generated \
[2021-09-06T18:10:02.103Z] 	./src/daemon/org.projectatomic.rpmostree1.xml \
[2021-09-06T18:10:02.103Z] 	
[2021-09-06T18:10:02.103Z] Traceback (most recent call last):
[2021-09-06T18:10:02.103Z]   File "/usr/bin/gdbus-codegen", line 55, in <module>
[2021-09-06T18:10:02.103Z]     sys.exit(codegen_main.codegen_main())
[2021-09-06T18:10:02.103Z]   File "/usr/share/glib-2.0/codegen/codegen_main.py", line 406, in codegen_main
[2021-09-06T18:10:02.103Z]     parsed_ifaces = parser.parse_dbus_xml(
[2021-09-06T18:10:02.103Z]   File "/usr/share/glib-2.0/codegen/parser.py", line 302, in parse_dbus_xml
[2021-09-06T18:10:02.103Z]     parser = DBusXMLParser(xml_data, h_type_implies_unix_fd)
[2021-09-06T18:10:02.103Z]   File "/usr/share/glib-2.0/codegen/parser.py", line 58, in __init__
[2021-09-06T18:10:02.103Z]     self._parser.Parse(xml_data)
[2021-09-06T18:10:02.103Z]   File "/builddir/build/BUILD/Python-3.9.6/Modules/pyexpat.c", line 407, in StartElement
[2021-09-06T18:10:02.103Z]   File "/usr/share/glib-2.0/codegen/parser.py", line 151, in handle_start_element
[2021-09-06T18:10:02.103Z]     self._cur_object.annotations.append(anno)
[2021-09-06T18:10:02.103Z] AttributeError: 'NoneType' object has no attribute 'annotations'

@cgwalters
Copy link
Member

Looks like it's this stray annotation:

diff --git a/src/daemon/org.projectatomic.rpmostree1.xml b/src/daemon/org.projectatomic.rpmostree1.xml
index 49775a2c..9ae49a6d 100644
--- a/src/daemon/org.projectatomic.rpmostree1.xml
+++ b/src/daemon/org.projectatomic.rpmostree1.xml
@@ -74,8 +74,6 @@
     </property>
   </interface>
 
-  <annotation name="org.qtproject.QtDBus.QtTypeName" value="QVariantMap"/>
-
   <interface name="org.projectatomic.rpmostree1.OS">
     <property name="BootedDeployment" type="a{sv}" access="read">
         <annotation name="org.qtproject.QtDBus.QtTypeName" value="QVariantMap"/>

@travier travier force-pushed the dbus-qt-annotations branch 4 times, most recently from 71b30c0 to 92b13ee Compare September 8, 2021 11:22
cgwalters
cgwalters previously approved these changes Sep 8, 2021
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems OK to me; a bit surprising that the a{sv} case requires annotation.

@travier
Copy link
Member Author

travier commented Sep 8, 2021

I'll most likely do one small rename before we go with this (finding something better for: GetDeploymentsRpmDiffResult).

@cgwalters
Copy link
Member

I'll most likely do one small rename before we go with this (finding something better for: GetDeploymentsRpmDiffResult).

Rename of what?

/test all

@travier
Copy link
Member Author

travier commented Sep 8, 2021

I'll most likely do one small rename before we go with this (finding something better for: GetDeploymentsRpmDiffResult).

Rename of what?

There was a custom class specified in the annotation for the "result" received for some DBus calls. I switched it to a standard Qt class instead and it looks OK so far. I'm keeping this as a draft for now as I'm still working on the code using this and may find other improvements.

jlebon
jlebon previously approved these changes Sep 9, 2021
@@ -65,13 +69,21 @@
</method>

<!-- Array of all deployments in boot order -->
<property name="Deployments" type="aa{sv}" access="read"/>
<property name="Deployments" type="aa{sv}" access="read">
<annotation name="org.qtproject.QtDBus.QtTypeName" value="QList&lt;QVariantMap>"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the &lt; expected here? Should > also be changed to &gt;?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried using < and it breaks somewhere in the code generation. Using > is currently working. I've not tried using &gt;.

kdesysadmin pushed a commit to KDE/discover that referenced this pull request Sep 23, 2021
- Use standard Qt types for DBus interface
- Simplify the interface generation logic until we make a full
  abstraction for the rpm-ostree DBus interface.
- Rename the DBus interface generated code to a clearer name.
- Prepare for the removal of the DBus interface internal copy. We will
  be able to switch to upstream definition once the upstream PR
  coreos/rpm-ostree#3112 is merged.
Add an initial set of type annotations to help with automated interface
code generation for Qt based applications.
@travier
Copy link
Member Author

travier commented Oct 19, 2021

Rebased to trigger retest. I have not made any new changes on that one so far so I don't think there will be any soon even though I'm not done with rpm-ostree support in Discover. Let's get this in and we will see if more is needed later.

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about missing this one in the review queue!

I think we really need to set up a periodic group "PR review backlog" thing.

@cgwalters cgwalters merged commit e774569 into coreos:main Nov 8, 2021
@travier
Copy link
Member Author

travier commented Nov 9, 2021

All good. Thanks!

@travier travier deleted the dbus-qt-annotations branch November 9, 2021 08:30
kdesysadmin pushed a commit to KDE/discover that referenced this pull request Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants