NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
17.34k stars 13.58k forks source link

ksysguard throw message is not helpful #148452

Open gilligan opened 2 years ago

gilligan commented 2 years ago

I just upgraded to 21.11 and tried to build my NixOS config which resulted in

building Nix...
building the system configuration...
error: ksysguard has been replaced with plasma-systemmonitor

Expected behavior

My suggestion would be to turn the error message into something more useful. Right now it doesn't provide any context or help to the user whatsoever.

Obviously we can't throw with an indication of where the ksysguard dependency comes from but the commit message and the code both indicate a workaround and my suggestion would be to also provide this to the user.

Maybe something like:

    } // lib.optionalAttrs (config.allowAliases or true) {
      ksysguard = throw "ksysguard has been replaced with plasma-systemmonitor. Please rename occurrences in your config or set allowAliases=true";
    };

Notify maintainers

@alyssais

alyssais commented 2 years ago

Actually, this is more confusing than I thought at first because i don't disable allowAliases anywhere and yet it still throws.

The throw isn't supposed to happen with allowAliases = false, it's supposed to (and does, AFAIK) happen with allowAliases = true. It's a bit confusing, but is how removal messages in Nixpkgs work. If you have aliases enabled, you get an error message, otherwise the attribute just doesn't exist.

nixos-discourse commented 2 years ago

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/aliasing-ksysguard/16906/1

edrex commented 2 years ago

As not7cd points out in discourse, the usability issue is that it's not now possible to install everything from plasma5Packages.plasma5 without explicitly listing the packages or turning off aliases. If the removal error should remain, is there some modification that can be made to this expression to filter out aliases? (or i guess filter out ksysguard specifically, although that still feels like a workaround)

 environment.systemPackages = with pkgs; 
 [] ++ builtins.filter lib.isDerivation (builtins.attrValues plasma5Packages.plasma5);

Does the removal message mechanism need some design tweak to better support this use case? @ttuegel @Ma27

edrex commented 1 year ago

Revisiting this in a new year, knowing more nixlang, I had the idea to catch the throw in the filter function, and that works. Here's what I did:

{ config, pkgs, lib, ... }:
let
  # filter out throws and non-derivations
  dervsFrom = s: builtins.filter (x: (builtins.tryEval x).success && (lib.isDerivation x)) (builtins.attrValues s);
in {
  environment.systemPackages = with pkgs; []
    ++ dervsFrom plasma5Packages.kdeGear
    ++ dervsFrom plasma5Packages.kdeFrameworks
    ++ dervsFrom plasma5Packages.plasma5;
}

It would be cool if there were a lib function for this (let's be real: there probably is but good luck finding it).