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.

Sign up for GitHub

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

Jump to bottom

nixos/dwm: add option package #94185

Closed
wants to merge 1 commit into from
Closed

nixos/dwm: add option package #94185

wants to merge 1 commit into from

Conversation

syberant
Copy link
Member

Motivation for this change

Allows specifying a modified dwm package.

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

@syberant
nixos/dwm: add option package
180cedf 
Allows specifying a modified dwm package.
@syberant
Copy link
Member Author

syberant commented Aug 6, 2020

@ck3d, I'm not sure who to ask to take a look at this PR but would you be so friendly? I'm asking because you commited to this file before.

ck3d
ck3d reviewed Aug 7, 2020
View reviewed changes
Copy link
Contributor

@ck3d ck3d left a comment

Choose a reason for hiding this comment

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

Thanks for pushing me to this review.
I never thought that this is needed. A dwm package is always personalized since no configuration file can be passed to dwm during runtime. This setting allows a systems administrator to make its configuration to the systems default. As alternative the systems adminstrator could override the dwm in its overlay.
I am neutral to this change, since it do not change the user environment override behaviour, an users installed dwm with nix-env will win.

@syberant
Copy link
Member Author

syberant commented Aug 7, 2020

Sure, this is just a bit of a cleaner option (at least, I think so) so that an overlay is not needed anymore.

@malte-v
Copy link
Contributor

malte-v commented Oct 13, 2020

Isn't this is pretty uncontroversial change? Many expressions define a package option and for dwm, it is arguably more needed than for any other program. Using an overlay feels like a hack, I would even say it breaks convention at this point: https://search.nixos.org/options?query=windowManager%20.package&from=0&size=30&sort=relevance&channel=unstable.

@jgarte
Copy link
Contributor

jgarte commented May 3, 2021

@ck3d

Hi what's the status on this? I would also prefer @syberant's suggestion to the dwm service api instead of using an overlay.

@stale
Copy link

stale bot commented Nov 9, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 9, 2021
@ck3d ck3d mentioned this pull request Oct 16, 2022
Merged
13 tasks
bobvanderlinden
bobvanderlinden approved these changes Oct 16, 2022
View reviewed changes
Copy link
Member

@bobvanderlinden bobvanderlinden left a comment

Choose a reason for hiding this comment

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

I created a PR almost identical to this one. This PR looks good 👍

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Oct 16, 2022
@ck3d
Copy link
Contributor

ck3d commented Oct 16, 2022

I merged the newer version of the same change: #196301

@ck3d ck3d closed this Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ck3d ck3d ck3d left review comments

@bobvanderlinden bobvanderlinden bobvanderlinden approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@syberant @malte-v @jgarte @ck3d @bobvanderlinden