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

Remove extra packages in nix module #343

Merged
merged 3 commits into from Jul 29, 2022
Merged

Remove extra packages in nix module #343

merged 3 commits into from Jul 29, 2022

Conversation

reptee
Copy link
Contributor

@reptee reptee commented Jul 10, 2022

Hello!

Describe your PR, what does it fix/add?

That's a super short PR, removing any default extra packages provided via nixos module. While extra packages make sense for full DE, i believe that WM is usually DIY environment.

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

Nope

Is it ready for merging, or does it need work?

Ready

@viperML
Copy link
Contributor

viperML commented Jul 10, 2022

I added that option thinking it could be useful for people to have a working environment with the default config, that uses kitty.

If you want to remove it, I'd also remove the extraPackages option altogether

@reptee
Copy link
Contributor Author

reptee commented Jul 10, 2022

extraPackages explicitly says that bunch of packages are needed with another package(hyprland), and aren't really required without it

EDIT: when it comes to system.packages, it isn't really clear, why are those packages installed, are they a part of an environment, or simply a standalone applications. Say notification daemon would make more sense with hyprwm as it is a part of complete hyprwm setup, while say, firefox is just a standalone app

@viperML
Copy link
Contributor

viperML commented Jul 10, 2022

extraPackages explicitly says that bunch of packages are needed with another package(hyprland), and aren't really required without it

EDIT: when it comes to system.packages, it isn't really clear, why are those packages installed, are they a part of an environment, or simply a standalone applications. Say notification daemon would make more sense with hyprwm as it is a part of complete hyprwm setup, while say, firefox is just a standalone app

Sorry, but I don't understand the point your are making

@reptee
Copy link
Contributor Author

reptee commented Jul 10, 2022

Separating package and its dependencies -> bad

Grouping package and its dependencies -> good

Even if dependencies aren't really a part of runtime for hyprland.

# separated
{
  programs.hyprland.enable = true;
  environment.systemPackages = with pkgs; [ some packages ];
}

# grouped, easy to understand extra packages are a part of hyprland setup
{
  programs.hyprland = {
    enable = true;
    extraPackages = with pkgs; [ some packages ];
  };
}

Trying my best to explain, at some point I agree with

If you want to remove it, I'd also remove the extraPackages option altogether

though having such option is nice

@viperML
Copy link
Contributor

viperML commented Jul 10, 2022

You can always split your config to make sure that those packages go with the Hyprland part. Personally I see it unnecesary to provide an alias to environment.systemPackages that comes empty by default.

Maybe it is just me, keep the changes I guess 👍

@viperML
Copy link
Contributor

viperML commented Jul 11, 2022

@fufexan

@fufexan
Copy link
Member

fufexan commented Jul 11, 2022

I agree with what viper said, we should also remove extraPackages while we're at it. I haven't seen anyone use them, and if you're tinkering with compositors you already know how to install the proper packages you'll need inside them.

@reptee
Copy link
Contributor Author

reptee commented Jul 11, 2022

Whatever you prefer, gentlemen. I will be satisfied with removal of default packages in either way

@fufexan
Copy link
Member

fufexan commented Jul 29, 2022

@viperML any last words before merge? xD

@viperML
Copy link
Contributor

viperML commented Jul 29, 2022

iirc you could set an option as removed with the nixos module system, if you want to go the extra mile

(Right now I don't have access to my nix machines to test)

otherwise lgtm

@fufexan
Copy link
Member

fufexan commented Jul 29, 2022

Should be fine now?

@fufexan fufexan merged commit fd99910 into hyprwm:main Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants