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

Redirect only GLib loggers to Journal #5962

Merged

Conversation

M4rtinK
Copy link
Contributor

@M4rtinK M4rtinK commented Oct 24, 2024

Previously we redirected all output from the main Anaconda process to Journal to avoid GTK log messages (as GTK runs in the main process) from spamming TTY. Turns out this broke a couple things, such as the shell prompt in rescue mode.

So drop the wholesale process output redirection and instead just redirect (hopefully) all GLib based loggers (used by GTK) to Journal.

Related: RHEL-58834

@pep8speaks
Copy link

pep8speaks commented Oct 24, 2024

Hello @M4rtinK! 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-11-04 16:12:41 UTC

@M4rtinK
Copy link
Contributor Author

M4rtinK commented Oct 24, 2024

So this seems to work well enough:
shell
glib_logger_journal
glib_logger

A couple notes:

  • Not sure how the *** BUG *** line gets to TTY1 - in any case, our GLib redirection code is not enough to get rid of it. Not even adding the redirect on top of the main anaconda.py helps. :P Not critical as the screen is still readable, but weird. :P @halfline
  • Could the gi & GLib import in our logging module somehow be problematic ? If so, I guess we could try to move this to the GUI modules, but it might be a bit tricky, as it needs to be done quite early before GTK starts doing stuff.

@M4rtinK
Copy link
Contributor Author

M4rtinK commented Oct 24, 2024

/kickstart-test --testtype smoke

anaconda.py Outdated Show resolved Hide resolved
pyanaconda/anaconda_logging.py Show resolved Hide resolved
pyanaconda/anaconda_logging.py Outdated Show resolved Hide resolved
pyanaconda/anaconda_logging.py Outdated Show resolved Hide resolved
pyanaconda/anaconda_logging.py Outdated Show resolved Hide resolved
pyanaconda/anaconda_logging.py Outdated Show resolved Hide resolved
pyanaconda/anaconda_logging.py Outdated Show resolved Hide resolved
pyanaconda/anaconda_logging.py Outdated Show resolved Hide resolved
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.

Hi, this is great but there are some interesting points for improvement we should take a look on.

pyanaconda/anaconda_logging.py Outdated Show resolved Hide resolved
pyanaconda/anaconda_logging.py Outdated Show resolved Hide resolved
pyanaconda/anaconda_logging.py Outdated Show resolved Hide resolved
@jkonecny12
Copy link
Member

I've updated the PR based on the suggestions above. There is still one problematic message getting into console but it's not related to this so let's resolve that in another PR.

@jkonecny12
Copy link
Member

/kickstart-test --testtype smoke

@jkonecny12
Copy link
Member

@M4rtinK and me decided that I'll take over this PR. So I'm removing my review.

@jkonecny12 jkonecny12 self-requested a review November 4, 2024 15:06
M4rtinK and others added 4 commits November 4, 2024 17:12
Previously we redirected all output from the main Anaconda process
to Journal to avoid GTK log messages (as GTK runs in the main process)
from spamming TTY. Turns out this broke a couple things, such as the
shell prompt in rescue mode.

So drop the wholesale process output redirection and instead just
redirect (hopefully) all GLib based loggers (used by GTK) to Journal.

Related: RHEL-58834
This place is used for all GLib imports to avoid gi.require import
across the code base.

Related: RHEL-58834
This is not required because we solve that on level of LogHandler which
is used in this solution.

Related: RHEL-58834
Do not ignore log levels from GLib when redirecting these logs to our
logs.

Related: RHEL-58834
@jkonecny12
Copy link
Member

Rebased to fix the test.

@jkonecny12
Copy link
Member

/kickstart-test --testtype smoke

Copy link
Contributor Author

@M4rtinK M4rtinK left a comment

Choose a reason for hiding this comment

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

Looks good to me - could not do it better myself! ;-)

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.

Looks good to me.

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

Successfully merging this pull request may close these issues.

5 participants