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.

Sign up for GitHub

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

Jump to bottom

installer: autorun xserver in graphical media #46997

Merged
merged 3 commits into from Oct 5, 2019
Merged

installer: autorun xserver in graphical media #46997

merged 3 commits into from Oct 5, 2019

Conversation

worldofpeace
Copy link
Contributor

Motivation for this change

When I first used the NixOS iso I found it confusing and strange to have to do systemctl start display-manager. I assumed that I would have went straight to the graphical environment.

If anyone has a reason as to why it shouldn't do this please share.

Notes

In #42610 @adisbladis suggested:

I think we should add another boot entry so you can still choose not to start the graphical environment, at the same time we make the one that autostarts the graphical environment the default.

Wouldn't this need #45345 to include that configuration as a child config in the grub menu?

Though I feel that might be to complex.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Fits CONTRIBUTING.md.

cc @samueldr @xeji @grahamc

@samueldr
Copy link
Member

samueldr commented Sep 20, 2018 •
edited

I would like to know why it wasn't the default. There is probably a reason behind, and knowing it helps ensuring we don't cause any unintended issues.

Though, I think this requires the one important sub-feature: the installer media should present an option that disables the display-manager.service so one can use the graphical iso without having X started; in case starting X is causing issues. IIRC this can be done through a systemd.mask=display-manager.service option on the kernel command line.

@worldofpeace
Copy link
Contributor Author

I would like to know why it wasn't the default. There is probably a reason behind, and knowing it helps ensuring we don't cause any unintended issues.

It seems it was a while ago

nixpkgs/modules/profiles/graphical.nix

Line 12 in f3cc01b

autorun = true;

Though, I think this requires the one important sub-feature: the installer media should present an option that disables the display-manager.service so one can use the graphical iso without having X started; in case starting X is causing issues

Was this perceived as a necessary addendum to the feature suggested by @adisbladis?
If so, could you justify the importance?

@samueldr
Copy link
Member

Yes, sorry, bad habit, jumped the gun and added my thoughts before reading that this was already in the body of your PR.

This is one way the boot option can be implemented.

@worldofpeace
Copy link
Contributor Author

worldofpeace commented Sep 20, 2018 •
edited

No problem, I'm guilty of the same thing too when I'm busy.

@worldofpeace
Copy link
Contributor Author

If there's an agreement to needing this feature implicitly then I'd say that this is blocked until
the tests for #45345 are reviewed and it's merged.

@samueldr
Copy link
Member

samueldr commented Sep 20, 2018 •
edited

Unless I'm misunderstanding, probably no relation. The installer iso has a different menu generation entirely from the installed system.

Around here a new option can be added (class is used to get an icon in the menu).

@mmahut
Copy link
Member

mmahut commented Aug 9, 2019

Any updates on this pull request, please?

@worldofpeace
Copy link
Contributor Author

I plan to pick this up again after the 19.09 release.

@worldofpeace
Copy link
Contributor Author

Ok, rebased and added a quirk to disable the display-manager.
I think we might've discussed on having a way to condition menu entries on whether it's for the graphical media or minimal. Though I still think this change won't do much harm without that feature.

@worldofpeace
Copy link
Contributor Author

worldofpeace commented Sep 28, 2019 •
edited

I've tested running the graphical media

  • Does it autorun X starting plasma5 ✔️
  • Did enabling the quirk send me to the tty ✔️

@worldofpeace worldofpeace added this to the 20.03 milestone Sep 28, 2019
samueldr
samueldr reviewed Sep 29, 2019
View reviewed changes
nixos/modules/installer/cd-dvd/iso-image.nix
# If we boot into a graphical environment where X is autorunned
# and always crashes, it makes the media unusable. Allow the user
# to disable this.
submenu "Disable display-manager" --class quirk-disable-displaymanager {
Copy link
Member

Choose a reason for hiding this comment

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

We can introduce an icon, if desired, and if one can be composed. They are composed from the breeze icon theme. If you see something in it that can be adapted, or used as an inspiration I can look into adding it to the relevant artwork.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found breeze/devices/64/video-display.svg the most suitable.
I mostly wanted something like just a blank monitor, or communicates a tty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm highly partial to elementary's icons which appears to be exactly what I'm looking for

Screenshot from 2019-09-28 21 14 48

Copy link
Member

Choose a reason for hiding this comment

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

Oh, forgot to specify the symbolic icons. Though the one from breeze, once made into outlines, may work, I'll try to remember to look into it later. (The PR can be merged without that change.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, there's computer/video-display symbolic icons that should be fine then.
I'll add an issue at nixos-artwork for that once merged @samueldr.

samueldr
samueldr requested changes Sep 29, 2019
View reviewed changes
Copy link
Member

@samueldr samueldr left a comment

Choose a reason for hiding this comment

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

Minor stuff, generally approving the change.

nixos/doc/manual/release-notes/rl-2003.xml Outdated Show resolved Hide resolved
nixos/doc/manual/release-notes/rl-2003.xml Outdated Show resolved Hide resolved
nixos/doc/manual/release-notes/rl-2003.xml Outdated Show resolved Hide resolved
@worldofpeace
Copy link
Contributor Author

@samueldr My English and personality is literally a neologism 🤣

Thanks for the suggestions, will try to implement.

@worldofpeace
Copy link
Contributor Author

Rebased and resolved the conflicts in the release note.
I plan to integrate this some time today if there's no issues raised.

@worldofpeace worldofpeace merged commit 789fceb into NixOS:master Oct 5, 2019
@worldofpeace worldofpeace deleted the installer-autorun branch October 5, 2019 11:34
@worldofpeace worldofpeace mentioned this pull request Jan 27, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samueldr samueldr Awaiting requested review from samueldr samueldr is a code owner

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
20.03
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@worldofpeace @samueldr @mmahut @GrahamcOfBorg