-
Notifications
You must be signed in to change notification settings - Fork 355
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
libxklavier removal #5463
libxklavier removal #5463
Conversation
afea075
to
e0f19bc
Compare
/build-image --boot.iso |
/build-image --live |
Images built based on commit e0f19bc:
Download the images from the bottom of the job status page. |
Images built based on commit e0f19bc:
Download the images from the bottom of the job status page. |
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.
First batch from my review. Not finish yet.
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 really like this a lot! Thanks for this.
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?
@jexposit feel free to ignore |
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. |
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.
Agreed with Jirkas observations (docs, string formatting, etc.) & otherwise looks good to me as well. :) Next I'll again download the boot.iso & Live image and try to see how it looks like in real live.
Looks like boot boot.iso and Live image generation fails due to some issues with the C widget compilation:
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. |
e0f19bc
to
72f4d07
Compare
/build-image --boot.iso |
The unit test task is failing with error:
It looks like Am I missing an extra step? |
Images built based on commit 72f4d07:
Download the images from the bottom of the job status page. |
I'll try to reproduce the issues locally & see whats going on. :) |
Weird, doing
|
72f4d07
to
f0b7094
Compare
/kickstart-test --skip-testtypes knownfailure,manual,skip-on-rhel,skip-on-rhel-10,gh576,gh640,gh804,gh1090,gh1104,gh1106,gh1105,gh1108,gh1109,gh1107,gh1110 |
1 similar comment
/kickstart-test --skip-testtypes knownfailure,manual,skip-on-rhel,skip-on-rhel-10,gh576,gh640,gh804,gh1090,gh1104,gh1106,gh1105,gh1108,gh1109,gh1107,gh1110 |
6d73f18
to
33ca39e
Compare
Rebased on latest RHEL 10 branch & dropped the adwaita-icon-theme and gnome-kiosk package rebuilds from the COPR. Lets see if everything still works with latest RHEL 10 repo. |
/kickstart-test --testtype smoke |
/kickstart-test --skip-testtypes knownfailure,manual,skip-on-rhel,skip-on-rhel-10,gh576,gh640,gh804,gh1090,gh1104,gh1106,gh1105,gh1108,gh1109,gh1107,gh1110 |
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.
LGTM!
Add a class wrapping GNOME Kiosk's input sources API that future commits will use via the localization service to replace the X11 keyboard management logic. Resolves: RHEL-38399
33ca39e
to
30dface
Compare
Rebased PR on latest rhel-10 branch & added correct Jira references. Also dropped python-pam from the temporary COPR repo, as it should be finally in the RHEL 10 repos. |
libxklavier is deprecated and X11-only. On RHEL, the GNOME Kiosk API can be used instead to handle the keyboard configuration. In order to make the code migration as simple as possible, keep the XklWrapper class and update its implementation to use GNOME Kiosk's API via the localization service. Resolves: RHEL-38399
Invoke gdbus-codegen as part of the make build and makeupdates script to generate C code from org.fedoraproject.Anaconda.Modules.Localization.xml. Resolves: RHEL-38399
This C widget is the last place were libxklavier is used. Use GNOME Kiosk's API via the localization service instead. Resolves: RHEL-38399
94d261b
to
bead3ba
Compare
Resolves: RHEL-38399
bead3ba
to
ce6cc95
Compare
/kickstart-test --skip-testtypes knownfailure,manual,skip-on-rhel,skip-on-rhel-10,gh576,gh640,gh804,gh1090,gh1104,gh1106,gh1105,gh1108,gh1109,gh1107,gh1110 |
So in kickstart tests for this PR, I found one interesting failure twice: In
Again in
Then there is another apparent race condition in storage (also seen in different kickstart test in last full kstestrun on the xrandr PR) in
|
@jexposit Given that |
/kickstart-test --skip-testtypes knownfailure,manual,skip-on-rhel,skip-on-rhel-10,gh576,gh640,gh804,gh1090,gh1104,gh1106,gh1105,gh1108,gh1109,gh1107,gh1110 |
@jexposit @halfline So it looks like the issue is with DBus call to The method is calld in two places: and
anaconda/pyanaconda/ui/gui/xkl_wrapper.py Line 238 in ce6cc95
So one of those triggers the race condition & crash. So what does the
This is where it gets interesting -
So it looks like this calls the "special" API GNOME Kiosk added for Anaconda to enable keyboard layout handling. And given how little logic is there in Anaconda around While unfortunately older logs from previous kickstart test runs already got purged, I don't remember this happening in the past, so this might might be another victim of the recent GNOME rebases ? UPDATE:
With the DBus error being I think this:
Maybe GNOME kiosk is not yet running at this point & we don't wait for it to start before calling this ? |
Opened a tracking issue for this: https://issues.redhat.com/browse/INSTALLER-3970 (eq. the GNOME Kiosk DBus API crash). |
That to me looks like an error saying that anaconda is trying to talk to gnome-kiosk before anaconda starts gnome-kiosk. |
If I remember correctly,
Which test is causing this crash? I wonder if the test needs to wait for GNOME Kiosk or if it is doing something different from the normal installation. |
It happens at random in our kickstart test suite (essentially a kickstart file with a %post% section that check the system is configured properly). The full batch has about 200 tests & runs with GUI. During the run about 1 or 2 tests randomly encounter this. It does not seems to depend on what the test is testing & happens apparently quite early when the GUI starts. |
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.