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

Add the locale1-x11-sync script to the anaconda-live subpackage #6149

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

M4rtinK
Copy link
Contributor

@M4rtinK M4rtinK commented Feb 7, 2025

This script, installed to the libexec folder, is needed for controlling X11 system keyboard layouts using the systemd-localed DBus API.

Without this script keyboard switching will not work on X11 Fedora Live spins.

The script might end up in a separate package eventually but for now it should be enough to ship it as part of the anaconda-live package.

@M4rtinK M4rtinK added f42 Fedora 42 f43 labels Feb 7, 2025
@jkonecny12
Copy link
Member

Looks good to me so far.

@KKoukiou KKoukiou removed the f42 Fedora 42 label Feb 13, 2025
@M4rtinK M4rtinK force-pushed the main-add_localed-x11-sync_for_live branch 5 times, most recently from 259675c to 70768b5 Compare February 17, 2025 15:18
@jkonecny12 jkonecny12 added the f42 Fedora 42 label Feb 18, 2025
Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

The systemd unit wants a bit of polishing but other than that it looks good to me :).

I tested it and when the unit is moved it should work nicely. I'm happy to test it again when the PR is ready if needed.

anaconda.spec.in Outdated
@@ -443,9 +449,11 @@ rm -rf \
# do not provide the live subpackage on RHEL

%files live
%{_unitdir}/locale1-x11-sync.service
Copy link
Member

Choose a reason for hiding this comment

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

The service file needs to be installed as user service file.

https://docs.fedoraproject.org/en-US/packaging-guidelines/Systemd/#packaging_filesystem

We need to store it in /usr/lib/systemd/user instead of the current path. The current path broke the script as it depends on user session variables. In other words use _userunitdir rpm macro instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, converted to a user unit. :)

@@ -0,0 +1,7 @@
[Unit]
Description=Sync localed configuration with running X11 system
Copy link
Member

Choose a reason for hiding this comment

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

Missing Documentation which is recommended by Packaging guidelines. So I'm thinking if we want to add Documentation field with link to the script installed locally.
https://docs.fedoraproject.org/en-US/packaging-guidelines/Systemd/#unit_files_basic_format_unit_doc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

It's not the same code but why not.

@@ -0,0 +1,7 @@
[Unit]
Description=Sync localed configuration with running X11 system

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, lets add that, pointing to the graphical target in our case.

[Service]
Type=exec
ExecStart=/usr/libexec/locale1-x11-sync
Restart=on-failure
Copy link
Member

Choose a reason for hiding this comment

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

Add a missing newline please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like an extra one at the end ? OK.

@M4rtinK M4rtinK changed the title Add the locale1_x11_sync script to the anaconda-live subpackage Add the locale1-x11-sync script to the anaconda-live subpackage Feb 18, 2025
@M4rtinK M4rtinK force-pushed the main-add_localed-x11-sync_for_live branch from 70768b5 to a063a69 Compare February 18, 2025 16:09
Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks great to me. Thanks a lot for your help.

@jkonecny12
Copy link
Member

Hi @AdamWill, this change will enable Anaconda to control keyboards on X11 systems. The original plan was to have it as separate package
https://github.com/rhinstaller/localed-x11-sync
however, I did not found time to finish it so for simplification we moved it under anaconda-live package.

It works with one bug similar to what Gnome Kiosk have right now, so it doesn't propagate keyboard change back to Anaconda (keyboard switching with keyboard shortcut). I'll try to get the fix to F42.

Do you agree in getting this to X11 based Live spins? If yes, I'll ping the maintainers and create a PRs on livesys-scripts and kiwi/fedora-kickstarts.

@jkonecny12
Copy link
Member

jkonecny12 commented Feb 18, 2025

Hmm, I don't see a reason of this failure:
https://download.copr.fedorainfracloud.org/results/packit/rhinstaller-anaconda-6149/fedora-eln-x86_64/08668976-anaconda/builder-live.log.gz

Why we have untracked file on ELN but it works on Rawhide...

Could it be because anaconda-live is not on ELN?

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

One more required change.

@@ -381,6 +386,7 @@ rm -rf \
%{buildroot}/%{_sysconfdir}/xdg/autostart/liveinst-setup.desktop \
%{buildroot}/%{_bindir}/liveinst \
%{buildroot}/%{_libexecdir}/liveinst-setup.sh \
%{buildroot}/%{_libexecdir}/locale1-x11-sync \
Copy link
Member

Choose a reason for hiding this comment

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

I see, you need to add also service file here or we will break ELN builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right, there is a special section for this - not very intuitive. :P Anyway, should be added now. :)

@AdamWill
Copy link
Contributor

did this used to "work"? didn't we used to defer to the host desktop environment for this kind of thing on live spins?

that is: is this a new feature, or an attempt to 'fix' something that was broken by wayland?

This script installed to the libexec folder and the systemd unit triggering it
are needed for controlling X11 system keyboard layouts using the systemd-localed DBus API.

Without the script and the systemd unit keyboard switching will not work on X11 Fedora Live spins.

Both might end up in a separate package eventually but for now it should
be enough to ship them as part of the anaconda-live package.
@M4rtinK M4rtinK force-pushed the main-add_localed-x11-sync_for_live branch from a063a69 to a8c0035 Compare February 18, 2025 17:00
@jkonecny12
Copy link
Member

jkonecny12 commented Feb 18, 2025

did this used to "work"? didn't we used to defer to the host desktop environment for this kind of thing on live spins?

that is: is this a new feature, or an attempt to 'fix' something that was broken by wayland?

It used to work but maintainers of the spin should be the one handling the migration. As requested here
https://fedoraproject.org/wiki/Changes/Anaconda_As_Native_Wayland_Application#Consistent_keyboard_control

However, as I see that most of the X11 based spin maintainers are not much active, we are actively doing the work for them this way. So it is expected to be broken after Wayland switch.

@jkonecny12
Copy link
Member

/kickstart-test --testtype smoke

Copy link
Member

@jkonecny12 jkonecny12 left a comment

Choose a reason for hiding this comment

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

Looks great to me. So if we get ACK from @AdamWill I think we can get this to F42.

@M4rtinK M4rtinK marked this pull request as ready for review February 19, 2025 12:42
@AdamWill
Copy link
Contributor

sure, give it a shot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f42 Fedora 42 f43
Development

Successfully merging this pull request may close these issues.

4 participants