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: refactor #46510

Merged
merged 1 commit into from Sep 20, 2018
Merged

installer: refactor #46510

merged 1 commit into from Sep 20, 2018

Conversation

worldofpeace
Copy link
Contributor

Motivation for this change

Borrow the harmless refactoring in #42610

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.

@samueldr
Copy link
Member

samueldr commented Sep 11, 2018 •
edited

Hi! Thanks for the PR!

Could you write a small human-friendly overview of the borrowed refactor?

(At a glance, it looks like it de-duplicates files, anything else to watch out for?)

worldofpeace
worldofpeace commented Sep 11, 2018
View reviewed changes
nixos/modules/installer/cd-dvd/installation-cd-graphical-base.nix
# Include some editors.
pkgs.vim
pkgs.bvi # binary editor
pkgs.joe
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need joe and bvi?

Copy link
Member

Choose a reason for hiding this comment

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

Not really, though bvi only adds 136K and joe clocks in at 2.3M, hardly anything to worry about (both on x86_64).

@worldofpeace
Copy link
Contributor Author

It adds kate to environment.systemPackages in installation-cd-graphical-kde.nix because that's an expected consistency to at least have a gui text editor.

We still default to using slim for a display manager, but it's disabled for installation-cd-graphical-gnome.nix because we default to gdm there. Prior to this it was enabling both gdm and slim which obviously didn't work.

There is a comment which I no longer know is true

GDM doesn't start in virtual machines with ISO

Verification is needed if this is still a problem (git blame says 3 years ago).

The rest is just de-duplication by adding installation-cd-graphical-base.nix for the shared configs between gnome and kde.

@adisbladis adisbladis merged commit b0987f2 into NixOS:master Sep 20, 2018
@adisbladis
Copy link
Member

I had issues running gnome in qemu because it requires accelerated graphics. Maybe GDM is the same?

@worldofpeace
Copy link
Contributor Author

It could be so, though I've had both working regularly in qemu.
Though testing shouldn't be important because installation-cd-graphical-gnome.nix isn't used.

@worldofpeace worldofpeace deleted the installer-refactor branch September 20, 2018 15:04
@oxij
Copy link
Member

oxij commented Sep 23, 2018 via email

@oxij oxij mentioned this pull request Sep 23, 2018
Merged
1 task
@samueldr
Copy link
Member

The previous discussion explains that it would be because Gnome doesn't have a desktop anymore. (I haven't verified.)

@oxij
Copy link
Member

oxij commented Sep 23, 2018

I see, still, the bottom of *-kde.nix looks pretty useful for all graphical installers, is it not?

@oxij
Copy link
Member

oxij commented Sep 23, 2018

I mean, konsole part isn't but the rest is. Like if we ever want to make an Xfce installer (which I would wholeheartedly vote for, as it is the most KISS of the big three DEs) or something, it would get those icons automagically.

@worldofpeace
Copy link
Contributor Author

@samueldr Yes GNOME removed this feature.

@oxij Maybe installation-cd-graphical-base.nix should have a mkDesktopItem (or something) in that case.

@oxij oxij mentioned this pull request Sep 23, 2018
Merged
@matthewbauer matthewbauer mentioned this pull request Sep 23, 2018
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adisbladis adisbladis adisbladis left review comments

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@worldofpeace @samueldr @adisbladis @oxij @GrahamcOfBorg