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

plymouth: attempt to make flicker-free #84158

Closed
wants to merge 7 commits into from

Conversation

eadwu
Copy link
Member

@eadwu eadwu commented Apr 3, 2020

If you rig up some plymouth prompts it should work fine for decryption and messages now. Patches are a big mess.

Not sure what's actually needed in stage-1 for this to run as early as possible.

Motivation for this change

label-ft: https://gitlab.freedesktop.org/plymouth/plymouth/-/merge_requests/91

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.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Apr 3, 2020
@ofborg ofborg bot requested a review from cillianderoiste April 3, 2020 02:08
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 labels Apr 3, 2020
@matthewbauer matthewbauer requested review from abbradar and hedning April 3, 2020 15:43
@matthewbauer
Copy link
Member

Awesome work! Haven't tried it yet, but this is definitely nice to have.

On stage 1, I think we may need to use the systemd integration more for this to work with plymouth. I'm not sure what this takes, but it had been brought up at one point.

Related to #26722 and #44965

@eadwu
Copy link
Member Author

eadwu commented Apr 3, 2020

Right now I'm using a makeshift wrapper for reading and showing messages, but the implementation is works only on bash due to the requirement on type.

    if [ ! "$(type -t _read)" = "function" ]; then
      _read() {
        local pwd
        pwd="$(plymouth ask-for-password "$@")"
        printf "$pwd"
      }
    fi

    if [ ! "$(type -t _prompt)" = "function" ]; then
      _prompt() {
        plymouth display-message "$@"
      }
    fi

but this ends up becoming far over-complicated because of the differing usages of outputting within initrd (or unless we enforce a strict format for the function, like for _read plymouth can only do --text "$1" and _prompt can only do --text "$1" which would probably work just fine).

Not sure on how to replicate non-newline displaying of messages, but it can be partially mitigated by having a sort of message history in the plymouth theme itself (bottom right on the image).
Screenshot_2020-04-03_12-49-38

@worldofpeace
Copy link
Contributor

cc @mkg20001

@matthewbauer
Copy link
Member

I'm getting this when building an OVA image:

output '/nix/store/d8yv4bc0kw9j3dg9fc3n2lp8qlljf6i4-extra-utils' is not allowed to refer to the following paths:
  /nix/store/il5jhmg7m01sflpz1g582dvwsii6byb9-plymouth-2020-03-25

@eadwu eadwu force-pushed the plymouth/less-flicker branch from 4d02077 to 02f53d8 Compare April 4, 2020 23:22
@eadwu
Copy link
Member Author

eadwu commented Apr 4, 2020

Should've fixed. Seemed to be a problem that occurred with the default settings.

Tried booting it up with the default theme and it should work fine as long as the password issue is resolved for passwords (without bash-specific workarounds). I'll work on that when I'm able to work on the PR for an extended period of time.

The default theme also has a problem with the password prompt "bullets" which I think is probably due to the font not containing the character.

local.bullet_image = WriteText("•", palette.text.contrast);

@eadwu eadwu force-pushed the plymouth/less-flicker branch from 02f53d8 to 261bea5 Compare April 5, 2020 18:41
@worldofpeace
Copy link
Contributor

Not sure what's actually needed in stage-1 for this to run as early as possible.

I believe from the people I've talked to about this, it would be #74842.

@worldofpeace worldofpeace requested a review from jtojnar April 6, 2020 22:44
@eadwu eadwu force-pushed the plymouth/less-flicker branch from 261bea5 to 74af0f3 Compare April 7, 2020 18:16
@lukateras lukateras self-requested a review April 8, 2020 10:58
@eadwu eadwu force-pushed the plymouth/less-flicker branch 2 times, most recently from 6df8088 to 039b3ed Compare April 10, 2020 21:35
pkgs/os-specific/linux/plymouth/default.nix Show resolved Hide resolved
pkgs/os-specific/linux/plymouth/default.nix Show resolved Hide resolved
pkgs/os-specific/linux/plymouth/default.nix Outdated Show resolved Hide resolved
# ./only_use_fb_for_cirrus_bochs.patch
] ++ stdenv.lib.optionals withLabelFt [
# https://gitlab.freedesktop.org/plymouth/plymouth/-/issues/45
./v3-0001-ply-label-Don-t-crash-if-label-plugin-fails.patch
Copy link
Member

Choose a reason for hiding this comment

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

Is this patch set necessary for flicker-free experience? I would rather not include unmerged patches using legacy libraries (freetype).

Copy link
Member Author

Choose a reason for hiding this comment

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

It isn't necessary but without it, some of the functionality for some themes will be impacted, anything that uses display-message won't be shown since we don't bundle the original label.so since it's far more bloated than label-ft.so.

But if it's just the password prompt (basically a pile of images) and the background, then no it isn't needed.

@eadwu eadwu force-pushed the plymouth/less-flicker branch 2 times, most recently from 7e127d4 to f87fbba Compare April 13, 2020 19:42
@eadwu eadwu force-pushed the plymouth/less-flicker branch from f87fbba to 9075640 Compare April 23, 2020 15:37
@eadwu eadwu force-pushed the plymouth/less-flicker branch from 9075640 to 54fa026 Compare May 23, 2020 15:39
@eadwu eadwu force-pushed the plymouth/less-flicker branch from 54fa026 to 87e0a8f Compare July 2, 2020 20:46
@eadwu eadwu force-pushed the plymouth/less-flicker branch from 87e0a8f to f3cf371 Compare July 20, 2020 02:17
@eadwu eadwu force-pushed the plymouth/less-flicker branch from f3cf371 to 0209b8b Compare August 11, 2020 15:55
@eadwu eadwu force-pushed the plymouth/less-flicker branch from 0209b8b to 01f8df6 Compare September 8, 2020 16:00
@berbiche
Copy link
Member

berbiche commented Sep 16, 2020

How can I test this in my configuration?

Am I right to assume I would only need to update my nixpkgs ref to this PR?

@davidak
Copy link
Member

davidak commented Sep 16, 2020

@berbiche you have also to enable plymouth, but then it should work.

@eadwu eadwu force-pushed the plymouth/less-flicker branch from 01f8df6 to 7c91c8d Compare September 26, 2020 15:24
@eadwu eadwu force-pushed the plymouth/less-flicker branch from 7c91c8d to c1552fe Compare October 7, 2020 18:00
@eadwu eadwu force-pushed the plymouth/less-flicker branch from c1552fe to c98c1c5 Compare October 29, 2020 20:15
@eadwu eadwu force-pushed the plymouth/less-flicker branch from c98c1c5 to f14f826 Compare November 16, 2020 19:08
[Daemon]
ShowDelay=0
DeviceTimeout=8
Copy link
Member

Choose a reason for hiding this comment

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

Should there be a way to make this configurable?

@eadwu eadwu force-pushed the plymouth/less-flicker branch from f14f826 to 3128d14 Compare December 21, 2020 06:07
@davidak
Copy link
Member

davidak commented Dec 26, 2020

I don't see a difference. https://youtu.be/wHbl62EgrlE

The stage1 text is still shown, then a black screen, and after 20 seconds i see the lightdm login.

i'm using Pantheon desktop
EFI systemd-boot
no disk encryption

should i test on a system with disk encryption? or still too early to test?

@eadwu
Copy link
Member Author

eadwu commented Dec 26, 2020

Took a quick glance at the related PRs and still doesn't look like it's possible. The earliest I went was stage-2 I believe (I don't remember)?

I think I broke initrd without plymouth (or at least if there are any secrets needed to be inputted) with the latest commit but I'll just keep it there since I usually just had it as a stash locally.

@SuperSandro2000 SuperSandro2000 added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 18, 2021
@worldofpeace worldofpeace self-assigned this Feb 21, 2021
@worldofpeace worldofpeace mentioned this pull request Feb 22, 2021
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants