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

nixos: unify virtual console options #71473

Merged
merged 4 commits into from Dec 20, 2019
Merged

nixos: unify virtual console options #71473

merged 4 commits into from Dec 20, 2019

Conversation

rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Oct 20, 2019

Motivation for this change

As per commit message:

This commit moves all the virtual console related options
to a dedicated config/console.nix NixOS module.

Currently most of these are defined in config/i18n.nix
with a "console" prefix like i18n.consoleFont,
i18n.consoleColors or under boot and are implemented
in tasks/kbd.nix.
Since they have little to do with actual internationalisation
and are (informally) in an attrset already, it makes sense to
move them to a specific module.

Also, this commit changes the console colors implementation
to use the kernel parameters instead of relying on terminal
escape sequences. This means the palette is applied by the
kernel itself with no for custom code running in the initrd
and works for virtual terminals (not only tty0).

Things done
  • Tested via one or more NixOS tests (not aware of any one specific)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @vcunat @abbradar @greedy @the-kenny

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/80

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/what-are-your-goals-for-20-03/4773/4

@pstch
Copy link
Contributor

pstch commented Nov 19, 2019

Reviewed points
  • changes are backward compatible : no, options were renamed
  • renamed options are declared with mkRenamedOptionModule
  • changes that are not backward compatible are documented in release notes
  • module tests succeed on linux amd64 : not applicable
  • options types are appropriate : no change
  • options description is set : no change
  • options example is provided : no change
  • documentation affected by the changes is updated : manual not affected
Possible improvements
  • as this PR changes important options that are defined in many configurations, it may be better to isolate it from the changes to the color palette application

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Nov 19, 2019

Thank you for the review, @pstch.

as this PR changes important options that are defined in many configurations, it may be better to isolate it from the changes to the color palette application

I thought about it but I made these changes simultaneously so it would require some nontrivial history rewriting.

@pstch
Copy link
Contributor

pstch commented Nov 22, 2019

I missed something in my review:

  • The old definition of i18n.consoleKeyMap has not been removed (in i18n.nix)
  • The XML of the release note is broken.

I'm rebasing your commits, fixing both of these issues, and will push my branch so that you can use it if you want. I also removed the changes to the color palette application by diffing the two files, so that we can add them in another PR.

@pstch
Copy link
Contributor

pstch commented Nov 22, 2019

@rnhmjoj I rebased your PR on recent master, applying the following steps:

  • fixing the XML in the release notes, removing the commit for indentation that doesn't apply anymore
  • removing the changes to the color palette application
  • removing the old definition of i18n.consoleKeyMap in i18n.nix

I have tested these commits on my machine by rebasing them on 19.09.

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Nov 22, 2019

@pstch Thank you. I fixed the merge conflict and followed your suggestions.

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Nov 28, 2019

Fixed yet another conflict.

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Dec 2, 2019

This time it was me. I wonder why git can't automaticallt merge this...

nixos/modules/config/console.nix Outdated Show resolved Hide resolved
This commit moves all the virtual console related options
to a dedicated config/console.nix NixOS module.

Currently most of these are defined in config/i18n.nix
with a "console" prefix like `i18n.consoleFont`,
`i18n.consoleColors` or under `boot` and are implemented
in tasks/kbd.nix.
Since they have little to do with actual internationalisation
and are (informally) in an attrset already, it makes sense to
move them to a specific module.
This commit changes the console colors implementation
to use the kernel parameters instead of relying on terminal
escape sequences. This means the palette is applied by the
kernel itself with no custom code running in the initrd
and works for all virtual terminals (not only tty0).
@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Dec 19, 2019

@infinisil What to do you think about adding a new top level attribute? I'm not too keen but I didn't find a suitable existing one.

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Sounds like a good idea to me, it really doesn't fit anywhere else.

I tested this in a VM, seems to work fine, let's merge :)

@infinisil infinisil merged commit d475361 into NixOS:master Dec 20, 2019
@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Dec 20, 2019

Thank you!

@rnhmjoj rnhmjoj deleted the console branch January 8, 2020 17:27
@rnhmjoj rnhmjoj mentioned this pull request Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants