Factorio update #22032
Factorio update #22032
Conversation
|
@bflyblue, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Baughn, @elitak and @vikstrous to be potential reviewers. |
|
Just tested this out. The game starts normally. I haven't tried any of the server stuff though. |
|
I don't think I'm a good reviewer for this anymore. There's a whole lot of code in here which I didn't write, and never used... The hash update looks fine, but I can't comment on the server code either. It's possible that @bflyblue is the only person using it, in which case... it should probably not be in nixpkgs? I'm not sure, there isn't much of a downside. |
| @@ -120,6 +201,7 @@ in | |||
| "--config=${cfg.configFile}" | |||
| "--port=${toString cfg.port}" | |||
| "--start-server=${mkSavePath cfg.saveName}" | |||
| "--server-settings=${serverSettingsFile}" | |||
| (optionalString (cfg.mods != []) "--mod-directory=${modDir}") | |||
| (optionalString (cfg.autosave-interval != null) "--autosave-interval ${toString cfg.autosave-interval}") | |||
There was a problem hiding this comment.
autosave-interval shouldn't need to go on cmdline if it's in the config file. Does the config file support changing the mod directory as well? If so, that can move there as well.
There was a problem hiding this comment.
I didn't see an option in the sample file bundled with Factorio to specify the mods so I only exposed a few options I thought may be useful.
There was a problem hiding this comment.
Oh! I meant to take it off the command line.. will do that now. Thanks for the catch.
There was a problem hiding this comment.
Fixed in commit d78f684
| Game will be broadcast on LAN. | ||
| ''; | ||
| }; | ||
| username = mkOption { |
There was a problem hiding this comment.
username and password are also given and used by nixpkgs.config.factorio.username/password. Should they be kept independent or merged into one or the other?
There was a problem hiding this comment.
That's a good question, I wasn't sure which is preferable. Does anyone know if it would be desirable for a server to authenticate using different credentials to a player?
There was a problem hiding this comment.
I suppose it's fine to leave them independent for now, since the nixpkgs one is used only to fetch the tarball and may eventually disappear. It does have the advantage of not being world-readable, however.
Anyway, we can sort it out later; the PR looks okay to me for a merge, as it is now.
Motivation for this change
A new version of Factorio was released on 24 Nov 2016.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)