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

dwm: cleanup #112299

Merged
merged 2 commits into from
Feb 9, 2021
Merged

dwm: cleanup #112299

merged 2 commits into from
Feb 9, 2021

Conversation

06kellyjac
Copy link
Member

@06kellyjac 06kellyjac commented Feb 7, 2021 •
edited
Loading

Motivation for this change

Dropped dwm-git since it hasn't been updated for years even thought it's meant to be trailing master
If people want dwm-git (or dmenu-git & st-git) I've made a neovim-nightly style overlay https://github.com/06kellyjac/suckless-git-overlay
LMK if the throw message needs changing

Cleaned up dwm

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS (x86_64)
    • 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.

@06kellyjac
Copy link
Member Author

Result of nixpkgs-review pr 112299 run on x86_64-linux 1

1 package built:
  • dwm

@ofborg ofborg bot requested a review from viric February 7, 2021 12:34
@AndersonTorres
Copy link
Member

If the git version is not being updated in 2 years, then it looks more logical to me it is the true stable one.

@06kellyjac
Copy link
Member Author

I haven't seen any stability concerns over 6.2 and it's not exactly a new release coming out 2019-02-02, and there are a lot of projects based off dwm at some point or another.
If it is a stable version it should be called dwm-stable-DATE IMO

@xeji would probably remember but they've not been active on GH for a while

@Mic92
Copy link
Member

Mic92 commented Feb 7, 2021

I haven't seen any stability concerns over 6.2 and it's not exactly a new release coming out 2019-02-02, and there are a lot of projects based off dwm at some point or another.
If it is a stable version it should be called dwm-stable-DATE IMO

@xeji would probably remember but they've not been active on GH for a while

I think this was meant as a joke.

@AndersonTorres
Copy link
Member

@06kellyjac ping

@06kellyjac
Copy link
Member Author

06kellyjac commented Feb 8, 2021 •
edited
Loading

Well is the plan to still delete dwm-git and go with just the overlay?
Or is @ivankovnatsky happy to take over from xeji maintaining it in nixpkgs?

Also if we're going to keep it in nixpkgs, it should really be an overlay of the original dwm. Especially now that they're identical excluding the modified name, different src, extra lines on the description, and the different maintainers

@ivankovnatsky
Copy link
Contributor

Hi Jack, I switched to i3, not interested in maintaining dwm* packages as of now. Thanks.

@06kellyjac
Copy link
Member Author

No worries 👍

06kellyjac
06kellyjac commented Feb 8, 2021
View reviewed changes
pkgs/top-level/all-packages.nix Outdated
@@ -21670,11 +21670,6 @@ in
patches = config.dwm.patches or [];
Copy link
Member Author

@06kellyjac 06kellyjac Feb 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is this actually missing conf = config.dwm.conf or null; that dwm-git has?
How does conf ? null get populated otherwise...

Should that be added in this PR too or another PR?

@AndersonTorres @viric

Copy link
Member

Choose a reason for hiding this comment

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

How many packages still use the config interface? I think it is 'reserved' for NixOS or external programs like Home Manager.

User-provided patches are usually a parameter with null as default.

Copy link
Member Author

Choose a reason for hiding this comment

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

🤷 I have no idea

I just saw conf was on the dwm-git copy but not on dwm but they both include it on the first line.

Copy link
Member

Choose a reason for hiding this comment

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

Get rid of that config reference.

Copy link
Member Author

Choose a reason for hiding this comment

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

The patches = config.dwm.patches part?
Do i also need to change the nixos module?

Copy link
Member

Choose a reason for hiding this comment

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

I have read the module, and it has no reference about configuring patches. Then it needs no modification at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I've removed it. LMK if that's right

Copy link
Member

@Mic92 Mic92 Feb 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It might break people still that used this option before through nixpkgs.config

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess if someone runs into the issue we add it back in?
Or do we direct them to do it a different way such as with overrides?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why the config reference was removed, I use nixpkgs.config.dwm.patches to set my patches and this needlessly broke my config on 21.05 upgrade. I'm making an issue to add this feature back.

@ofborg ofborg bot requested a review from viric February 8, 2021 16:27
@06kellyjac
dwm: cleanup
0e1bf2f 
- nixpkgs-fmt
- use pname and version
- use multiline for prePatch to make tree-wide modifications easier
- remove global `with lib;`
- update homepage to dwm page
- copy description from git repo
- add longDescription
- remove unused config.dwm.patches
@Mic92 Mic92 merged commit bc89cb0 into NixOS:master Feb 9, 2021
@06kellyjac 06kellyjac deleted the dwm branch February 9, 2021 16:45
@neonfuz neonfuz mentioned this pull request May 28, 2021
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndersonTorres AndersonTorres AndersonTorres left review comments

@neonfuz neonfuz neonfuz left review comments

@Mic92 Mic92 Mic92 left review comments

@viric viric Awaiting requested review from viric

Assignees
No one assigned
Labels
8.has: clean-up 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 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.
6 participants
@06kellyjac @AndersonTorres @Mic92 @ivankovnatsky @viric @neonfuz