nixos/plymouth: fix theme dependency resolution in systemd stage 1 #179619
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some plymouth themes use assets of others, like is the case with our default bgrt depending on spinner. Missing assets would cause the splashscreen to not render at all in stage 1. Preliminary dependency resolution code seemed to be broken, and this should fix it. Only direct dependencies of selected theme are pulled in.
|
Ping @NixOS/systemd for visibility. |
Majiir
reviewed
Aug 11, 2022
| cp -r "${themesEnv}/share/plymouth/themes/$theme" $out | ||
| fi | ||
| else | ||
| echo "Missing theme dependency: $theme" |
There was a problem hiding this comment.
Should we fail the build when there is a theme dependency missing?
Suggested change
| echo "Missing theme dependency: $theme" | |
| echo "Missing theme dependency: $theme" | |
| exit 1 |
|
My motivation behind just showing a warning and not crashing outright is
that, in general, the whole "theme dependency" is not really a standardized
thing. I can see how this could match eg. some path in a comment and error
out.
If someone tests out all default/nixpkgs packaged Plymouth themes and
checks if they work properly, we can probably switch it to exiting out on
missing dependencies.
Otherwise, to prevent any unexpected breakage, I would leave it as is, and
will just assume people will notice the warning if something goes wrong
(when trying out new themes).
…On Thu, 11 Aug 2022, 04:32 Majiir Paktu, ***@***.***> wrote:
***@***.**** commented on this pull request.
Tested this on nixos-22.05. It fixes an issue (#26722
<#26722>) where the graphical
password prompt is not shown for the default bgrt theme.
------------------------------
In nixos/modules/system/boot/plymouth.nix
<#179619 (comment)>:
> @@ -184,9 +184,14 @@ in
mkdir $out
cp -r ${themesEnv}/share/plymouth/themes/${cfg.theme} $out
# Copy more themes if the theme depends on others
- for theme in $(grep -hRo '/etc/plymouth/themes/.*$' ${themesEnv} | xargs -n1 basename); do
- if [[ -d "${themesEnv}/theme" ]]; then
- cp -r "${themesEnv}/theme" $out
+ for theme in $(grep -hRo '/etc/plymouth/themes/.*$' $out | xargs -n1 basename); do
+ if [[ -d "${themesEnv}/share/plymouth/themes/$theme" ]]; then
+ if [[ ! -d "$out/$theme" ]]; then
+ echo "Adding dependent theme: $theme"
+ cp -r "${themesEnv}/share/plymouth/themes/$theme" $out
+ fi
+ else
+ echo "Missing theme dependency: $theme"
Should we fail the build when there is a theme dependency missing?
⬇️ Suggested change
- echo "Missing theme dependency: $theme"
+ echo "Missing theme dependency: $theme"
+ exit 1
—
Reply to this email directly, view it on GitHub
<#179619 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIUEXRRJD7SETSRBUTB223VYRQ5DANCNFSM52HCRWUQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Thanks! |
|
Hi, can this be merged into |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes
Some plymouth themes use assets of others, like is the case with our
default bgrt depending on spinner. Missing assets would cause the
splashscreen to not render at all in stage 1.
Preliminary dependency resolution code seemed to be broken, and this
should fix it.
Only direct dependencies of selected theme are pulled in.
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes