qt: fix unintended leak from system to hm #1310
Conversation
stylix-automation
bot
added
topic: nixos
|
Let me know if I should add some more explanation. I'd also like to hear some feedback regarding the KDE defaults. |
trueNAHO
left a comment
trueNAHO
left a comment
There was a problem hiding this comment.
This fix prevents home-manager from initialising with defaults that potentially breakes plasma6 when the systemd module is not enabled.
I do not see how this patch would resolve such a problem.
|
I have this minimal configuration (running this branch, but the issue also happens on the main branch): {
inputs = {
nixpkgs.url = "github:NixOS/nixpkgs/nixpkgs-unstable";
stylix.url = "git+https://github.com/Mikilio/stylix?ref=qt-hm-fix";
home-manager = {
url = "github:nix-community/home-manager/master";
# The `follows` keyword in inputs is used for inheritance.
# Here, `inputs.nixpkgs` of home-manager is kept consistent with
# the `inputs.nixpkgs` of the current flake,
# to avoid problems caused by different versions of nixpkgs.
inputs.nixpkgs.follows = "nixpkgs";
};
};
outputs = inputs @ {
self,
nixpkgs,
stylix,
home-manager,
...
}: let
system = "x86_64-linux";
in {
# basic is a hostname for our machine
nixosConfigurations.basic = nixpkgs.lib.nixosSystem {
inherit system;
specialArgs = {
inherit inputs;
};
modules = [
stylix.nixosModules.stylix
./configuration.nix
# make home-manager as a module of nixos
# so that home-manager configuration will be deployed automatically when executing `nixos-rebuild switch`
home-manager.nixosModules.home-manager
{
home-manager.extraSpecialArgs = {
inherit inputs;
};
home-manager.useGlobalPkgs = true;
home-manager.useUserPackages = true;
home-manager.backupFileExtension = "hm-backup";
home-manager.users.ontake.imports = [
./home.nix
];
}
];
};
};
# nix run .#nixosConfigurations.basic.config.system.build.vm
}configuration.nix: {
config,
lib,
pkgs,
...
}: {
imports = [
./styling.nix
];
# customize kernel version
boot.kernelPackages = pkgs.linuxPackages_zen;
users.groups.admin = {};
users.users = {
ontake = {
isNormalUser = true;
extraGroups = ["wheel"];
password = "";
group = "admin";
};
};
networking.hostName = "nix-vm"; # Define your hostname.
virtualisation.vmVariant = {
# following configuration is added only when building VM with build-vm
virtualisation = {
memorySize = 8192; # Use 2048MiB memory.
cores = 8;
graphics = true;
};
};
services.openssh = {
enable = true;
settings.PasswordAuthentication = true;
};
networking.firewall.allowedTCPPorts = [22];
environment.systemPackages = with pkgs; [
htop
firefox
nano
wine
];
services.xserver.enable = true;
services = {
displayManager.sddm = {
enable = true;
wayland.enable = true;
};
displayManager.gdm.enable = false;
desktopManager.gnome.enable = true;
desktopManager.plasma6.enable = true;
};
# Choose seahorse over ksshaskpass
programs.ssh.askPassword = "${pkgs.seahorse}/libexec/seahorse/ssh-askpass";
time.timeZone = "Europe/Paris";
system.stateVersion = "24.11";
}home.nix {
config,
lib,
pkgs,
...
}: {
home.stateVersion = "25.05";
# Let home Manager install and manage itself.
programs.home-manager.enable = true;
}styling.nix {
config,
lib,
pkgs,
...
}: {
stylix = {
image = ./assets/wallhaven_7pyqqy_4000x2666.png;
enable = true;
};
}Yet I get the same issues. Black screen on login, systemsettings complaining about kvantum not being installed. Did miss anything? |
|
Only one thing about that report is somehow weird, and I would like you to elaborate more on what you mean with "systemsettings complaining about kvantum not being installed". That really should not be happening. It should be installed and on plasma6 that seems to be bad for different reasons, but it should still be there. |
|
I'm getting the same |
|
Also if you show me what the attributes changed by stylix are supposed to look like I can have a look myself |
|
@make-42 so basically, due to the fact that stylix is enabled on |
|
If I enable stylix in hm as well I get some errors, let me copy them over |
|
nevermind, it compiles, but shows the same errors |
|
Manually installing |
|
with the same error |
As I said that's because it already is installed. All of this unfortunately confirms my suspicion that plasma6 conflicts with qtct which is why I will have to disable qt styling when multiple DEs are enabled. |
|
It works fine on GNOME, I'll have to test on Plasma 5 |
|
Thanks a lot |
|
|
I took your heart emoji as agreement. The home-manager module is no longer autoEnabled as it requires the user to make conscious styling decision. Even though I agree, I will not take it upon myself to disable the kde module. As for reenabling qt on hm automatically because it might not cause issues under the absence of KDE deserves its own PR in my opinion or nothing will ever get merged.
I have followed exactly the approach from your last comment. I just forgot to resolve it.
One may wish to review under a new context again because they have changed their opinion since then. |
|
Someone broke the CI I guess. |
trueNAHO
left a comment
trueNAHO
left a comment
There was a problem hiding this comment.
The home-manager module is no longer autoEnabled as it requires the user to make conscious styling decision. Even though I agree, I will not take it upon myself to disable the kde module. As for reenabling qt on hm automatically because it might not cause issues under the absence of KDE deserves its own PR in my opinion or nothing will ever get merged.
As discussed in #1310 (comment), I would prefer disabling KDE by default instead of Qt. However, I would be fine merging this in the meantime to keep the ball rolling, although this would require end-users to first manually enable Qt support and then ideally remove it again, while KDE users are additionally required to manually enable KDE support. Directly disabling KDE support by default avoids the first two Qt roundtrips.
modules/qt/hm.nix
Outdated
| "stylix: qt: Changing `config.qt.style` is unsupported and may result in breakage! Use with caution!" | ||
| ) | ||
| ++ (lib.optional (config.stylix.targets.qt.platform == null && nixosConfig != null) | ||
| "stylix: qt: When using standalone home-manager, `config.stylix.targets.qt.platform` must be set." |
There was a problem hiding this comment.
What about the following rephrasing:
| "stylix: qt: When using standalone home-manager, `config.stylix.targets.qt.platform` must be set." | |
| "stylix: qt: `config.stylix.targets.qt.platform` is disabled by default to prevent Plasma 6 incompatibilities. Non-KDE users can set it to 'qtct', while KDE users can disable this warning with `<TODO: Add option to disable warning.>`." |
Could add you the missing option to disable the warning for KDE users?
Mikilio
force-pushed
the
qt-hm-fix
branch
2 times, most recently
from
00eae23 to
f6beb3b
Compare
stylix-automation
bot
added
topic: documentation
|
Since ultimately I'll probably be making both PR's anyway because I need KDE apps to be Qt styled. I don't mind increasing the scope of this PR. But then it's going to be a big PR and I don't want to have to deal with objections about "unrelated" fixes. I'd prefer, to just throw all changes in here and merge them together as long as they are Qt related. |
For brevity, my previous review intentionally did not mention that Qt should probably be enabled in all non-KDE testbeds. This should probably be done by enabling Qt in the general testbed configuration and specifically disabling it in the KDE testbeds. Disabling KDE by default instead of Qt does not cause such complexity because KDE is generally disabled and only KDE testbeds enable KDE. I will try the KDE approach myself tomorrow and see how that goes. |
The original #1092 issue can be reproduced on master (a1451bc ("qt: add missing configuration detail and update template (#2023)")) by declaring stylix.targets.qt.platform = "qtct"`;and running KDE with either of the following testbeds:
My following attempt results in a nice warning but does not prevent stylix.targets.qt.platform = "qtct"`;from being used on KDE, as it may be installed external to NixOS and Home Manager: diff --git a/modules/kde/hm.nix b/modules/kde/hm.nix
index 98e70a8e..a97856ae 100644
--- a/modules/kde/hm.nix
+++ b/modules/kde/hm.nix
@@ -2,6 +2,7 @@
pkgs,
config,
lib,
+ nixosConfig ? null,
...
}:
let
@@ -9,7 +10,7 @@ let
inherit (config.lib.stylix)
colors
- mkEnableTarget
+ mkEnableTargetWith
mkEnableWallpaper
;
inherit (config.stylix)
@@ -349,7 +350,19 @@ let
in
{
options.stylix.targets.kde = {
- enable = mkEnableTarget "KDE" true;
+ enable = mkEnableTargetWith {
+ name = "KDE";
+ autoEnable =
+ nixosConfig == null
+ || !nixosConfig.services.desktopManager.plasma6.enable
+ || config.stylix.targets.qt.platform != "qtct";
+ autoEnableExpr = ''
+ nixosConfig == null
+ || !nixosConfig.services.desktopManager.plasma6.enable
+ || config.stylix.targets.qt.platform != "qtct";
+ '';
+ };
+
useWallpaper = mkEnableWallpaper "KDE" true;
decorations = lib.mkOption {
type = lib.types.str;
@@ -368,9 +381,17 @@ in
};
config =
- lib.mkIf
- (config.stylix.enable && cfg.enable && pkgs.stdenv.hostPlatform.isLinux)
+ let
+ enable = config.stylix.enable && cfg.enable;
+ in
+ lib.mkMerge [
{
+ warnings =
+ lib.optional (!enable)
+ "`config.stylix.targets.kde.enable` is disabled on NixOS when using Plasma 6 and setting `config.stylix.targets.qt.platform` to `qtct` to prevent KDE from being borked: https://github.com/nix-community/stylix/issues/1092. Consider using a different `config.stylix.targets.qt.platform` value.";
+ }
+
+ (lib.mkIf enable {
home = {
packages = [ themePackage ];
@@ -394,5 +415,6 @@ in
X-KDE-AutostartScript=true
'';
};
- };
+ })
+ ];
}In other words, disabling our KDE module does not generally avoid the issue. It seems disabling Qt by default is indeed the most reliable solution, which is truly sad for non-KDE users. Considering how essential Qt support is, would it be better to disable Qt by default and always trigger a warning about Qt being disabled? The warning should mention that it is a precaution against using Qt with KDE, and that non-KDE user can safely disable this warning. Otherwise, should we change the Lines 69 to 71 in a1451bc Also, why exactly does KDE freeze when one of the Another possiblity would be to leave the KDE and Qt modules technically broken until upstream resolves the issue, and merely warn in the main ( |
I have no strong opinions on this. I just think that users should be made aware that they could have Qt styling using stylix if they wanted to, and just need to provide a bit of information about their DE. How we do this, I don't care.
KDE will not solve this as they don't support qtct and that's it. A workaround could look like adding something like this to the end of home-manager activation. if [[ "$CURRENT_DESKTOP" = "KDE" ]]; then
unset QT_QPA_PLATFORMTHEME
fiand/or use the patches from this AUR package at https://www.opencode.net/trialuser/qt6ct#tag=0.11 But I haven't tested that. |
If that works, this would be a great solution.
How fragile would those patches or overrides be? It might be safer to disable Qt on KDE with something like your previous proposal:
|
|
I will look into both solutions, but either way I'm not confident to maintain this, without having a testbed able to detect the breakage of this and me somehow getting notified on GitHub when it breaks. Additionally: Qt still doesn't have a maintainer and even though I have helped to solve issues for this module on multiple occasions, I have never registered myself as maintainer on purpose. That is because I really only ever contributed when it involved my own setup and when I have spare time, though I do make an effort that my changes are actually helpful, and I sometimes also help when it did not involve my setup (like here). That being said, if stylix had an additional "active" maintainer that would keep an eye on these things (especially for KDE or other setups that I don't use) that would be the best for this module overall. |
Thinking about this more, the following first approach might somewhat improve the situation but not generally solve the issue:
Attempting to impurely disable Qt this way would only work when running this activation script inside KDE. This does not trigger when KDE users bootstrap Stylix outside KDE, and this is a false positive when bootstrapping Stylix inside KDE only to switch to another graphical environment afterwards. Checking whether KDE is installed on the system is obviously unreliable. Without causing a sad breaking change of disabling Qt by default, attempting to disable Qt seems unacceptable and impossible. Attempting to disable the KDE module does not work either because it may be installed externally:
Since attempting to disable Qt or KDE is unacceptable or impossible, it might just be impossible to have a good workaround for this on our end.
Unless using externally maintained patches works, we should just consider this as unsolvable or out-of-scope, and simply warn about the KDE and Qt incompatibility in the documentation:
Since the issue is essentially a graphical freezing issue, I doubt we can and should reliably implement a catching mechanism beyond the previously provided testbeds to manually verify freezing:
I would prefer not adding those testbeds that test whether something is still broken because the current testbeds verify whether everything is working correctly. Adding this new type of testbeds is IMHO not worth it.
Totally understandable and I am very thankful for all your work so far, even though it is not specifically for your setup.
Agreed. Sadly, despite Qt and KDE being one of our most important and fragile GUI modules, they lack explicit module maintainers, which is unlikely to change anytime soon. The fact that these modules require extensive knowledge about the Linux desktop environment does not make it any easier to attract new maintainers. For example, I am familiar enough with Qt to drive its PRs, but my KDE unfamiliarity is the main reason #1943 and #2018 are pending. |
|
I generally agree with what you said. But with regard to the action being taken: Should Qt now be disabled on home-manager or be the one exception to the rule. In that latter case, it would by default trigger a warning that can be explicitly disabled. |
If the externally maintained patches work, we can do that. Otherwise, I was thinking about not touching the code and only mentioning the problem in the documentation:
Specifically, neither Qt nor KDE should be disabled by default, and no |
This fix prevents home-manager from initialising with defaults that potentially breakes plasma6 when the systemd module is not enabled. Relates to nix-community#1092
Fixes an issue occuring when mutliple DEs are used and qtct is chose. If one of these DEs is plasma6 the caused conflicts cause the DE to break.
Co-authored-by: 0xda157 <[email protected]>
Miscellaneous formatting and doc changes Co-authored-by: NAHO <[email protected]>
|
I rebased! @trueNAHO Where in the docs should I add that? It's kinda important. It usually takes some time of using stylix before someone actually checks the targets. And if stylix breaks their config only after importing it, I can imagine it being quite frustrating. Ideally, someone would be aware of this already on the getting started part. The decision to still autoEnable these modules is also based on the fact that they are deemed important, after all. Note for anyone looking for a workaround: Just disable the qt target. You probably don't need it. |
Unless using externally maintained patches works, I thought we agreed this is unsolvable or out-of-scope on the Stylix-level: #1310 (comment). Did you try whether your proposed externally maintained patches work? Otherwise, the Stylix functionality should not be changed.
What about adding this warning as the first subsection in the "Tips and tricks" ( |
6 participants
This fix prevents home-manager from initialising with defaults that potentially breakes plasma6 when the systemd module is not enabled.
Relates to #1092
Things done
Notify maintainers