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

systemd-firstboot: add vconsole keymap support #7035

Merged
merged 1 commit into from Nov 10, 2017

Conversation

2 participants
@tblume
Contributor

tblume commented Oct 9, 2017

Enable systemd-firstboot to set the keymap.

RFE:

#6346

@poettering

looks pretty good, but some coding style issues

Show outdated Hide outdated src/basic/locale-util.c Outdated
Show outdated Hide outdated src/basic/locale-util.c Outdated
Show outdated Hide outdated src/firstboot/firstboot.c Outdated
Show outdated Hide outdated src/firstboot/firstboot.c Outdated
@tblume

This comment has been minimized.

Show comment
Hide comment
@tblume

tblume Oct 11, 2017

Contributor

Thanks for the feedback. I have updated the patch. Can you please review again?

Contributor

tblume commented Oct 11, 2017

Thanks for the feedback. I have updated the patch. Can you please review again?

@poettering

This comment has been minimized.

Show comment
Hide comment
@poettering

poettering Oct 24, 2017

Member

CI failed on this one, and it appears to be caused by your patch. Please rebase on current git and repush

Member

poettering commented Oct 24, 2017

CI failed on this one, and it appears to be caused by your patch. Please rebase on current git and repush

@poettering

looks pretty good, just one nitpick. Also is missing documentation in the man page of the new option

Show outdated Hide outdated src/basic/locale-util.c Outdated
@tblume

This comment has been minimized.

Show comment
Hide comment
@tblume

tblume Oct 25, 2017

Contributor

Ok, thanks I have added a manpage entry and corrected the indendation.
The reason why the CI was failing was my keymap testing additions in test-locale-util.c.
In these tests, the get_keymaps function doesn't find any keymap.
The tests work correctly on SUSE.
I've commented out the tests now and the CI seems to succeed.
Does the internal test system have keymaps installed?

Contributor

tblume commented Oct 25, 2017

Ok, thanks I have added a manpage entry and corrected the indendation.
The reason why the CI was failing was my keymap testing additions in test-locale-util.c.
In these tests, the get_keymaps function doesn't find any keymap.
The tests work correctly on SUSE.
I've commented out the tests now and the CI seems to succeed.
Does the internal test system have keymaps installed?

@poettering

both the tool and the tests should really be able to deal with no keymaps being installed. the tool sould just skip over the keymap logic in that case I think. And the tests should test what they can, but skip the relevant tests if no keymaps are installed

if (!arg_prompt_keymap)
return 0;
r = get_keymaps(&kmaps);

This comment has been minimized.

@poettering

poettering Oct 25, 2017

Member

I figure we should add some additional handlig here, if no keymaps are installed, and simply skip the operation in that case

@poettering

poettering Oct 25, 2017

Member

I figure we should add some additional handlig here, if no keymaps are installed, and simply skip the operation in that case

Show outdated Hide outdated src/test/test-locale-util.c Outdated
Show outdated Hide outdated src/test/test-locale-util.c Outdated
@poettering

This comment has been minimized.

Show comment
Hide comment
@poettering

poettering Oct 25, 2017

Member

Does the internal test system have keymaps installed?

That's pretty likely, as test machines are VMs and thus headless...

Member

poettering commented Oct 25, 2017

Does the internal test system have keymaps installed?

That's pretty likely, as test machines are VMs and thus headless...

@tblume

This comment has been minimized.

Show comment
Hide comment
@tblume

tblume Oct 27, 2017

Contributor

I have updated my patch to address the no-keymap situation.
Ok, like that?

Contributor

tblume commented Oct 27, 2017

I have updated my patch to address the no-keymap situation.
Ok, like that?

Show outdated Hide outdated src/firstboot/firstboot.c Outdated
Show outdated Hide outdated src/firstboot/firstboot.c Outdated
Show outdated Hide outdated src/firstboot/firstboot.c Outdated
assert_se(!keymap_is_valid("\x01gar\x02 bage\x03"));
r = get_keymaps(&kmaps);
if (r == -ENOENT)

This comment has been minimized.

@poettering

poettering Oct 27, 2017

Member

please add a comment here, why we ingore this

@poettering

poettering Oct 27, 2017

Member

please add a comment here, why we ingore this

This comment has been minimized.

@tblume

tblume Oct 30, 2017

Contributor

I made the changes requested, ok now?

@tblume

tblume Oct 30, 2017

Contributor

I made the changes requested, ok now?

Show outdated Hide outdated src/test/test-locale-util.c Outdated
@poettering

This comment has been minimized.

Show comment
Hide comment
@poettering

poettering Nov 8, 2017

Member

The fedora CI failed, and I don't fully grok why. It might be because this PR is not on top of current git, and is missing some fixes that went into git master. Any chance you can rebase on top of current git, then repush, so that the CI gets restarted and tests run with the newest version we have in place

Member

poettering commented Nov 8, 2017

The fedora CI failed, and I don't fully grok why. It might be because this PR is not on top of current git, and is missing some fixes that went into git master. Any chance you can rebase on top of current git, then repush, so that the CI gets restarted and tests run with the newest version we have in place

systemd-firstboot: add vconsole keymap support
Enable systemd-firstboot to set the keymap.

RFE:

#6346
@tblume

This comment has been minimized.

Show comment
Hide comment
@tblume

tblume Nov 9, 2017

Contributor

Ok, rebased.

Contributor

tblume commented Nov 9, 2017

Ok, rebased.

@poettering poettering merged commit ed457f1 into systemd:master Nov 10, 2017

2 of 5 checks passed

xenial-s390x autopkgtest finished (failure)
Details
xenial-amd64 autopkgtest running
Details
xenial-i386 autopkgtest running
Details
Fedora Rawhide CI x86_64 rpm build succeeded
Details
semaphoreci The build passed on Semaphore.
Details

@tblume tblume deleted the tblume:add-keymap-setup-to-systemd-firstboot branch Dec 8, 2017

SergioAtSUSE pushed a commit to SergioAtSUSE/systemd_systemd that referenced this pull request Jun 7, 2018

systemd-firstboot: add vconsole keymap support (systemd#7035)
Enable systemd-firstboot to set the keymap.

RFE:

systemd#6346
(cherry picked from commit ed457f1)

[tblume: fixes bsc#1046436]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment