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

gnome-flashback: add option to remove gnome-panel, auto-generate wmName #113957

Merged
merged 1 commit into from
May 28, 2021

Conversation

chpatrick
Copy link
Contributor

I wanted to remove the wmName option since it would fail if it wasn't in the right format. I also added a flag to disable the GNOME panel.

  • 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.

@chpatrick chpatrick force-pushed the gnome-flashback-panel-fix branch 2 times, most recently from 6cfb7ce to ba63f7d Compare February 21, 2021 23:18
@ofborg ofborg bot added 6.topic: GNOME GNOME desktop environment and its underlying platform 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: 1-10 10.rebuild-linux: 1 labels Feb 21, 2021
@chpatrick chpatrick requested a review from jtojnar February 22, 2021 00:16
@@ -160,15 +160,9 @@ in
customSessions = mkOption {
type = types.listOf (types.submodule {
options = {
wmName = mkOption {
Copy link
Contributor

@worldofpeace worldofpeace Feb 22, 2021

Choose a reason for hiding this comment

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

you should use mkRemovedOption~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I don't think that works because the option is in a submodule. I added an assertion.

@chpatrick chpatrick force-pushed the gnome-flashback-panel-fix branch 3 times, most recently from dafbc54 to fe79988 Compare February 22, 2021 15:01
@bitonic
Copy link

bitonic commented Mar 8, 2021

@worldofpeace friendly bump, this is very useful if you want to run gnome flashback and a tiling WM such as i3.

requiredComponents = wmName: enableGnomePanel: "RequiredComponents=${lib.concatStringsSep ";" ([ wmName ] ++ requiredComponentsCommon enableGnomePanel ++ requiredComponentsGsd)};";

# Make an ASCII filename-friendly name for a wm configuration, similar to Nix.
mkWmName = args@{ wmLabel, wmCommand, enableGnomePanel }: builtins.substring 0 32 (builtins.hashString "sha256" (builtins.toJSON args));
Copy link
Member

Choose a reason for hiding this comment

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

A benefit of human readable name is that it is clearer in the logs.

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 think this is better for the user, since this name is only needed internally and this way it can't be named in a way that breaks things.

Copy link
Member

Choose a reason for hiding this comment

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

Alternately, we might use slugify wmLabel as the default value for the name and make it optional. Possibly suffix it with the hash to avoid conflicts.

slugify =
  let
    allowed = lib.lowerChars ++ lib.upperChars ++ lib.stringToCharacters "1234567890-_";
  in
    lib.stringAsChars (c: if builtins.elem c allowed then c else "_");

Copy link
Member

Choose a reason for hiding this comment

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

And types.strMatching "[a-zA-Z0-9_-]+" for the option would work.

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 think it's fine how it is now. This identifier is only needed to get GNOME to find it internally, and exposing it would be a leaky abstraction IMO.

Copy link
Member

@jtojnar jtojnar May 28, 2021

Choose a reason for hiding this comment

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

Actually, I just remembered that the identifier is needed for auto-login (services.xserver.displayManager.defaultSession) so this is technically another BC break.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is not particularly nice:

trace: Default graphical session, 'gnome-flashback-xmonad', not found.
Valid names for 'services.xserver.displayManager.defaultSession' are:
  gnome
  gnome-xorg
  gnome-flashback-Metacity-4175ca004519ae58f559894fbcaadd3f
  gnome-flashback-XMonad-8382231e25a9fde7159467bbc610bdbf
  sm.puri.Phosh

Copy link
Contributor

Choose a reason for hiding this comment

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

BC break

@jtojnar What's "BC"?

Copy link
Member

Choose a reason for hiding this comment

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

@nh2 backwards compatibility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtojnar OK fair enough, I put the wmName back in.

@jtojnar
Copy link
Member

jtojnar commented May 1, 2021

But generally, it looks okay so if you do not think these are worth addressing, we can merge it as is.

@chpatrick chpatrick force-pushed the gnome-flashback-panel-fix branch from fe79988 to d1d7b1c Compare May 2, 2021 20:41
@chpatrick
Copy link
Contributor Author

I've tried it on top of latest master but the custom session doesn't seem to work due to some systemd issues. I don't really know too much about that stuff, could you maybe take a look @worldofpeace?

@jtojnar
Copy link
Member

jtojnar commented May 2, 2021

They left the project. I will try to take a look when I have some more time.

@jtojnar
Copy link
Member

jtojnar commented May 2, 2021

Do you have any error message?

@chpatrick
Copy link
Contributor Author

@jtojnar I'm not sure what the exact thing is, but the WM doesn't seem to start. The systemd unit we use in the gnome-flashback is now in a directory instead of a file. I changed cp to copy the service.conf file inside, but I don't know if that's actually correct.

@jtojnar
Copy link
Member

jtojnar commented May 5, 2021

Oh, I see.

The systemd units containing @ sign in the name are instantiated templates, the name before @ being the name of the original unit.

I suspect that before, the whole [email protected] instance created by systemd was being replaced by the one from the package, which can lead to the flashback instance being outdated.

Likely, to prevent that, upstream decided to switch to drop-in overrides, which use the original instance auto-created by systemd from [email protected] and only override few specific fields listed in the .conf file.

So I would just copy what is now in the package, just like we did before. Do not forget to adjust to the new unit name.

@chpatrick chpatrick force-pushed the gnome-flashback-panel-fix branch from d1d7b1c to 351ff96 Compare May 27, 2021 22:44
@chpatrick
Copy link
Contributor Author

@jtojnar OK, it seems to work now!

@chpatrick chpatrick force-pushed the gnome-flashback-panel-fix branch from 351ff96 to a85e371 Compare May 28, 2021 00:37
@chpatrick chpatrick force-pushed the gnome-flashback-panel-fix branch 2 times, most recently from c2e1800 to da1e70d Compare May 28, 2021 10:25
@jtojnar
Copy link
Member

jtojnar commented May 28, 2021

The docs should be updated as well. Otherwise, looks good.

@chpatrick chpatrick force-pushed the gnome-flashback-panel-fix branch from da1e70d to 620ff94 Compare May 28, 2021 10:37
@chpatrick chpatrick force-pushed the gnome-flashback-panel-fix branch from 620ff94 to 424cd7d Compare May 28, 2021 11:10
@jtojnar jtojnar merged commit e923fc2 into NixOS:master May 28, 2021
@jtojnar
Copy link
Member

jtojnar commented May 28, 2021

Can confirm that it works, thanks.

@B4dM4n
Copy link
Contributor

B4dM4n commented May 28, 2021

This PR enables many Gnome services by default without setting any gnome related option:

services.xserver.desktopManager.gnome.flashback.enableMetacity = mkOption {
  default = true;
}
->
flashbackEnabled = cfg.flashback.enableMetacity || length cfg.flashback.customSessions > 0;
->
mkIf (cfg.enable || flashbackEnabled) {
      services.gnome.core-os-services.enable = true;
      services.gnome.core-shell.enable = true;
      services.gnome.core-utilities.enable = mkDefault true;
}

I noticed this because it breaks any configuration with networking.wireless.enable = true:

nix-instantiate ./nixos --arg configuration '{
  fileSystems."/".fsType = "tmpfs";
  boot.loader.grub.device = "/dev/null";
  networking.wireless.enable = true;
}'

error:
       Failed assertions:
       - You can not use networking.networkmanager with networking.wireless.
       Except if you mark some interfaces as <literal>unmanaged</literal> by NetworkManager.

@chpatrick
Copy link
Contributor Author

@jtojnar I know there were plans to allow enabling flashback but not gnome shell, was this done? If not, we should only apply flashback configs if gnome is enabled right?

jtojnar added a commit that referenced this pull request May 28, 2021
Did not realize this is not conditional on gnome-flashback being enabled.

Partially reverts #113957
@jtojnar
Copy link
Member

jtojnar commented May 28, 2021

Sorry about that, I forgot we do not have a separate flashback.enable option. Reverted back to mkEnableOption in b2f86e6.

chpatrick pushed a commit to chpatrick/nixpkgs that referenced this pull request Sep 6, 2021
Did not realize this is not conditional on gnome-flashback being enabled.

Partially reverts NixOS#113957
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: GNOME GNOME desktop environment and its underlying platform 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: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants