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

boot.loader.grub: add theme option #83678

Merged
merged 1 commit into from Aug 2, 2020
Merged

Conversation

@mkg20001
Copy link
Member

@mkg20001 mkg20001 commented Mar 29, 2020

Motivation for this change

Added theme option for grub

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@mkg20001
Copy link
Member Author

@mkg20001 mkg20001 commented Mar 29, 2020

@Yutyo this is the thing you need

# Load theme fonts
";

find( { wanted => sub { # TODO: recursive glob for *.pf2 in theme, rewrite path from $theme/(.*) to $1

This comment has been minimized.

@mkg20001

mkg20001 Jul 5, 2020
Author Member

I want to get this hack removed before merge, anyone with good perl knowledge has any idea?

This comment has been minimized.

@mkg20001

mkg20001 Jul 29, 2020
Author Member

Breaks if the .pf2 file is in a subfolder of the theme, since we're referencing it by it's filename, not sub-path

This comment has been minimized.

@samueldr

samueldr Jul 30, 2020
Member

The wanted function takes no arguments but rather does its work through a collection of variables.

`$File::Find::dir` is the current directory name,
`$_` is the current filename within that directory
`$File::Find::name` is the complete pathname to the file.

https://perldoc.perl.org/File/Find.html#The-wanted-function

I believe you should be able to replace $_ with $File::Find::name to fix the issue with files in subfolders.

This comment has been minimized.

@samueldr

samueldr Jul 30, 2020
Member

Alternatively, could we instead leave it to the user to set font separately from theme? EDIT: no

It's not like it's a self-contained theme system anyway, which is why we have this hack.

Having distinct options allows more customization in the end. The user would use "${pkgs.nixos-grub-theme}/the/font.pf2" as a value in that instance.

Or am I missing something?

This comment has been minimized.

@mkg20001

mkg20001 Jul 30, 2020
Author Member

The theme specifies the font, we're just loading it

See ${nixos-grub2-theme}/theme.txt

This comment has been minimized.

@samueldr

samueldr Jul 30, 2020
Member

Right, I see what I did there.

It's been such a long time I don't remember all that :|

I figure then that this find is appropriate, given that AFAICT from superficial searches there is no recursive glob. That's the perl way to do it.

@mkg20001 mkg20001 force-pushed the mkg20001:add-theme-option branch 2 times, most recently from 50427eb to 46a42bd Jul 5, 2020
@mkg20001 mkg20001 force-pushed the mkg20001:add-theme-option branch from 46a42bd to 6a2c8e6 Jul 29, 2020
@worldofpeace worldofpeace requested review from samueldr, nh2 and edolstra Jul 30, 2020
Copy link
Member

@samueldr samueldr left a comment

Looks okay, though not big on perl, I don't see problems.

Though obviously, approval only once the discussion about the "hacky" function resolved.

# Load theme fonts
";

find( { wanted => sub { # TODO: recursive glob for *.pf2 in theme, rewrite path from $theme/(.*) to $1

This comment has been minimized.

@samueldr

samueldr Jul 30, 2020
Member

The wanted function takes no arguments but rather does its work through a collection of variables.

`$File::Find::dir` is the current directory name,
`$_` is the current filename within that directory
`$File::Find::name` is the complete pathname to the file.

https://perldoc.perl.org/File/Find.html#The-wanted-function

I believe you should be able to replace $_ with $File::Find::name to fix the issue with files in subfolders.

# Load theme fonts
";

find( { wanted => sub { # TODO: recursive glob for *.pf2 in theme, rewrite path from $theme/(.*) to $1

This comment has been minimized.

@samueldr

samueldr Jul 30, 2020
Member

Alternatively, could we instead leave it to the user to set font separately from theme? EDIT: no

It's not like it's a self-contained theme system anyway, which is why we have this hack.

Having distinct options allows more customization in the end. The user would use "${pkgs.nixos-grub-theme}/the/font.pf2" as a value in that instance.

Or am I missing something?

@mkg20001 mkg20001 force-pushed the mkg20001:add-theme-option branch 2 times, most recently from 898d008 to 03d817a Jul 31, 2020
@mkg20001 mkg20001 force-pushed the mkg20001:add-theme-option branch from 03d817a to 6382a15 Jul 31, 2020
@mkg20001
Copy link
Member Author

@mkg20001 mkg20001 commented Aug 1, 2020

merge?

Copy link
Member

@samueldr samueldr left a comment

I have tested the PR and, other than this documentation bug, it seems to work just fine.

So once this minor bug squashed, it's ready for a merge.

image

Co-authored-by: Eelco Dolstra <edolstra@gmail.com>

Co-authored-by: Samuel Dionne-Riel <samuel@dionne-riel.com>
@mkg20001 mkg20001 force-pushed the mkg20001:add-theme-option branch from 5d4ba4b to a7a0d79 Aug 2, 2020
@samueldr
Copy link
Member

@samueldr samueldr commented Aug 2, 2020

Thank you very much!

@samueldr samueldr merged commit 8857f40 into NixOS:master Aug 2, 2020
11 of 13 checks passed
11 of 13 checks passed
grahamcofborg-eval Calculating Changed Outputs
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a7a0d79"; rev="a7a0d79ef3fd0bf86847612597f4b62ce6ec5a18"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a7a0d79"; rev="a7a0d79ef3fd0bf86847612597f4b62ce6ec5a18"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a7a0d79"; rev="a7a0d79ef3fd0bf86847612597f4b62ce6ec5a18"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a7a0d79"; rev="a7a0d79ef3fd0bf86847612597f4b62ce6ec5a18"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a7a0d79"; rev="a7a0d79ef3fd0bf86847612597f4b62ce6ec5a18"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a7a0d79"; rev="a7a0d79ef3fd0bf86847612597f4b62ce6ec5a18"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="a7a0d79"; rev="a7a0d79ef3fd0bf86847612597f4b62ce6ec5a18"; } ./pkgs/t
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
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

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