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

webui: Saving webui-desktop log to anaconda.log #5734

Merged

Conversation

adamkankovsky
Copy link
Contributor

No description provided.

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 think the below suggestion would simplify work a lot.

pyanaconda/ui/webui/__init__.py Outdated Show resolved Hide resolved
@jkonecny12
Copy link
Member

Removing my review after discussion with @adamkankovsky. He pointed me to a fact that execWithRedirect won't return pid to watch.

@adamkankovsky
Copy link
Contributor Author

/build-image --live

Copy link

Images built based on commit 0dfb4f4:

  • Live: success

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

@adamkankovsky
Copy link
Contributor Author

@jkonecny12 I tested this on boot.iso and it works well.

pyanaconda/ui/webui/__init__.py Outdated Show resolved Hide resolved
pyanaconda/ui/webui/__init__.py Outdated Show resolved Hide resolved
f.write(repr(proc.pid))
proc.wait()
try:
proc = startProgram(
Copy link
Member

Choose a reason for hiding this comment

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

Why not use stdout and stderr parameters of the startProgram?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because its easier to use the subprocess.PIPE as default and proc.comunicate() for me. But i can change it to IO string buffer as well.

pyanaconda/ui/webui/__init__.py Outdated Show resolved Hide resolved
pyanaconda/ui/webui/__init__.py Outdated Show resolved Hide resolved
with open(self._viewer_pid_file, "w") as f:
f.write(repr(proc.pid))

(output_string, err_string) = proc.communicate()
Copy link
Member

Choose a reason for hiding this comment

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

The issue I'm seeing (if I read it correctly) is that you are blocking completely this thread. If this thread is the main thread (which I guess it is) it will also block event loop so no events are processed. That could have a problematic outcomes like not being able to react on exceptions and react on DBus events.

Please verify if my assumptions are correct or wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There shouldn't be any difference from the original code, there was a wait in the same place, which blocked the kernel until the webui was finished.
The BOSS who takes care of this should have been running in a parallel process long ago and this should not affect him in any way.

Copy link
Member

Choose a reason for hiding this comment

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

I somehow remember that I said even back then that this is not correct way to handle it. But if it was already part of the original code I guess it should be fine here....

@adamkankovsky
Copy link
Contributor Author

/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.

LGTM.

@adamkankovsky adamkankovsky merged commit 82ff58e into rhinstaller:master Sep 25, 2024
16 checks passed
@KKoukiou KKoukiou deleted the webui-desktop-log-update branch October 1, 2024 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants