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

@rnhmjoj rnhmjoj requested a review from Infinisil as a code owner Oct 20, 2019
@rnhmjoj rnhmjoj force-pushed the rnhmjoj:console branch 2 times, most recently from c7e2b2d to 211afb4 Oct 25, 2019
@rnhmjoj rnhmjoj force-pushed the rnhmjoj:console branch 2 times, most recently from 8230905 to f6d1d3e Nov 3, 2019
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Nov 10, 2019

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

@c0bw3b c0bw3b added this to the 20.03 milestone Nov 10, 2019
@rnhmjoj rnhmjoj force-pushed the rnhmjoj:console branch from f6d1d3e to a1e1937 Nov 16, 2019
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Nov 16, 2019

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 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 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 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 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 rnhmjoj force-pushed the rnhmjoj:console branch from a1e1937 to 0d79a2b Nov 22, 2019
@rnhmjoj
Copy link
Contributor Author

@rnhmjoj rnhmjoj commented Nov 22, 2019

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

@rnhmjoj rnhmjoj force-pushed the rnhmjoj:console branch from 0d79a2b to 4223f2a Nov 28, 2019
@rnhmjoj
Copy link
Contributor Author

@rnhmjoj rnhmjoj commented Nov 28, 2019

Fixed yet another conflict.

@rnhmjoj rnhmjoj force-pushed the rnhmjoj:console branch from 4223f2a to fa45d25 Dec 2, 2019
@rnhmjoj
Copy link
Contributor Author

@rnhmjoj rnhmjoj commented Dec 2, 2019

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

@rnhmjoj rnhmjoj force-pushed the rnhmjoj:console branch from fa45d25 to 2c924fe Dec 4, 2019
@rnhmjoj rnhmjoj force-pushed the rnhmjoj:console branch from 2c924fe to 0d9b012 Dec 19, 2019
nixos/modules/config/console.nix Outdated Show resolved Hide resolved
rnhmjoj added 4 commits Oct 20, 2019
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 rnhmjoj force-pushed the rnhmjoj:console branch from 0d9b012 to a35b12e Dec 19, 2019
@rnhmjoj
Copy link
Contributor Author

@rnhmjoj 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

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
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@rnhmjoj
Copy link
Contributor Author

@rnhmjoj rnhmjoj commented Dec 20, 2019

Thank you!

@rnhmjoj rnhmjoj deleted the rnhmjoj:console branch Jan 8, 2020
@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
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.