plymouth: set custom font #339
Conversation
|
@trueNAHO Requesting a review since you've been doing a lot of work around bash scripts recently, so you may be interested in this new one. |
modules/plymouth/nixos.nix
Outdated
| mkPlymouthFont = | ||
| font: | ||
| pkgs.runCommand "${font.package.name}.ttf" | ||
| { FONTCONFIG_FILE = pkgs.makeFontsConf { fontDirectories = [ font.package ]; }; } | ||
| '' | ||
| matchingFonts=$(${pkgs.fontconfig}/bin/fc-match -asf "%{family}|%{file}\n" "${font.name}" | grep "^${font.name}|" | cut -d'|' -f2) | ||
|
|
||
| if [[ -z "$matchingFonts" ]]; then | ||
| echo "No font named '${font.name}' found." | ||
| exit 1 | ||
| fi | ||
|
|
||
| if ttfFont=$(echo "$matchingFonts" | grep -m1 -E "\.ttf$"); then | ||
| cp "$ttfFont" "$out" | ||
| else | ||
| font=$(echo "$matchingFonts" | head -n1) | ||
| ${pkgs.fontforge}/bin/fontforge -lang=ff -c 'Open($1); Generate($2); Close()' "$font" "$out" | ||
| fi | ||
| ''; |
There was a problem hiding this comment.
Suggestions
Note: I have not tested the code. Feel free to fix any emerging bugs.
I suggest the following changes:
- Reformat the code
- Wrap the code at 80 characters
- Merge the first
if-statement - Expand short flag names into their longer form
- Merge the
grepandcutcommands into one (faster)awkcommand - Use
verboseEchoinstead ofechofor logging - Improve logging message structure to simplify
grepping logs - Replace non-logging
echocommands withprintf '%s\n' - Replace
${pkgs.<PACKAGE>}/bin/<COMMAND>with${lib.getExe' <PACKAGE> <COMMAND>} - Add
TODO:comments.
| mkPlymouthFont = | |
| font: | |
| pkgs.runCommand "${font.package.name}.ttf" | |
| { FONTCONFIG_FILE = pkgs.makeFontsConf { fontDirectories = [ font.package ]; }; } | |
| '' | |
| matchingFonts=$(${pkgs.fontconfig}/bin/fc-match -asf "%{family}|%{file}\n" "${font.name}" | grep "^${font.name}|" | cut -d'|' -f2) | |
| if [[ -z "$matchingFonts" ]]; then | |
| echo "No font named '${font.name}' found." | |
| exit 1 | |
| fi | |
| if ttfFont=$(echo "$matchingFonts" | grep -m1 -E "\.ttf$"); then | |
| cp "$ttfFont" "$out" | |
| else | |
| font=$(echo "$matchingFonts" | head -n1) | |
| ${pkgs.fontforge}/bin/fontforge -lang=ff -c 'Open($1); Generate($2); Close()' "$font" "$out" | |
| fi | |
| ''; | |
| mkPlymouthFont = font: | |
| pkgs.runCommand "${font.package.name}.ttf" | |
| {FONTCONFIG_FILE = pkgs.makeFontsConf {fontDirectories = [font.package];};} | |
| '' | |
| if ! matchingFonts="$( | |
| # TODO: Explain why the '--sort' flag is needed. | |
| ${lib.getExe' pkgs.fontconfig "fc-match"} \ | |
| --all \ | |
| --format '%{family}|%{file}\n' \ | |
| '${font.name}' \ | |
| --sort | | |
| awk --field-separator '|' '/^${font.name}\|/ { print $2 }' | |
| )"; then | |
| verboseEcho "Font not found: '${font.name}'" | |
| exit 1 | |
| fi | |
| if ttfFont="$( | |
| # TODO: Explain why the '--max-count' flag is needed, if the | |
| # previous '--sort' flag is required. | |
| printf '%s\n' "$matchingFonts" | | |
| grep --extended-regexp --max-count 1 "\.ttf$" | |
| )"; then | |
| cp "$ttfFont" "$out" | |
| else | |
| # TODO: Explain why only the first line is needed, if the previous | |
| # '--sort' flag is required. | |
| font=$(printf '%s\n' "$matchingFonts" | head --lines 1) | |
| ${lib.getExe' pkgs.fontforge "fontforge"} \ | |
| -lang=ff \ | |
| -c 'Open($1); Generate($2); Close()' \ | |
| "$font" \ | |
| "$out" | |
| fi | |
| ''; |
Unresolved Questions
Resolve the suggested TODO: comments.
There was a problem hiding this comment.
verboseEcho is only defined within home.activation scripts; a normal echo or printf should be used here.
There was a problem hiding this comment.
verboseEchois only defined withinhome.activationscripts; a normalechoorprintfshould be used here.
Good catch. In that case, we should print to stderr with:
printf 'Font not found: %s' '${font.name}'" >&2Here is the updated suggestion:
| mkPlymouthFont = | |
| font: | |
| pkgs.runCommand "${font.package.name}.ttf" | |
| { FONTCONFIG_FILE = pkgs.makeFontsConf { fontDirectories = [ font.package ]; }; } | |
| '' | |
| matchingFonts=$(${pkgs.fontconfig}/bin/fc-match -asf "%{family}|%{file}\n" "${font.name}" | grep "^${font.name}|" | cut -d'|' -f2) | |
| if [[ -z "$matchingFonts" ]]; then | |
| echo "No font named '${font.name}' found." | |
| exit 1 | |
| fi | |
| if ttfFont=$(echo "$matchingFonts" | grep -m1 -E "\.ttf$"); then | |
| cp "$ttfFont" "$out" | |
| else | |
| font=$(echo "$matchingFonts" | head -n1) | |
| ${pkgs.fontforge}/bin/fontforge -lang=ff -c 'Open($1); Generate($2); Close()' "$font" "$out" | |
| fi | |
| ''; | |
| mkPlymouthFont = font: | |
| pkgs.runCommand "${font.package.name}.ttf" | |
| {FONTCONFIG_FILE = pkgs.makeFontsConf {fontDirectories = [font.package];};} | |
| '' | |
| if ! matchingFonts="$( | |
| # TODO: Explain why the '--sort' flag is needed. | |
| ${lib.getExe' pkgs.fontconfig "fc-match"} \ | |
| --all \ | |
| --format '%{family}|%{file}\n' \ | |
| '${font.name}' \ | |
| --sort | | |
| awk --field-separator '|' '/^${font.name}\|/ { print $2 }' | |
| )"; then | |
| printf 'Font not found: %s' '${font.name}'" >&2 | |
| exit 1 | |
| fi | |
| if ttfFont="$( | |
| # TODO: Explain why the '--max-count' flag is needed, if the | |
| # previous '--sort' flag is required. | |
| printf '%s\n' "$matchingFonts" | | |
| grep --extended-regexp --max-count 1 "\.ttf$" | |
| )"; then | |
| cp "$ttfFont" "$out" | |
| else | |
| # TODO: Explain why only the first line is needed, if the previous | |
| # '--sort' flag is required. | |
| font=$(printf '%s\n' "$matchingFonts" | head --lines 1) | |
| ${lib.getExe' pkgs.fontforge "fontforge"} \ | |
| -lang=ff \ | |
| -c 'Open($1); Generate($2); Close()' \ | |
| "$font" \ | |
| "$out" | |
| fi | |
| ''; |
Also, should we apply lib.escapeShellArg on variables like font.name, similar to #318 ?
There was a problem hiding this comment.
Yes, I think that would be appropriate.
There was a problem hiding this comment.
lib.escapeShellArg seems to only add single quotes around the variable. This breaks the awk program and i haven't found a reason for escaping anything else in the script. To be clear im not against using it, if anyone can get it to work how it should be i would appreciate it.
There was a problem hiding this comment.
lib.escapeShellArgseems to only add single quotes around the variable.
Yes, that is literally all it does:
https://github.com/NixOS/nixpkgs/blob/23.11/lib/strings.nix#L435-L443.
This breaks the awk program
Indeed. Yesterday, I actually stumbled across the same issue myself.
and i haven't found a reason for escaping anything else in the script. To be clear im not against using it, if anyone can get it to work how it should be i would appreciate it.
In my case, I ignored the escaping to simplify the implementation. However, I see two ways of solving this problem in the following descending order of weirdness:
- Concatenate the AWK code:
awk '<BEFORE>'"${lib.escapeShellArg <ARGUMENT>}"'<AFTER>'. - Provide an AWK variable:
awk --assign <VARIABLE>=${lib.escapeShellArg <ARGUMENT>} '<CODE>'.
Btw, very nice error propagation in the matchingFonts variable with the awk script :)
|
I have since read more about fonts in plymouth and as i understand it only truetype and opentype are supported with opentype being the preferred format. I am not sure if there is a way to have the derivation point to a |
| printf 'Font not found: %s' '${font.name}' >&2 | ||
| exit 1 | ||
| fi | ||
| echo "fonts: '$matchingFonts'" >&2 |
There was a problem hiding this comment.
Would this not be echoed on every build? Should we remove this debugging statement:
| echo "fonts: '$matchingFonts'" >&2 |
I have not done any further research on this, but would it be possible to extract the font types from certain paths? Maybe the For reference, there is an open issue regarding fonts: #308. |
Sets the user configured font in plymouth. On my setup plymouth only seemed to accept / work with truetype fonts and i didn't find any documentation for plymouth mentioning fonts at all, so the script checks for that first and falls back to converting another font format to ttf with fontforge.