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

Xorg server removal #5485

Merged
merged 2 commits into from
Jun 6, 2024
Merged

Conversation

jexposit
Copy link
Contributor

Drop the X.Org server dependency

Start GNOME Kiosk as a Wayland compositor and run Anaconda as a native
Wayland client.

This commit is a follow up on the work done by @Conan-Kudo [1] and
@M4rtinK [2]. Credit goes to them for the code I copied and pasted.

[1] #5401
[2] #5309


Please note that the last patch ("Drop the X.Org server dependency") is the only change intended in this PR.

I cherry picked my patches removing libxklavier and xrandr as they are required to support a Wayland session.
I'll rebase this PR once the mentioned dependencies are merged.

@jexposit jexposit added blocked Don't merge this pull request! rhel-10 labels Feb 21, 2024
@pep8speaks
Copy link

pep8speaks commented Feb 21, 2024

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! 🍻

Comment last updated at 2024-06-06 12:21:12 UTC

pyanaconda/core/constants.py Outdated Show resolved Hide resolved
@Conan-Kudo
Copy link
Contributor

Could you please use Co-authored-by: Neal Gompa <neal@gompa.dev> in your commit message instead of tagging @Conan-Kudo, since parts of #5401 are included in here and I would very much like to not to get an email every time this commit gets shuffled around.

@Conan-Kudo
Copy link
Contributor

Please reword like so:

Start GNOME Kiosk as a Wayland compositor and run Anaconda as a native
Wayland client.

This commit is a follow up on the work done by Neal Gompa [1] and
Martin Kolman [2]. Credit goes to them for the code I copied and pasted.

[1] https://github.com/rhinstaller/anaconda/pull/5401
[2] https://github.com/rhinstaller/anaconda/pull/5309

Co-authored-by: Neal Gompa <neal@gompa.dev>
Co-authored-by: Martin Kolman <mkolman@redhat.com>

@Conan-Kudo
Copy link
Contributor

Thanks! I really didn't want an email every time this commit got refreshed, cherry-picked, or forked. 😅

@jexposit
Copy link
Contributor Author

No problem, I didn't want to share your email publicly without your consent, so I used your GitHub username. Fixed now :D

@jexposit
Copy link
Contributor Author

/build-image --boot.iso

Copy link

Images built based on commit 01f49fc:

  • boot.iso: success

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

@Conan-Kudo
Copy link
Contributor

Is there a reason this isn't tagged for F40?

@M4rtinK
Copy link
Contributor

M4rtinK commented Feb 21, 2024

When we are at it I suggest we mention also @halfline, as he helped me a lot with my PR & an important part of it was provided by him - run-in-new-session.py script, even though it might not be evident from the way the PR was structured:

Ray Strode <rstrode@redhat.com>

@jexposit
Copy link
Contributor Author

Is there a reason this isn't tagged for F40?

In order to fully support Wayland, we need to remove libxklavier and drop xrandr and xrdb.

While I think that the xrandr change can be merged in Fedora -as it doesn't affect the live session and the Spins-, the change libxklavier removal would affect the spins.

#5463 relies on GNOME Kiosk to configure the keyboard layouts, which won't be available in other environments.

Until we find an API that works across DEs or we implement different backends, this PR can't be merged for Fedora.

@M4rtinK could correct me if I'm wrong. I've been working on Anaconda literally one month part time, he knows way better than me how the different images work.

@Conan-Kudo
Copy link
Contributor

Yes, the keyboard layout thing is an issue. Most of the compositors support being live reconfigured through locale1, GNOME is the only exception at the moment.

@Conan-Kudo
Copy link
Contributor

Could we put all the the GNOME Kiosk/Mutter stuff into its own class and make the display.py module a "simple" class like I did in #5401? I would like to be able to rebase #5401 for shipping with CentOS Hyperscale and a few other things, and I expect other distributions using Anaconda (such as ROSA et al) would like to be able to easily plug in alternative compositors (such as KWin or Cage).

@M4rtinK
Copy link
Contributor

M4rtinK commented Feb 21, 2024

While I think that the xrandr change can be merged in Fedora -as it doesn't affect the live session and the Spins-, the change libxklavier removal would affect the spins.

Agreed, I think we can merge the xrandr change into Fedora, but I would like to do that to Rawhide, rather than piling more stuff on Fedora 40 by this point.

#5463 relies on GNOME Kiosk to configure the keyboard layouts, which won't be available in other environments.

Until we find an API that works across DEs or we implement different backends, this PR can't be merged for Fedora.

So with this change, we do cover the boot.iso/netinst images, as those always have GNOME Kiosk, but indeed the problem is with the spins. On the other hand I'm pretty sure we already can't change the keyboard in all the spins that switched to Wayland, but this would potentially also break all the other spins that still have Xorg & libXklavier still works.

I'm not saying we can totally avoid breaking at least some of the spins eventually, but I think this definitely needs to be a process that starts with an announcement & most likely a Fedora Change as well, to make it possible to the various Spin stakeholders to react if they have the capacity to do something about it.

Best options would be if at least the major spins adopted the localed keyboard API - IIRC KDE already does and possibly others ?

Copy link
Contributor

@rvykydal rvykydal left a comment

Choose a reason for hiding this comment

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

As far as I can tell, looks good to me. Thank you.

@M4rtinK
Copy link
Contributor

M4rtinK commented Apr 26, 2024

/build-image --boot.iso

Copy link

Images built based on commit 528561f:

  • boot.iso: failure

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

@M4rtinK
Copy link
Contributor

M4rtinK commented Apr 30, 2024

/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
@M4rtinK
Copy link
Contributor

M4rtinK commented May 3, 2024

/kickstart-test --skip-testtypes knownfailure,manual,skip-on-rhel,skip-on-rhel-10,gh576,gh640,gh804,gh1090,gh1104,gh1106,gh1105,gh1108,gh1109,gh1107,gh1110

@jkonecny12
Copy link
Member

jkonecny12 commented May 23, 2024

Hi, there are two RHEL issues which should be mentioned in these commits before merging:

Adding blocker flag for this.

@jexposit
Copy link
Contributor Author

jexposit commented May 23, 2024

@jkonecny12 I just added a "Resolves: RHEL-38399" tag to the right commit: 4788bb2

@M4rtinK would need to add a reference to RHEL-38407 in the RDP pull request.

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.

I have a few minor notes otherwise a great job done here! :)

Just a note. Please do not forget dropping the last commit before merge. I would keep the blocked label to make this visible.

"""

self._engine.lock_group(0)
layouts = self._keyboard_manager.GetCompositorLayouts()
if len(layouts) < 1:
Copy link
Member

Choose a reason for hiding this comment

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

This check could be

if not layouts:

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed. :)


self._rec.set_options(new_options)
layouts = self._keyboard_manager.GetCompositorLayouts()
if len(layouts) < 1:
Copy link
Member

Choose a reason for hiding this comment

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

The same here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed. :)

Comment on lines 330 to 335
def generate_dbus_code(srcdir):
os.system('gdbus-codegen '
'--interface-prefix org.fedoraproject.Anaconda.Modules. '
'--c-namespace An '
'--generate-c-code an-localization '
'--output-directory %s/widgets/src '
'%s/widgets/src/dbus/org.fedoraproject.Anaconda.Modules.Localization.xml'
Copy link
Member

Choose a reason for hiding this comment

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

I think this logic should be part of Makefile.am not makebumpver.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is in the (widgets) makefile: https://github.com/rhinstaller/anaconda/pull/5485/files#diff-1f27decebb07d587ad45222e1df48fbb14aa33b42ff0f314f3f4abe4b3f02c55R138

This instance is just for convenience when generating updates images. IIRC we do something similar for other cases where we need to compile something before stuffing it into the updates image ?

Comment on lines 162 to 163
const gchar *layout,
AnacondaLayoutIndicator *self) {
Copy link
Member

Choose a reason for hiding this comment

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

Bad indentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed.

@M4rtinK
Copy link
Contributor

M4rtinK commented May 27, 2024

Rebased PR on latest versions of #5463 and #5470 & added correct Jira references. Also dropped python-pam from the temporary COPR repo, as it should be finally in the RHEL 10 repos.

@M4rtinK
Copy link
Contributor

M4rtinK commented May 27, 2024

@jkonecny12 I just added a "Resolves: RHEL-38399" tag to the right commit: 4788bb2

@M4rtinK would need to add a reference to RHEL-38407 in the RDP pull request.

Thanks & will make sure the RDP PR also points to the right Jiras. :)

@M4rtinK M4rtinK force-pushed the xorg-server-removal branch 3 times, most recently from a782596 to c98f5af Compare May 28, 2024 15:41
@M4rtinK
Copy link
Contributor

M4rtinK commented May 28, 2024

/kickstart-test --testtype smoke

@M4rtinK
Copy link
Contributor

M4rtinK commented May 29, 2024

/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 and others added 2 commits June 6, 2024 14:20
Start GNOME Kiosk as a Wayland compositor and run Anaconda as a native
Wayland client.

This commit is a follow up on the work done by Neal Gompa [1], Martin
Kolman and Ray Strode [2]. Credit goes to them for the code I copied
and pasted.

[1] rhinstaller#5401
[2] rhinstaller#5309

Co-authored-by: Neal Gompa <neal@gompa.dev>
Co-authored-by: Martin Kolman <mkolman@redhat.com>
Co-authored-by: Ray Strode <rstrode@redhat.com>

Resolves: RHEL-38399
@M4rtinK M4rtinK removed the blocked Don't merge this pull request! label Jun 6, 2024
@M4rtinK
Copy link
Contributor

M4rtinK commented Jun 6, 2024

/kickstart-test --testtype smoke

@M4rtinK M4rtinK merged commit a7fd670 into rhinstaller:rhel-10 Jun 6, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6 participants