pixiecore: initial commit #62113
pixiecore: initial commit #62113
Conversation
|
can we have seperate commits for the service and the package? Also the merge commit is kinda unnecessary |
|
Thank you for the feedback. Will resolve all those today and resubmit! Should it be new clean pullrequest, right? |
|
You can just amend your commits and force-push to the same branch again |
|
Please do a rebase and then you can avoid the merge in your git history as well. Let us know if you need any help. |
Initial commit of pixiecore package + module. Currently supports only boot mode.
| description = "Log more things that aren't directly related to booting a recognized client"; | ||
| }; | ||
|
|
||
| logTimestamps = mkOption { |
There was a problem hiding this comment.
If this is logging to syslog then won't time stamps always be added anyways? If this is true will there be 2 timestamps for every entry?
|
|
||
| bootMsg = mkOption { | ||
| type = types.nullOr types.string; | ||
| default = ""; |
There was a problem hiding this comment.
Looks like this option is never actually used, so I'd vote for just deleting the mkOption block.
| }; | ||
|
|
||
| port = mkOption { | ||
| type = types.int; |
| }; | ||
|
|
||
| statusPort = mkOption { | ||
| type = types.int; |
| @@ -0,0 +1,26 @@ | |||
| { stdenv, buildGoPackage, fetchgit, fetchhg, fetchbzr, fetchsvn }: | |||
|
|
|||
There was a problem hiding this comment.
As mentioned name is no longer required.
|
|
||
| cmdLine = mkOption { | ||
| type = types.nullOr types.string; | ||
| default = ""; |
There was a problem hiding this comment.
Again, maybe null is more appropriate default.
There was a problem hiding this comment.
Having it be types.string (without nullOr and default = "" would make more sense to me, unless no command line should be different from an empty command line.
(Also note that as currently implemented the --cmdline argument is passed iff cmdLine is not "", so it'll be passed if it's null and this will probably break)
| PIDFile="/run/pixiecore.pid"; | ||
| DynamicUser="yes"; | ||
| AmbientCapabilities = "cap_net_bind_service"; | ||
| ExecStart = "${pkgs.pixiecore}/bin/pixiecore boot ${cfg.kernel} ${cfg.initrd} ${optionalString (cfg.cmdLine != "") "--cmdline=\\\'${cfg.cmdLine}\\\'"} ${optionalString cfg.debug "--debug"} ${optionalString cfg.logTimestamps "--log-timestamps"} ${optionalString cfg.dhcpNoBind "--dhcp-no-bind"} ${optionalString (cfg.listen != "") "--listen-addr ${cfg.listen}"} ${optionalString (cfg.port != 0) "--port ${toString cfg.port}"} ${optionalString (cfg.statusPort!= 0) "--status-port ${toString cfg.statusPort}"}"; |
There was a problem hiding this comment.
| ExecStart = "${pkgs.pixiecore}/bin/pixiecore boot ${cfg.kernel} ${cfg.initrd} ${optionalString (cfg.cmdLine != "") "--cmdline=\\\'${cfg.cmdLine}\\\'"} ${optionalString cfg.debug "--debug"} ${optionalString cfg.logTimestamps "--log-timestamps"} ${optionalString cfg.dhcpNoBind "--dhcp-no-bind"} ${optionalString (cfg.listen != "") "--listen-addr ${cfg.listen}"} ${optionalString (cfg.port != 0) "--port ${toString cfg.port}"} ${optionalString (cfg.statusPort!= 0) "--status-port ${toString cfg.statusPort}"}"; | |
| ExecStart = "${pkgs.pixiecore}/bin/pixiecore boot ${cfg.kernel} ${cfg.initrd} --cmdline='${cfg.cmdLine}' ${optionalString cfg.debug "--debug"} ${optionalString cfg.logTimestamps "--log-timestamps"} ${optionalString cfg.dhcpNoBind "--dhcp-no-bind"} --listen-addr '${cfg.listen}' --port ${toString cfg.port} --status-port ${toString cfg.statusPort}"; |
Unless --cmdline "" is different from no --cmdline argument. (Also note that as currently written I think you have one too many escapes here, which would not get triggered when staying at the default --cmdline though)
Similarly, cfg.listen already defaults to 0.0.0.0, so it can just always be passed. And same for cfg.port/statusPort, as they're already set to the pixiecore default, they can just be always passed. (not sure whether there's necessarily a status port, though? in which case it should be of type nullOr port and not be passed when null -- not when 0).
Sure it's hardcoding the defaults of pixiecore into the module, but it sounds better to me to just have the whole thing simpler.
Oh, and it's a really long line even with the suggested changes, so you may want to break it over multiple lines -- for instance by closing the NixOS string and opening it again.
| @@ -24181,4 +24181,6 @@ in | |||
|
|
|||
| dapper = callPackage ../development/tools/dapper { }; | |||
|
|
|||
| pixiecore = callPackage ../tools/networking/pixiecore {}; | |||
There was a problem hiding this comment.
This is not the right place, it should go under the ### TOOLS heading (a good trick is to grep for tools/networking and place it close to a match)
There was a problem hiding this comment.
@RaunoVV Courage! You're almost there, we know it's a lot of feedback to handle… but sorry, you started directly with a package+module, so you get all the feedback you'd normally get over the course of several PRs spread over weeks or months :p So courage! The PR should soon be ready :)
|
@RaunoVV do you have any motivation to continue with this PR? |
|
For the author, reviewers, and committers: this PR was scanned and appears to add a use of the deprecated
|
|
I'm looking forward for this module. Any progress on it? |
|
added in #83373 |
Motivation for this change
Initial commit for pixiecore tool: https://github.com/danderson/netboot/tree/master/pixiecore
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)