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.

Sign up for GitHub

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

Jump to bottom

liveinst: Allow running as a Wayland-native application #5946

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

liveinst: Allow running as a Wayland-native application #5946

wants to merge 1 commit into from

Conversation

Conan-Kudo
Copy link
Contributor

As long as we have the path to the Wayland socket, we can run Anaconda as a Wayland-native application.

@github-actions github-actions bot added the f42 Fedora 42 label Oct 18, 2024
@jkonecny12
Copy link
Member

/kickstart-test --testtype smoke

@jkonecny12
Copy link
Member

/build-image --live

github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 18, 2024
View reviewed changes
data/liveinst/liveinst Fixed Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

Images built based on commit 8c55c2f:

  • Live: success

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

github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 18, 2024
View reviewed changes
data/liveinst/liveinst Fixed Show fixed Hide fixed
@jkonecny12
Copy link
Member

/kickstart-test --testtype smoke

@jkonecny12
Copy link
Member

/build-image --live

@github-actions GitHub Actions
Copy link

Images built based on commit b7b5800:

  • Live: success

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

@jkonecny12
Copy link
Member

The shellcheck issues needs to be resolved first:

data/liveinst/liveinst:51:12: warning: Declare and assign separately to avoid masking return values. [SC2155]
data/liveinst/liveinst:55:22: note: Double quote to prevent globbing and word splitting. [SC2086]
data/liveinst/liveinst:56:8: warning: Declare and assign separately to avoid masking return values. [SC2155]

If you think this is fine to go, you can disable the checks with comment

# shellcheck disable=SC2155

@Conan-Kudo
Copy link
Contributor Author

@jkonecny12 Done.

@jkonecny12
Copy link
Member

/kickstart-test --testtype smoke

@jkonecny12
Copy link
Member

/build-image --live

@github-actions GitHub Actions
Copy link

Images built based on commit 900abc1:

  • Live: success

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

KKoukiou
KKoukiou reviewed Oct 22, 2024
View reviewed changes
Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

@Conan-Kudo have you maybe tested that on the non wayland spins as well?

@Conan-Kudo
Copy link
Contributor Author

@Conan-Kudo have you maybe tested that on the non wayland spins as well?

Yes, I verified this changes none of the behavior on Fedora LXQt in X11 mode.

jkonecny12
jkonecny12 requested changes Oct 22, 2024
View reviewed changes
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.

Seems that this patch will break Fedora Workstation.

data/liveinst/liveinst Show resolved Hide resolved
@jkonecny12 jkonecny12 mentioned this pull request Oct 23, 2024
Merged
@jkonecny12
Copy link
Member

I finally get to the retesting of your change and it doesn't return any error on the terminal. However, I was able to find:

anaconda: ui.webui: web-ui: the webui-desktop script ended abruptly!

@jkonecny12
Copy link
Member

So there is more from the error:
Screenshot_Test-live_2024-10-24_15:16:44

and I was able to debug the fail to this line: https://github.com/rhinstaller/anaconda-webui/blob/main/webui-desktop#L102

and from the screenshot you should be able to see:

GLib.py:497: Warning: ../glib/gmain.c:5930: waitpid(pid:5154) failed: No child processes (10). See documentation of g_child_watch_source_new() for possible causes.

Not sure how this exactly related to systemd-cat call or why your changes are causing this issue.

@KKoukiou
Copy link
Contributor

@Conan-Kudo the tried this as well with some extra debugging and I got:
Screen Shot 2024-10-24 at 18 32 46 (1)

Looks like this change has problem with Web UI firefox running as root. We are about to change that, #5689

So maybe we can re-try this PR after the rootless Web UI is active.

@KKoukiou KKoukiou added the blocked Don't merge this pull request! label Oct 25, 2024
KKoukiou
KKoukiou reviewed Oct 25, 2024
View reviewed changes
Copy link
Contributor

@KKoukiou KKoukiou left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Needs rebase.
Here is a rebased version https://github.com/KKoukiou/anaconda/tree/wayland-liveinst in case you dont have time to check.

thanks I will re-test after it's conflict free.

@Conan-Kudo
liveinst: Allow running as a Wayland-native application
54a7098 
As long as we have the path to the Wayland socket, we can run
Anaconda as a Wayland-native application.
@Conan-Kudo
Copy link
Contributor Author

I rebased this yesterday.

KKoukiou
KKoukiou approved these changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

For Workstation with Web UI it works fine. I did not test any non wayland spin.

@KKoukiou KKoukiou removed the blocked Don't merge this pull request! label Oct 29, 2024
@jkonecny12
Copy link
Member

/build-image --live

@github-actions GitHub Actions
Copy link

Images built based on commit 54a7098:

  • Live: success

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

@jkonecny12
Copy link
Member

/kickstart-test --testtype smoke

@jkonecny12
Copy link
Member

/kickstart-test fedora-live-image-build

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KKoukiou KKoukiou KKoukiou approved these changes

@jkonecny12 jkonecny12 Awaiting requested review from jkonecny12

Assignees
No one assigned
Labels
f42 Fedora 42
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Conan-Kudo @jkonecny12 @KKoukiou