Use keyboard configuration from Gnome Shell live #4976

jkonecny12 · 2023-07-27T23:49:18Z

Use keyboard configuration from live media before fallbacking to default.

This changeset sets the keyboard layout from live when no configuration was done by user. This way we know that we targeting web UI only because otherwise we force user to do the configuration (have to verify).

Currently this is just a draft. Needs more polishing. Missing pieces below.

Please check that your PR follows these rules:

[Use first layout fro mru as source for vconsole] This needs to be done to use currently used keyboard for LUKS unlocking.
Code conventions. tl;dr: Follow PEP8, wrap at 100 chars, and provide docstrings.
Commit message conventions. tl;dr: Heading, empty line, longer explanations, all wrapped manually. If in doubt, write a longer commit message with more details.
Tests pass and cover your changes. You can run tests locally manually, or have them run as part of the PR. A team member will have to enable tests manually after inspecting the PR.
Release notes and docs if the PR adds something major or changes existing behavior.
[Verify that we don't broke spins nor boot.iso] The logic here builds on premise that keyboard is forced on non web UI installations to be set. I should verify this is true.

jkonecny12 · 2023-07-28T00:06:50Z

I talked to @AdamWill and we decided that this feature could be delivered to 100% code complete deadline instead of the Code testable deadline. Thanks Adam!

So I'll finish this after I'll get from my PTO.

jkonecny12 · 2023-08-06T21:42:34Z

UPDATED:

Change solution which better integrates with the current structure. Enhancing existing missing method with live data.
Some refactorization of the code to better reflect the current parts.
Add new tests for enhanced execWithCapture method

TODO:

Still missing test coverage for the Live code
Needs proper testing
Needs release notes

rvykydal · 2023-08-07T11:10:46Z

tests/unit_tests/pyanaconda_tests/core/test_system.py

+# License and may only be used or replicated with the express permission of
+# Red Hat, Inc.
+#
+# Red Hat Author(s): Vendula Poncova <vponcova@redhat.com>


I guess Author needs updating or removing.

pyanaconda/core/util.py

pyanaconda/modules/localization/utils.py

rvykydal · 2023-08-07T12:10:09Z

Looks good to me.

VladimirSlavik

Overall I think it's good. From the things that can be addressed, file names are probably the most important. This code works with keyboard and user, not the whole "system" :)

I really dislike how the live user is tied into exec functions, but at this moment there seems to be few alternatives to this.

pyanaconda/core/util.py

pyanaconda/modules/localization/live_system.py

pyanaconda/modules/localization/utils.py

pyanaconda/core/system.py

pyanaconda/modules/localization/live_system.py

jkonecny12 · 2023-08-07T23:22:16Z

UPDATED:

Add tests coverage
Changed a structure so the live_system instanced is passed in similar to localed. That makes testing easier and it's more consistent with the current behavior.

VladimirSlavik

Thank you for the thorough tests, that's a joy to see.

I think the main issue I have with this is naming - you leave out the word "keyboard" very systematically. Is there a reason for that? I can't imagine what would be in the files and classes, especially when some are placed in a module that does keyboards, but if you know what will be added there (will, not could), then that starts making sense.

Other than that, LGTM!

(I didn't check all of the tests in detail, and still need to take a look the last release note paragraph.)

pyanaconda/core/system.py

pyanaconda/modules/localization/live_system.py

pyanaconda/modules/localization/runtime.py

pyanaconda/modules/localization/live_system.py

pyanaconda/modules/localization/utils.py

docs/release-notes/keyboard_from_live.rst

jkonecny12 · 2023-08-08T20:11:28Z

Thanks a lot for the review @VladimirSlavik. I'll fix these tomorrow I also found that I need minor changes in the code because there is unexpected behavior on other than Gnome Shell live systems.

This data needs to be used to get keyboard information from the Live media. However, in general it would be useful to be able to run commands under liveuser.

This enables us to run any command easily under liveruser user account. We need execWithCaptureAsLiveUser for reading live keyboard configuration.

This method is not directly bound to localed and will be enhanced to also take into account Live system configuration. So let's move it first.

jkonecny12 · 2023-08-09T13:20:06Z

UPDATED:

Slightly change the logic of missing keyboard resolution.
- I found after testing that when user set the keyboard in UI they set just layout and virtual console keymap is not set automatically by this. In this case we don't want to read live configuration but rather want to convert what user set in the Anaconda.
- Change is to read live data only if x_layouts is not set and ignoring virtual console keymap configuration here
Fix release notes
- Simplify explanation of why live solution sucks and instead just explain why these BZs are attached.
Refactorizations
- Rename core.system to core.live_user
- Rename localization.live_system to localization.live_keyboard
- Do this for all the parameters where this was used.

M4rtinK

Looks good to me, but I would suggest improving the docstrings in a couple places.

Also I wonder if special casing the exec function for the live user is a good idea or maybe we could use something more generic (eq. run as a given user).

M4rtinK · 2023-08-09T14:50:08Z

pyanaconda/core/util.py

    return _run_program(argv, stdin=stdin, root=root, log_output=log_output,
                        filter_stderr=filter_stderr, do_preexec=do_preexec)[1]


+def execWithCaptureAsLiveUser(command, argv, stdin=None, root='/', log_output=True,


Why special case this for the live user ? I think execWithCaptureAsUser would sound better and would be more flexible as we could provide user name as necessary.

There is no need for other user and I would be surprised if we would have something like that in the near future. So this would be just a complication of the usage because you would have to provide parameter which would be the same for every case.

M4rtinK · 2023-08-09T14:54:24Z

pyanaconda/modules/localization/utils.py

+
+
+def get_missing_keyboard_configuration(localed_wrapper, x_layouts, vc_keymap):
+    """Get missing keyboard settings by conversion and default values.


I think this is missing some context what we are trying to accomplish and how. Why is the keyboard configuration missing & how are we going to get se settings (conversion of what ? where do the default values come from ?).

Honestly, the original code was so confusing because it was just about conversion but no word about defaults. I tried to improve the state but not that successful.

I think this is greatly improved in the last commit change.

M4rtinK · 2023-08-09T14:57:13Z

pyanaconda/modules/localization/live_keyboard.py

+        """Read keyboard configuration from the current system.
+
+        The configuration have to be returned in format which is understandable by us for the
+        installation. That means localed will understand it.


Maybe it would be a good idea to note there what formats are supported ? Or at least what tooling is going to work with them, so one can find out the supported formats transitively. Is it just localed or also something else ?

It is specified in the return value of the docstring.

Oh right, now I see it. :)

M4rtinK · 2023-08-09T14:58:42Z

pyanaconda/modules/localization/utils.py

+    """Get missing keyboard settings.
+
+    It will be obtained in this order:
+    - if supported live environment - use settings from there


I would suggest "from there" -> "settings from the Live environment"

M4rtinK · 2023-08-09T14:59:49Z

pyanaconda/modules/localization/utils.py

+
+    It will be obtained in this order:
+    - if supported live environment - use settings from there
+    - use DEFAULT_KEYBOARD for missing part


I'm confused what "missing part" means ? Eq. use DEFAULT_KEYBOARD in environments that are not supported ?

I tried to improve this even more.

jkonecny12 · 2023-08-09T16:19:48Z

Tested on boot.iso and live images.

It works well. However, I found a bug that when you set the keyboard on Live without changing layout the mru will return empty field. I need to file that, seems like a bug in Gnome Shell.

VladimirSlavik

live_keyboard = LGTM.
release note = LGTM.
exec stuff - as agreed, needs changes as a whole, so ok
It isn't necessary to pass the keyboard class through so much. There is no costly initialization or checks that should not be repeated (unlike localed). There is also only one function that needs it.

VladimirSlavik · 2023-08-09T15:40:41Z

pyanaconda/modules/localization/utils.py

+def _resolve_missing_from_live(localed_wrapper, live_keyboard_wrapper, x_layouts, vc_keymap):
+    if not live_keyboard_wrapper:
+        return x_layouts, vc_keymap


The fact that this is called always, even if not on live, is really confusing. Check for live outside, before calling it.

Construct the wrapper (it's not wrapper...) with get_live_keyboard_instance only inside this function, because it will not be called anywhere else in the whole codebase. There is no need to pass it through the two or three layers on the way.

VladimirSlavik · 2023-08-09T15:41:03Z

pyanaconda/modules/localization/utils.py

+        return x_layouts, vc_keymap
+
+    # layouts are not set by user, we should take a look for live configuration if available
+    if not x_layouts:


....check for live here.

Creating the live here to get it passed into the function which work with it seems to me also unnecessary complication. I understand your point but not sure about the solution. I'll think about that.

The check here would be if conf..., in the function you'd just create the object and do what you need.

VladimirSlavik · 2023-08-09T16:27:55Z

tests/unit_tests/pyanaconda_tests/modules/localization/test_module_localization.py

-        localed.convert_layouts.return_value = converted_x_layouts
+                                                 result_vc_keymap,
+                                                 localed,
+                                                 live_keyboard):

        result = get_missing_keyboard_configuration(


After that, you should be able to do here

with patch("pyanaconda.modules.localization.utils.get_live_keyboard_instance") as get_mock: (...)

instead of passing the keyboard class as parameter.

It's definitely less disruptive to the whole codebase and great improvement to my passing everything as arguments :)

VladimirSlavik · 2023-08-09T16:31:38Z

tests/unit_tests/pyanaconda_tests/modules/localization/test_module_localization.py

@@ -1088,3 +1233,137 @@ def test_localed_wrapper_no_systembus(self, mocked_system_bus):
        mocked_system_bus.check_connection.return_value = False
        localed_wrapper = LocaledWrapper()
        self._guarded_localed_wrapper_calls_check(localed_wrapper)
+
+
+class LiveSystemKeyboardTestCase(unittest.TestCase):


Consider doing this in a new test file matching the new module file.

This whole file could use some splitting anyway, but that's another independent PR...

If I do that I should also split localed test from this to keep consistency. However, not sure if it should be part of this PR (it is already big). Let's handle it as followup.

Yes - #5029.

jkonecny12 · 2023-08-09T19:21:14Z

Good point about the live_keyboard passing. I did it this way to make it consistent with localed but true is that it's causing unnecessary troubles. Good point about removing this back.

This configuration will be used if no keyboard layout is set by user and Anaconda is running on supported live environment. This solution is integrated to the already existing `get_missing_keyboard_configuration` function before using the DEFAULT_KEYBOARD layout (US). Before this change the logic was: - use configuration if set by user - if only keyboard layout or virtual console is set convert the other by localed - if nothing set use DEFAULT_KEYBOARD After this change: - use configuration if set by user - if on supported live environment, read the current keyboard configuration - if only keyboard layout or virtual console is set convert the other by localed - if nothing set use DEFAULT_KEYBOARD Currently, only Gnome Shell is supported but we should extend this solution ideally to all Wayland based spins. Wayland is hostile to Anaconda because it don't allow us to control live keyboard configuration. Live environment now have configuration of the keyboard separate from the Anaconda configuration, which is used for the installed system. That means that users can write passwords with other layout than what is installed (especially problem for LUKS).

Look for currently used keyboard layout and set that as virtual console keymap after conversion. It's probably what user wants to use because it was used during the installation.

jkonecny12 · 2023-08-10T19:14:29Z

UPDATED:

Removed passing the live keyboard instance everywhere instead create it on place where it is used. Thanks for the suggestion @VladimirSlavik .
Tried to improve the code comments and commit message based on the @M4rtinK feedback. Thanks!

jkonecny12 · 2023-08-10T19:14:55Z

/kickstart-test --testtype smoke

jkonecny12 · 2023-08-10T19:16:40Z

/kickstart-test --testtype keyboard

VladimirSlavik

Thank you, LGTM now!

jkonecny12 · 2023-08-15T13:31:23Z

/packit build

jkonecny12 · 2023-08-15T15:33:37Z

/packit build

github-actions bot added the f39 label Jul 27, 2023

jkonecny12 force-pushed the master-install-keyboard-layout-from-Gnome-Shell branch from c8bab80 to ad2158c Compare August 6, 2023 21:40

jkonecny12 added the release note required Write a release note for this change. label Aug 6, 2023

rvykydal reviewed Aug 7, 2023

View reviewed changes

pyanaconda/core/util.py Outdated Show resolved Hide resolved

rvykydal reviewed Aug 7, 2023

View reviewed changes

pyanaconda/modules/localization/utils.py Outdated Show resolved Hide resolved

rvykydal reviewed Aug 7, 2023

View reviewed changes

pyanaconda/modules/localization/utils.py Outdated Show resolved Hide resolved

VladimirSlavik reviewed Aug 7, 2023

View reviewed changes

jkonecny12 force-pushed the master-install-keyboard-layout-from-Gnome-Shell branch from ad2158c to 466ecae Compare August 7, 2023 23:19

jkonecny12 force-pushed the master-install-keyboard-layout-from-Gnome-Shell branch from 466ecae to 43babcb Compare August 8, 2023 12:54

jkonecny12 marked this pull request as ready for review August 8, 2023 13:02

jkonecny12 force-pushed the master-install-keyboard-layout-from-Gnome-Shell branch 3 times, most recently from 7edd3f9 to c980ee6 Compare August 8, 2023 14:50

VladimirSlavik reviewed Aug 8, 2023

View reviewed changes

jkonecny12 added 4 commits August 9, 2023 14:43

Extract liveuser data from help to generic tooling

21fbce0

This data needs to be used to get keyboard information from the Live media. However, in general it would be useful to be able to run commands under liveuser.

Add execWithCaptureAsLiveUser to run as liveuser

d1b6645

This enables us to run any command easily under liveruser user account. We need execWithCaptureAsLiveUser for reading live keyboard configuration.

Fix typo in the LocalizationInterface docstring

a915755

Move get_missing_keyboard_configuration method

b8d4dbf

This method is not directly bound to localed and will be enhanced to also take into account Live system configuration. So let's move it first.

jkonecny12 force-pushed the master-install-keyboard-layout-from-Gnome-Shell branch from c980ee6 to 0bf83dc Compare August 9, 2023 13:13

M4rtinK approved these changes Aug 9, 2023

View reviewed changes

VladimirSlavik reviewed Aug 9, 2023

View reviewed changes

VladimirSlavik mentioned this pull request Aug 9, 2023

tests: Split l10n module tests #5029

Merged

jkonecny12 added 3 commits August 10, 2023 21:00

Add support for virtual console keymap from live

696f1f5

Look for currently used keyboard layout and set that as virtual console keymap after conversion. It's probably what user wants to use because it was used during the installation.

Add realease notes for keyboard from live system

a9ecfc4

jkonecny12 force-pushed the master-install-keyboard-layout-from-Gnome-Shell branch from 0bf83dc to a9ecfc4 Compare August 10, 2023 19:13

VladimirSlavik approved these changes Aug 15, 2023

View reviewed changes

jkonecny12 merged commit 345bbab into rhinstaller:master Aug 15, 2023
18 checks passed

jkonecny12 mentioned this pull request Aug 22, 2023

Copy /var/lib/gnome-initial-setup/state to installed system #5056

Merged



		def get_missing_keyboard_configuration(localed_wrapper, x_layouts, vc_keymap):
		"""Get missing keyboard settings by conversion and default values.

Use keyboard configuration from Gnome Shell live #4976

Use keyboard configuration from Gnome Shell live #4976

Conversation

jkonecny12 commented Jul 27, 2023 • edited

jkonecny12 commented Jul 28, 2023 • edited

jkonecny12 commented Aug 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvykydal commented Aug 7, 2023

VladimirSlavik left a comment

Choose a reason for hiding this comment

jkonecny12 commented Aug 7, 2023

VladimirSlavik left a comment

Choose a reason for hiding this comment

jkonecny12 commented Aug 8, 2023

jkonecny12 commented Aug 9, 2023

M4rtinK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkonecny12 commented Aug 9, 2023

VladimirSlavik left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkonecny12 Aug 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkonecny12 commented Aug 9, 2023

jkonecny12 commented Aug 10, 2023

jkonecny12 commented Aug 10, 2023

jkonecny12 commented Aug 10, 2023

VladimirSlavik left a comment

Choose a reason for hiding this comment

jkonecny12 commented Aug 15, 2023

jkonecny12 commented Aug 15, 2023

jkonecny12 commented Jul 27, 2023 •

edited

jkonecny12 commented Jul 28, 2023 •

edited

jkonecny12 commented Aug 6, 2023 •

edited

VladimirSlavik left a comment •

edited

jkonecny12 Aug 9, 2023 •

edited