fix(raspberry-pi-os): Keymap setting on trixie#6483
Conversation
There was a problem hiding this comment.
Thank you for this submittal @paulober. I'd kindly request we get some unittest coverage on this logic to establish minimally that the code of raspberry_pi_os::Distro.set_keymap performs the operations that you have added.
We really appreciate having test coverage for new code paths as it helps us avoid regressions when future changes are introduced in other areas of the code base.
Also, it would be helpful to represent some console output of the validation of this PR running within Raspberry Pi OS environment to assert its correct behavior and the changed keymap.
Do you have any pointers to emulated environment setup for people who may not have Raspberry Pi hardware?
Sure, I just added some unit testing for the keymap setting.
There are no official instructions how to do this but I recommend looking at qemu - raspi or Corellium and arm virtual hardware. |
blackboxsw
left a comment
There was a problem hiding this comment.
Thank you for this submission @paulober.
- Just a couple of minor nits about rcs=[0] (which will affect your unit tests).
- Was the dropping of growpart and resizefs modules intentional in this PR. If so, why is this needed, it seems completely unrelated.
Otherwise LGTM
| def set_keymap(self, layout: str, model: str, variant: str, options: str): | ||
| """Currently Raspberry Pi OS sys-mods only supports | ||
| setting the layout""" | ||
| contents = "\n".join( |
There was a problem hiding this comment.
While there is a lot of duplication with debian.Distro.set_keymap the difference appears to be the absence of a call to self.manage_service("restart", "console-setup") in raspberry_pi plus the supplementary operations starting with calling /usr/bin/raspi-config. I don't think it's worth avoiding this duplication at this stage due to the slight differences of the overridden subclass method.
There was a problem hiding this comment.
I didn’t include the restart console-setup call because the implementation already runs setsid setupcon, which is effectively what console-setup would do and aligns with how raspi-config currently applies keymap changes.
See this issue for reference: RPi-Distro/raspi-config#286
Regarding the duplication, I’d have to disagree slightly — there’s a reason raspi-config handles it this way. Those additional steps are needed to propagate the configuration properly to the desktop and other components of Raspberry Pi OS.
There was a problem hiding this comment.
This make sense to me as justifcation. Thank you for this discussion.
Yes, that was intentional — sorry for not including a note about it earlier. To avoid this conflict and duplication, we decided to remove these modules for Raspberry Pi OS. It seemed reasonable to include the fix here rather than opening a separate PR, as it’s a small related change discovered in the same testing cycle. |
Remove growpart and resizefs from raspberry-pi-os Signed-off-by: paulober <paul.oberosler@raspberrypi.com>
ab6abd8 to
fe108f6
Compare
Thanks @paulober Just for my own clarification, how does RaspberryPi disable filesystem resizing? I wonder if cloud-init's growpart module and resizefs modules could grow awareness of such settings generally to avoid running in such an environment.
Totally reasonable, just wanted to make sure it's intentional and documented in commit message |
| ) | ||
| subp.subp( | ||
| [ | ||
| "/usr/bin/raspi-config", |
There was a problem hiding this comment.
So we expect /usr/bin/raspi-config to always be present in RaspberrPi images? If not we may need a gating subp.which("/usr/bin/raspi-config") check.
There was a problem hiding this comment.
Right, it is present on every image and if it isn't than you either don't have rpios or performed serious damage to the core system components.
blackboxsw
left a comment
There was a problem hiding this comment.
Thanks for this discussion @paulober and for minor changes here. Just have a couple of discussion questions, but nothing blocking this from landing otherwise
A user can just remove the |
If there are docs for RaspberryPi OS we can look at this in the future I'd be game for adapting our resizefs and growpart modules to be cmdline-aware for RPI environments to NOOP if disabled. It's beneficial to not have different base cloud.cfg in various downstreams. But that RPI downstream difference will be preserved and documented in upstream's config/cloud.cfg.tmpl, so I'm less concerned about the discoverability of the differences in RPI. We can circle back to change that if you decide downstream to move cc_raspberry_pi module earlier in boot and deal with downstream cloud-init deb packaging of the cloud-init-network service ordering related to NetworkManager services. |
…al#6483) This commit fixes keymap configuration on Trixie-based Raspberry Pi OS images by aligning behavior with raspi-config. It also removes growpart and resizefs from the cloud-init configuration for Raspberry Pi OS, as these functions are already provided by the system and caused unnecessary duplication.
This commit fixes keymap configuration on Trixie-based Raspberry Pi OS images by aligning behavior with raspi-config. It also removes growpart and resizefs from the cloud-init configuration for Raspberry Pi OS, as these functions are already provided by the system and caused unnecessary duplication.
Proposed Commit Message
Test Steps
Merge type
@tdewey-rpi