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

plasma-workspace: fix app_id by moving bin/.plasmashell-wrapped to #139213

Closed
wants to merge 1 commit into from

Conversation

Artturin
Copy link
Member

@Artturin Artturin commented Sep 23, 2021

bin/plasmashell.wrapped

Closes #118650
screenshot_2021-09-23_22-07-58_137426204

Motivation for this change
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages 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/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@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 Sep 23, 2021
@Artturin
Copy link
Member Author

Artturin commented Sep 23, 2021

to test:
put this in flake.nix and run nix run --no-write-lock-file

{
  inputs = {
    nixpkgs.url = "github:Artturin/nixpkgs/plasmashellid";
    nixpkgs1.url = "github:Artturin/nixpkgs/plasmawayland";
  };

  outputs = inputs@{ self, nixpkgs, nixpkgs1 }: {

    nixosConfigurations.vm = nixpkgs.lib.nixosSystem {
      system = "x86_64-linux";
      specialArgs = { inherit inputs; };
      modules = [
        ({ pkgs, ... }: {
          disabledModules = [ "services/x11/desktop-managers/plasma5.nix" ];
          imports = [
            "${inputs.nixpkgs1}/nixos/modules/services/x11/desktop-managers/plasma5.nix"

            ## For virtualisation settings
            "${inputs.nixpkgs}/nixos/modules/virtualisation/qemu-vm.nix"
          ];
          environment.systemPackages = with pkgs; [
            libnotify
          ];

          services.xserver = {
            enable = true;
            displayManager.sddm = {
              enable = true;
              settings.Wayland.SessionDir = 
                "${pkgs.plasma5Packages.plasma-workspace}/share/wayland-sessions";
            };
            desktopManager.plasma5 = {
              enable = true;
              runUsingSystemd = true;
            };
          };

          # Documentation for these is in nixos/modules/virtualisation/qemu-vm.nix
          virtualisation = {
            memorySize = 1024 * 3;
            diskSize = 1024 * 4;
            cores = 4;
            msize = 104857600;
          };

          boot.tmpOnTmpfs = true;
          documentation.enable = false;
          users.mutableUsers = false;
          users.defaultUserShell = pkgs.zsh;
          users.users.root = {
            password = "root";
          };
          users.users.user = {
            password = "user";
            isNormalUser = true;
            extraGroups = [ "wheel" ];
          };
        })
      ];
    };
    # So that we can just run 'nix run' instead of
    # 'nix build ".#nixosConfigurations.vm.config.system.build.vm" && ./result/bin/run-nixos-vm'
    defaultPackage.x86_64-linux = self.nixosConfigurations.vm.config.system.build.vm;
    defaultApp.x86_64-linux = {
      type = "app";
      program = "${self.defaultPackage.x86_64-linux}/bin/run-nixos-vm";
    };
  };
}

then in a terminal in the vm run notify-send test

@samueldr
Copy link
Member

samueldr commented Sep 23, 2021

Wouldn't this be better handled at the root location that is checking for binary name?

In a similar fashion to this one:

+ // While the parts needed are present, "unwrap" one layer of wrapper names.
+ while (executablePath.endsWith("-wrapped") && executablePath[executablePath.lastIndexOf("/")+1] == QChar('.')) {
+ // Approximately equivalent to s/-wrapped$//
+ executablePath.remove(executablePath.length() - 8, 8);
+ // Approximately equivalent to s;/\.;/;
+ executablePath.remove(executablePath.lastIndexOf("/")+1, 1);
+ }

We're likely to play whack-a-mole otherwise.

@r-rmcgibbo
Copy link

r-rmcgibbo commented Sep 23, 2021

Result of nixpkgs-review pr 139213 at b6b6dfa2 run on x86_64-linux 1

34 packages skipped due to time constraints:
  • kdev-php
  • kdev-python
  • kdevelop
  • kdevelop-unwrapped
  • libsForQt512.kde-cli-tools
  • libsForQt512.kdeplasma-addons
  • libsForQt512.khotkeys
  • libsForQt512.kinfocenter
  • libsForQt512.kmenuedit
  • libsForQt512.krohnkite
  • ...
16 packages built successfully:
  • libsForQt5.kde-cli-tools (libsForQt515.kde-cli-tools ,plasma5Packages.kde-cli-tools)
  • libsForQt5.kdeplasma-addons (libsForQt515.kdeplasma-addons ,plasma5Packages.kdeplasma-addons)
  • libsForQt5.khotkeys (libsForQt515.khotkeys ,plasma5Packages.khotkeys)
  • libsForQt5.kinfocenter (libsForQt515.kinfocenter ,plasma5Packages.kinfocenter)
  • libsForQt5.kmenuedit (libsForQt515.kmenuedit ,plasma5Packages.kmenuedit)
  • libsForQt5.krohnkite (libsForQt515.krohnkite ,plasma5Packages.krohnkite)
  • libsForQt5.kwin-dynamic-workspaces (libsForQt515.kwin-dynamic-workspaces ,plasma5Packages.kwin-dynamic-workspaces)
  • libsForQt5.kwin-tiling (libsForQt515.kwin-tiling ,plasma5Packages.kwin-tiling)
  • libsForQt5.parachute (libsForQt515.parachute ,plasma5Packages.parachute)
  • libsForQt5.plasma-browser-integration (libsForQt515.plasma-browser-integration ,plasma5Packages.plasma-browser-integration)
  • libsForQt5.plasma-desktop (libsForQt515.plasma-desktop ,plasma5Packages.plasma-desktop)
  • libsForQt5.plasma-disks (libsForQt515.plasma-disks ,plasma5Packages.plasma-disks)
  • libsForQt5.plasma-workspace (libsForQt515.plasma-workspace ,plasma5Packages.plasma-workspace)
  • libsForQt5.powerdevil (libsForQt515.powerdevil ,plasma5Packages.powerdevil)
  • libsForQt5.systemsettings (libsForQt515.systemsettings ,plasma5Packages.systemsettings)
  • wacomtablet

Result of nixpkgs-review pr 139213 at b6b6dfa2 run on aarch64-linux 1

33 packages skipped due to time constraints:
  • kdev-php
  • kdev-python
  • kdevelop-unwrapped
  • libsForQt512.kde-cli-tools
  • libsForQt512.kdeplasma-addons
  • libsForQt512.khotkeys
  • libsForQt512.kinfocenter
  • libsForQt512.kmenuedit
  • libsForQt512.krohnkite
  • libsForQt512.kwin-dynamic-workspaces
  • ...
16 packages built successfully:
  • libsForQt5.kde-cli-tools (libsForQt515.kde-cli-tools ,plasma5Packages.kde-cli-tools)
  • libsForQt5.kdeplasma-addons (libsForQt515.kdeplasma-addons ,plasma5Packages.kdeplasma-addons)
  • libsForQt5.khotkeys (libsForQt515.khotkeys ,plasma5Packages.khotkeys)
  • libsForQt5.kinfocenter (libsForQt515.kinfocenter ,plasma5Packages.kinfocenter)
  • libsForQt5.kmenuedit (libsForQt515.kmenuedit ,plasma5Packages.kmenuedit)
  • libsForQt5.krohnkite (libsForQt515.krohnkite ,plasma5Packages.krohnkite)
  • libsForQt5.kwin-dynamic-workspaces (libsForQt515.kwin-dynamic-workspaces ,plasma5Packages.kwin-dynamic-workspaces)
  • libsForQt5.kwin-tiling (libsForQt515.kwin-tiling ,plasma5Packages.kwin-tiling)
  • libsForQt5.parachute (libsForQt515.parachute ,plasma5Packages.parachute)
  • libsForQt5.plasma-browser-integration (libsForQt515.plasma-browser-integration ,plasma5Packages.plasma-browser-integration)
  • libsForQt5.plasma-desktop (libsForQt515.plasma-desktop ,plasma5Packages.plasma-desktop)
  • libsForQt5.plasma-disks (libsForQt515.plasma-disks ,plasma5Packages.plasma-disks)
  • libsForQt5.plasma-workspace (libsForQt515.plasma-workspace ,plasma5Packages.plasma-workspace)
  • libsForQt5.powerdevil (libsForQt515.powerdevil ,plasma5Packages.powerdevil)
  • libsForQt5.systemsettings (libsForQt515.systemsettings ,plasma5Packages.systemsettings)
  • wacomtablet

@Artturin
Copy link
Member Author

Wouldn't this be better handled at the root location that is checking for binary name?

In a similar fashion to this one:

+ // While the parts needed are present, "unwrap" one layer of wrapper names.
+ while (executablePath.endsWith("-wrapped") && executablePath[executablePath.lastIndexOf("/")+1] == QChar('.')) {
+ // Approximately equivalent to s/-wrapped$//
+ executablePath.remove(executablePath.length() - 8, 8);
+ // Approximately equivalent to s;/\.;/;
+ executablePath.remove(executablePath.lastIndexOf("/")+1, 1);
+ }

We're likely to play whack-a-mole otherwise.

#139213 (comment)

@andrevmatos
Copy link
Member

My 2 cents: It seems the fix has 2 requirements which kind of conflict with each other with current makeWrapper approach:

  • the wrapped binary ideally would keep its original name, and probably stay in its original folder relative to its assets and other binaries (in case it tries to do nasty things like path.join(dirname($0), '/sibling_bin')
  • the wrapper script probably should be accessible from package/bin/orig_bin_name, in order to remain compatible with nix code that expect the binary to be in that path

A possible generic solution I can see would be the original package and its unwrapped binaries be put in a separate derivation, in this case, an internal plasma-workspace-unwrapped package, and plasma-workspace/bin/plasmashell would then wrap/exec xxxx-plasma-workspace-unwrapped/bin/plasmashell.

This would then possibly add the problem where other packages that expect other files (other binaries, share, lib, etc) to be present in plasma-workspace, but maybe symbolic links to the unwrapped packages could ensure compatibility. This way, both wrapped and unwrapped executables would keep the original name (fixing said issue), assets would be accessible from both pkgs paths and relative paths would keep working.
Could something like that work?

@Artturin
Copy link
Member Author

Artturin commented Sep 24, 2021

I have discovered a better/simpler solution which is to just change the - in -wrapped to a .

@Artturin
Copy link
Member Author

oh... like this no programs appear in the task bar

@Artturin Artturin marked this pull request as draft September 24, 2021 18:59
@Artturin Artturin closed this Sep 26, 2021
@Artturin Artturin deleted the plasmashellid branch October 23, 2021 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: qt/kde 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.

Notifications and other popups are treated like "normal" windows on Plasma Wayland
6 participants