anaconda icon indicating copy to clipboard operation
anaconda copied to clipboard

libxklavier removal

Open jexposit opened this issue 1 year ago • 20 comments

RHEL 10 only! Please do not merge until the branch for RHEL 10 is created.


libxklavier is deprecated and X11 only. A Wayland alternative is required for RHEL 10.

While there is an ongoing conversation about enabling org.freedesktop.locale1 as a way to set the keyboard layout used by the compositor, not all compositors used by the different Fedora Spins support it.

This PR allows to use GNOME Kiosk's API to replace libxklavier in the environments were it is used.

jexposit avatar Feb 08 '24 14:02 jexposit

Hello @jexposit! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2024-05-28 15:10:18 UTC

pep8speaks avatar Feb 08 '24 14:02 pep8speaks

/build-image --boot.iso

jexposit avatar Feb 08 '24 15:02 jexposit

/build-image --live

jexposit avatar Feb 08 '24 15:02 jexposit

Images built based on commit e0f19bceef72399418c2512d49a5861bf4fa9062:

  • Live: failure

Download the images from the bottom of the job status page.

github-actions[bot] avatar Feb 08 '24 15:02 github-actions[bot]

Images built based on commit e0f19bceef72399418c2512d49a5861bf4fa9062:

  • boot.iso: failure

Download the images from the bottom of the job status page.

github-actions[bot] avatar Feb 08 '24 15:02 github-actions[bot]

@jexposit feel free to ignore Live ISO use case because RHEL doesn't support Live ISO so we don't have to care about that here.

jkonecny12 avatar Feb 08 '24 16:02 jkonecny12

What I wonder is if we can reduce the C code logic to a bare minimum and change the widgets to just a set/get/signal code without doing the switching directly. Would it be doable?

That was something I was also thinking about originally, but then I realized it makes more sense for Jose to do it like this dues to his strong C background + its closer to how the widget worked originally.

The main issues could be with maintenance for use (with our GTK/C API knowledge level), but maybe it will be fine for the expected remaining GTK GUI lifetime ?

Edit: Though looking at the C code in detail, it uses the Gnome Kiosk API directly, so while perfectly fine for RHEL 10, this would have to be changed if we want to have keyboard switching working again in Fedora Live images with their different compositors.

M4rtinK avatar Feb 08 '24 16:02 M4rtinK

Looks like boot boot.iso and Live image generation fails due to some issues with the C widget compilation:

make[7]: Entering directory '/tmp/anaconda/widgets/src'
gdbus-codegen \
--interface-prefix org.gnome. \
--c-namespace Gk \
--generate-c-code gk-kiosk \
--output-directory . \
/usr/local/share/dbus/org.gnome.Kiosk.xml
Traceback (most recent call last):
  File "/usr/bin/gdbus-codegen", line 55, in <module>
    sys.exit(codegen_main.codegen_main())
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/share/glib-2.0/codegen/codegen_main.py", line 419, in codegen_main
    with open(fname, "rb") as f:
         ^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: '/usr/local/share/dbus/org.gnome.Kiosk.xml'

I see that file included in the PR, but it seems to be unable to locate it at build time ?

I don't see the new file listed in a makefile & in spec file, so it might not actually be shipped in the tarball & this is why its missing at build time.

M4rtinK avatar Feb 08 '24 18:02 M4rtinK

/build-image --boot.iso

jexposit avatar Feb 09 '24 09:02 jexposit

The unit test task is failing with error:

LayoutIndicator.c:31:10: fatal error: gk-kiosk.h: No such file or directory
   31 | #include "gk-kiosk.h"
      |          ^~~~~~~~~~~~
compilation terminated.

It looks like gdbus-codegen is not being invoked. However, when I run it locally, it is generating the right files in /tmp/anaconda/widgets/src.

Am I missing an extra step?

jexposit avatar Feb 09 '24 10:02 jexposit

Images built based on commit 72f4d07893a2bf7d8161dd8db39f6add7ad453e7:

  • boot.iso: success

Download the images from the bottom of the job status page.

github-actions[bot] avatar Feb 09 '24 10:02 github-actions[bot]

It looks like gdbus-codegen is not being invoked. However, when I run it locally, it is generating the right files in /tmp/anaconda/widgets/src.

Am I missing an extra step?

I'll try to reproduce the issues locally & see whats going on. :)

M4rtinK avatar Feb 09 '24 11:02 M4rtinK

Weird, doing make -f ./Makefile.am container-rpms-scratch does build & I end up with successfully built RPM. So I tried to restart the failed unit tests, but now it seems to hit a merge conflict apparently ?

Run git config user.name github-actions
c949c03b27 Merge pull request #5464 from poncovka/master-session_bus
Auto-merging anaconda.py
CONFLICT (content): Merge conflict in anaconda.py
Auto-merging pyanaconda/display.py
CONFLICT (content): Merge conflict in pyanaconda/display.py
Rebasing (1/6)
error: could not apply 3baee6de49... Set up the default session bus
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 3baee6de49... Set up the default session bus
Error: Process completed with exit code 1.

@jexposit

M4rtinK avatar Feb 09 '24 14:02 M4rtinK

Weird, doing make -f ./Makefile.am container-rpms-scratch does build & I end up with successfully built RPM. So I tried to restart the failed unit tests, but now it seems to hit a merge conflict apparently ?

Yes, I cherry-picked a commit from Vendula (https://github.com/rhinstaller/anaconda/pull/5464) and now that it is merged there was a conflict. Rebased.

jexposit avatar Feb 09 '24 14:02 jexposit

/build-image --boot.iso

jexposit avatar Feb 13 '24 19:02 jexposit

In addition to the changes requested, I added a few fixes for bugs I noticed during testing.

In addition to the UI, I tested the inst.keymap= boot option and I didn't noticed any difference from the master branch.

I wasn't able to make the keyboard kickstart option work. Both in the master branch and in this branch it doesn't affect the UI or the installed system. My code changes shouldn't affect this option, but it'd nice to investigate why it is not working (or why I'm not being able to make it work).

I'm pretty happy with this revision of the code, I think it's ready for another round of review/testintg.

jexposit avatar Feb 13 '24 19:02 jexposit

Images built based on commit 46ac1ea93c98bfe97410cdeb74627a48e3b53af4:

  • boot.iso: success

Download the images from the bottom of the job status page.

github-actions[bot] avatar Feb 13 '24 19:02 github-actions[bot]

I've done some testing to compare behavior of regular Rawhide Anaconda and this PR, using this kickstart:

graphical
halt
lang cs_CZ.UTF-8
keyboard cz
timezone America/New_York
rootpw anaconda
bootloader --location=mbr
zerombr
clearpart --all --initlabel
autopart

%packages
bash
%end

Regular rawhide

kickstart_keyboard_rawhide_boot boot options

kickstart_keyboard_rawhide installation running, note keyboard badge in upper right

kickstart_keyboard_rawhide_instalation_localectl localectl output in installation env

kickstart_keyboard_rawhide_localectl localectl output in installed system

Rawhide with this PR applied

kickstart_keyboard_pr_boot boot options

kickstart_keyboard_pr installation running, note keyboard badge in upper right

kickstart_keyboard_pr_localectl localectl output in installation env

kickstart_keyboard_pr_instalation_localectl localectl output in installed system

So as far as I can tell keyboard configuration from kickstart works the same with the PR as before, which is the desired state - eq. things working the same, but without the obsolete libXklavier dependency. :)

M4rtinK avatar Feb 14 '24 12:02 M4rtinK

Using the inst.keymap=cz boot option behaves exactly the same as with kickstart. :)

M4rtinK avatar Feb 14 '24 14:02 M4rtinK

Can you mark this as a draft PR then?

Conan-Kudo avatar Feb 21 '24 12:02 Conan-Kudo

/kickstart-test --skip-testtypes knownfailure,manual,skip-on-fedora,gh576,gh595,gh640,gh641,gh680,gh740,gh769,gh774,gh777,gh910,gh890,gh871,rhbz1853668,gh975,gh1023

M4rtinK avatar Apr 05 '24 13:04 M4rtinK

FYI, --locale1 is now supported in GNOME Kiosk.

In the end is was implemented as a fallback rather than a extra command line option: When there is no session configuration, the system configuration is used instead.

We are a bit late in the Fedora 40 cycle to change things, but hopefully for Fedora 41, all compositors should be ready to support a mechanism to set locale configuration from org.freedesktop.locale1.

jexposit avatar Apr 05 '24 13:04 jexposit

FYI, --locale1 is now supported in GNOME Kiosk.

Nice! :)

In the end is was implemented as a fallback rather than a extra command line option: When there is no session configuration, the system configuration is used instead.

We are a bit late in the Fedora 40 cycle to change things, but hopefully for Fedora 41, all compositors should be ready to support a mechanism to set locale configuration from org.freedesktop.locale1.

Yep, makes sense to use this API exclusively on Fedora if possible. :)

Currently I'm a bit preoccupied assuring landing of this version of the Wayland migration patch set to RHEL goes smoothly, but in the near future I plan to start a discussion (kinda like @jkonecny12 recently did for the Web UI stuff) about how to best handle the Wayland migration on Fedora. :)

M4rtinK avatar Apr 05 '24 16:04 M4rtinK

If we want to get it into F41 which seems we want. I would suggest to just create a system wide change for F41 which is going to start the discussion.

jkonecny12 avatar Apr 05 '24 16:04 jkonecny12

At least to me, the important thing is that we have a framework in Anaconda so that I can slot in KWin or Weston instead of GNOME Kiosk.

Conan-Kudo avatar Apr 05 '24 16:04 Conan-Kudo

Also, could we make sure these improvements are part of RHEL 10 Anaconda?

Conan-Kudo avatar Apr 05 '24 16:04 Conan-Kudo

If we want to get it into F41 which seems we want. I would suggest to just create a system wide change for F41 which is going to start the discussion.

Yeah, might as well do that. :)

At least to me, the important thing is that we have a framework in Anaconda so that I can slot in KWin or Weston instead of GNOME Kiosk.

I think the PR ended up in a way that it could be possible to support more compositors. In general, it needs to keep support for what we need on RHEL, which is GNOME Kiosk as Wayland compositor and GNOME Remote Desktop for RDP access.

Also, could we make sure these improvements are part of RHEL 10 Anaconda?

Indeed, it would be ideal from code commonality PoV to have the Wayland support code to be similar between Fedora and RHEL. But we will see if we can eventually pull it off given the various timelines and requirements of both projects. :P

M4rtinK avatar Apr 08 '24 14:04 M4rtinK

/kickstart-test --skip-testtypes knownfailure,manual,skip-on-fedora,gh576,gh595,gh640,gh641,gh680,gh740,gh769,gh774,gh777,gh910,gh890,gh871,rhbz1853668,gh975,gh1023

M4rtinK avatar Apr 09 '24 16:04 M4rtinK

Failed to load packit config file:

Cannot parse package config. TypeError("'NoneType' object is not iterable")

For more info, please check out the documentation or contact the Packit team. You can also use our CLI command validate-config or our pre-commit hooks for validation of the configuration.