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

slstatus: init at unstable-2018-03-28 #37706

Merged
merged 1 commit into from Apr 1, 2018

Conversation

oxzi
Copy link
Member

@oxzi oxzi commented Mar 23, 2018

Motivation for this change

This PR adds the slstatus software to display status information for display managers like dwm.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

nativeBuildInputs = [ pkgconfig ];
buildInputs = [ libX11 ];

installPhase = "make install PREFIX=$out";
Copy link
Member

Choose a reason for hiding this comment

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

You can remove this line and instead use installFlags = [ "PREFIX=$(out)" ]

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks and please excuse my late reply.

};

configFile = optionalString (conf!=null) (writeText "config.def.h" conf);
preBuild = optionalString (conf!=null) "cp ${configFile} config.def.h";
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to not do the String -> Path conversions here, because it could be the case that the user wants to provide their own file, so they'd have to pass builtins.readFile ./my-config.def.h, which would only get converted right back to a file here.

So I suggest making conf a path parameter, removing the configFile attribute and just using cp ${conf} config.def.h. Also an assertion that checks for conf actually being a path (starts with "/") would be nice, along with a simple message.

And probably configFile would be a better name then, there's a couple uses of a configFile argument throughout nixpkgs already.

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 to admit that I'm not a big fan from this solution, which I adapted from the nixpkgs/pkgs/applications/misc/st/default.nix-file. Isn't it better if there is at least some regularity? That's the reason why I choose this way.

Copy link
Member

Choose a reason for hiding this comment

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

I see, didn't know that. While I don't like this approach because it possibly does redundant work, I think it it makes sense to have some regularity, so yeah that's okay

@@ -4802,6 +4802,10 @@ with pkgs;

slsnif = callPackage ../tools/misc/slsnif { };

slstatus = callPackage ../applications/misc/slstatus {
conf = config.slstatus.conf or null;
Copy link
Member

Choose a reason for hiding this comment

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

I like this idea, and I see config being used in other places throughout nixpkgs, but I'm not sure whether I understand the purpose, because it's just as easy to use slstatus.override { configFile = ./foobar.def.h; } instead

Copy link
Member Author

Choose a reason for hiding this comment

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

This allows you to set this specific config-entry in, for example, your ~/.nixpkgs/config.nix file.

{
  slstatus.conf = ''
    #define MAXLEN 2048

    static const int interval = 15000;
    static const char unknown_str[] = "NA";

    static const struct arg args[] = { … };
  '';
}

@oxzi oxzi force-pushed the slstatus-unstable-2018-03-21 branch from 05c145c to 80872c1 Compare March 29, 2018 09:14
@oxzi oxzi force-pushed the slstatus-unstable-2018-03-21 branch from 80872c1 to f51e669 Compare March 29, 2018 09:29
@oxzi oxzi changed the title slstatus: init at unstable-2018-03-21 slstatus: init at unstable-2018-03-28 Mar 29, 2018
@matthewbauer matthewbauer merged commit 2e705b0 into NixOS:master Apr 1, 2018
@oxzi oxzi deleted the slstatus-unstable-2018-03-21 branch April 2, 2018 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants