-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Conversation
nativeBuildInputs = [ pkgconfig ]; | ||
buildInputs = [ libX11 ]; | ||
|
||
installPhase = "make install PREFIX=$out"; |
There was a problem hiding this comment.
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)" ]
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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[] = { … };
'';
}
05c145c
to
80872c1
Compare
80872c1
to
f51e669
Compare
Motivation for this change
This PR adds the
slstatus
software to display status information for display managers likedwm
.Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)