Add qm-wayland package #934
Conversation
aesteve-rh
requested review from
Yarboa,
dougsland,
kleinffm,
nsednev,
pengshanyu and
rhatdan
as code owners
Reviewer's GuideThis PR restructures the QM project by introducing a new qm-wayland RPM package (including spec, packaging scripts, Makefile targets, and documentation), extending Packit pipelines for the new packages, enhancing the qm-oci-hooks package (adding a seat manager hook and cleaning up rootfs path logic), updating associated build and test tooling, and removing the deprecated windowmanager subsystem. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @aesteve-rh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This PR introduces the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The spec file’s conditional logic around copr_username and Epoch adds unnecessary complexity; consider simplifying the version/Epoch handling for consistency across build environments.
- The Makefile’s dist target hardcodes paths for each file, which is brittle; switch to wildcard patterns or a manifest to dynamically include subsystem files.
- There’s an unintended empty diff entry for tools/version-update — please remove or properly populate this change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The spec file’s conditional logic around copr_username and Epoch adds unnecessary complexity; consider simplifying the version/Epoch handling for consistency across build environments.
- The Makefile’s dist target hardcodes paths for each file, which is brittle; switch to wildcard patterns or a manifest to dynamically include subsystem files.
- There’s an unintended empty diff entry for tools/version-update — please remove or properly populate this change.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new qm-wayland package to replace the old windowmanager subpackage. The changes are extensive, including a new RPM spec, systemd units, PAM configurations, and a session management script. My review focuses on ensuring the correctness and robustness of these new components. I've found several critical issues, including a potential packaging bug in the Makefile, a CPU-spinning bug in the session script, and a typo that would prevent the OCI hook from working correctly. I've also noted some areas for improvement in script efficiency and RPM packaging practices. Please address the critical and high severity issues to ensure the package works as intended.
subsystems/wayland/etc/container/systemd/wayland-compositor.container
Outdated
Show resolved
Hide resolved
aesteve-rh
force-pushed
the
upstream-wayland
branch
from
279e80a to
6583fc4
Compare
|
Failed to load packit config file: For more info, please check out the documentation or contact the Packit team. You can also use our CLI command |
aesteve-rh
force-pushed
the
upstream-wayland
branch
3 times, most recently
from
ff6f766 to
23e537a
Compare
|
@aesteve-rh we have that issue, #921 |
|
This is not a qm subpackage in the same sense the others that were discussed previously. In this case, there is no direct dependency between both packages in the specfile. The tweak is that this is a package meant to be installed within the qm container, adding some pre-defined workloads to support a scenario. This was discussed with @alexlarsson too, he suggested following this approach. I have to link the commit to the issue, true. I am waiting for the |
MHO, go with what exist, later we could change things gradually, |
Fair point. I will try to do that for now. |
aesteve-rh
force-pushed
the
upstream-wayland
branch
8 times, most recently
from
1b62cd1 to
67c61f4
Compare
|
@sourcery-ai summary |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new Packit pipeline entries for qm-wayland and qm-oci-hooks are very similar—consider extracting common anchors or jobs and overriding only identifier/specfile_path to reduce duplication.
- Rather than having two nearly identical
.packit-*.shscripts, you could factor out the tarball creation and spec‐file sed logic into a shared helper to avoid duplicated versioning and source‐setup code. - The special-case seat-manager executable lookup in the tests would be cleaner if you extended the hook_configs fixture or parametrized the mapping, so you don’t need ad-hoc branches inside the test body.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new Packit pipeline entries for qm-wayland and qm-oci-hooks are very similar—consider extracting common anchors or jobs and overriding only identifier/specfile_path to reduce duplication.
- Rather than having two nearly identical `.packit-*.sh` scripts, you could factor out the tarball creation and spec‐file sed logic into a shared helper to avoid duplicated versioning and source‐setup code.
- The special-case seat-manager executable lookup in the tests would be cleaner if you extended the hook_configs fixture or parametrized the mapping, so you don’t need ad-hoc branches inside the test body.
## Individual Comments
### Comment 1
<location> `rpm/wayland/qm-wayland.spec:51` </location>
<code_context>
+
+%build
+
+%install
+# Install wayland script
+install -d %{buildroot}%{_bindir}
</code_context>
<issue_to_address>
**issue (complexity):** Consider grouping repeated install and macro commands to reduce boilerplate in the spec file.
Here’s one way you can collapse the boilerplate without changing any functionality:
1) Group all your directory-creates into one `install -d`:
```spec
%install
# install binaries
install -d %{buildroot}%{_bindir}
install -m 755 subsystems/wayland/usr/bin/wayland-session %{buildroot}%{_bindir}/
# create all config & unit dirs in one go
install -d \
%{buildroot}%{_sysconfdir}/containers/systemd \
%{buildroot}%{_sysconfdir}/containers/systemd/qm-dbus-broker.container.d \
%{buildroot}%{_sysconfdir}/containers/systemd/wayland-compositor.container.d \
%{buildroot}%{_unitdir} \
%{buildroot}%{_unitdir}/wayland-session.service.d \
%{buildroot}%{_sysconfdir}/pam.d \
%{buildroot}%{_sysconfdir}/weston \
%{buildroot}%{_presetdir}
```
2) Group your `-m 644` copies by destination:
```spec
# container files
install -m 644 \
subsystems/wayland/etc/containers/systemd/qm-dbus-broker.container \
subsystems/wayland/etc/containers/systemd/wayland-compositor.container \
%{buildroot}%{_sysconfdir}/containers/systemd/
# systemd units
install -m 644 subsystems/wayland/etc/systemd/system/* %{buildroot}%{_unitdir}/
# pam, weston, preset
install -m 644 \
subsystems/wayland/etc/pam.d/* \
%{buildroot}%{_sysconfdir}/pam.d/
install -m 644 subsystems/wayland/etc/weston/weston.ini %{buildroot}%{_sysconfdir}/weston/
install -m 644 subsystems/wayland/50-qm-wayland.preset %{buildroot}%{_presetdir}/
```
3) Collapse your four systemd macros into one each:
```spec
%post
%systemd_post qm-dbus.socket \
wayland.socket \
wayland-session.service \
wayland-compositor.service
%preun
%systemd_preun qm-dbus.socket \
wayland.socket \
wayland-session.service \
wayland-compositor.service
%postun
%systemd_postun_ex qm-dbus.socket \
wayland.socket \
wayland-session.service \
wayland-compositor.service
```
This preserves all paths and unit handling but cuts ~20 lines of boilerplate.
</issue_to_address>
### Comment 2
<location> `oci-hooks/tests/test_hook_configs.py:246-248` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 3
<location> `oci-hooks/tests/test_hook_configs.py:406-410` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
aesteve-rh
force-pushed
the
upstream-wayland
branch
2 times, most recently
from
c750d85 to
84c35ee
Compare
|
@Yarboa @dougsland @telemaco ready to be reviewed! |
|
Can already be tested with https://gitlab.com/CentOS/automotive/src/automotive-prototyper/-/merge_requests/261. |
Yarboa
left a comment
Yarboa
left a comment
There was a problem hiding this comment.
Sorry small leftovers, from previous review, I did not submit
aesteve-rh
force-pushed
the
upstream-wayland
branch
3 times, most recently
from
d44f103 to
7a5b183
Compare
aesteve-rh
force-pushed
the
upstream-wayland
branch
2 times, most recently
from
066e73a to
772c692
Compare
|
@aesteve-rh ready for review? |
|
@dougsland yes, there is still a bit of refinement with systemd orchestration going on, but that will only change a few lines in the quadlets, sockets and service files for wayland. Which should not block reviewing this overall. I know it's many changes, I tried to split in commits to make it easier to review. |
aesteve-rh
force-pushed
the
upstream-wayland
branch
from
772c692 to
5c237e3
Compare
|
@dougsland @Yarboa refinement done. This works in my setup. Feel free to review it already. I will take the changes from #942 once it lands. |
Sure, I am back, reviewing |
aesteve-rh
force-pushed
the
upstream-wayland
branch
from
5c237e3 to
f6708dd
Compare
|
Pushed mostly to include the container support in 6a06e95 That was missing even though I remember adding it at some point. Shouldn't change other parts of the patch. |
Remove windowmanager subsystem package. We want to revamp the scenario and allow the package to be installed within QM instead of extending QM. Signed-off-by: Albert Esteve <[email protected]>
Add qm-wayland package. This is a revamp of the previous windowmanager subpackage. But in this case, it is not a qm subpackage in the sense of requiring QM and installing in rootfs. Instead, this qm-wayland package is meant to be installed within QM. It adds the files required to run the wayland scenario, and depends on container images maintained on quay.io. This is the first draft, but it requires the dev-binder-hook to function correctly. Signed-off-by: Albert Esteve <[email protected]>
Signed-off-by: Albert Esteve <[email protected]>
Signed-off-by: Albert Esteve <[email protected]>
Make qm-oci-hooks package indepent from qm. It can then be installed both in the host or within the qm to get the hooks installed in the right folders. As it is indepent we can now add it to the Packit configuration so it gets built and deployed automatically. Signed-off-by: Albert Esteve <[email protected]>
Split seat and devices hooks as otherwise the annotations were not matching correctly to execute the hooks when they should. Also, fix the package to add a missing dependency to include udevadm application. Signed-off-by: Albert Esteve <[email protected]>
Now that hooks is an independent package, qm-wayland can recomment it to work properly. Signed-off-by: Albert Esteve <[email protected]>
Exclude packages that were added to packit from the subpackages target. That will exclude also from the `Build Subpackages` job in the pipelines, as the build environment have missing dependencies. Signed-off-by: Albert Esteve <[email protected]>
Signed-off-by: Albert Esteve <[email protected]>
aesteve-rh
force-pushed
the
upstream-wayland
branch
from
f6708dd to
cae811f
Compare
|
I've been reviewing the PR @aesteve-rh and LGTM. |
4 participants
Add qm-wayland package. This is a revamp of the
previous windowmanager subpackage. But in this case,
it is not a qm subpackage in the sense of requiring QM
and installing in rootfs. Instead, this qm-wayland
package is meant to be installed within QM. It adds
the files required to run the wayland scenario, and
depends on container images maintained on quay.io.
This is the first draft, but it requires the
dev-binder-hook to function correctly.
Signed-off-by: Albert Esteve [email protected]
Summary by Sourcery
Split out the previous windowmanager subpackage into two independent RPMs (qm-wayland and qm-oci-hooks) with their own Packit pipelines, refactor specs and file layouts, update tests, and add documentation for the Wayland subsystem.
New Features:
Enhancements:
Documentation:
Tests:
Chores: