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

Added support for Factorio's experimental branch. #33957

Merged
merged 1 commit into from Feb 16, 2018
Merged

Added support for Factorio's experimental branch. #33957

merged 1 commit into from Feb 16, 2018

Conversation

seanparsons
Copy link
Contributor

@seanparsons seanparsons commented Jan 16, 2018 •
edited

Motivation for this change

Factorio has a normal release version and an experimental release version, it would be nice if NixOS had support for both.

Things done

I've added a level of nesting to the original Factorio nix file so that it supports platform, release type and now also experimental/final. Additionally I added a factorio-experimental top level package so that it's more easily installed.

Ideally this would also make it into the 17.09 branch.

  • 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.

jtojnar
jtojnar reviewed Jan 16, 2018
View reviewed changes
pkgs/games/factorio/default.nix Outdated
@@ -14,19 +15,38 @@ let

# NB If you nix-prefetch-url any of these, be sure to add a --name arg,
# where the ultimate "_" (before the version) is changed to a "-".
experimentalOrFinal = if experimental then "experimental" else "final";
Copy link
Contributor

Choose a reason for hiding this comment

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

The variable could be named branch or something.

Also, the website uses the following terms:

Latest releases
Stable: 0.15.40
Experimental: 0.16.16

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the changes you suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtojnar Is there any remaining issues?

@jtojnar
Copy link
Contributor

jtojnar commented Jan 21, 2018

I do not see any problems but structural changes like these are better judged by the maintainers. cc @Baughn and @elitak

@elitak
Copy link
Contributor

elitak commented Jan 21, 2018

LGTM except the commit messages don't comply with CONTRIBUTING.md.

The number of variations and the very idea of "alpha-experimental" vs "alpha-stable" are getting a bit mad, but I suppose that's upstream's doing.

@seanparsons seanparsons force-pushed the add-factorio-experimental-support branch 2 times, most recently from ca28595 to 0872948 Compare January 24, 2018 22:33
@seanparsons
Copy link
Contributor Author

@elitak I squashed the commits and in the process believe I've sorted the commit message.

@seanparsons
Copy link
Contributor Author

@Baughn @elitak Is there any other reason why this should still be held up? I think the version I've updated it to in here is already out of date.

@elitak
Copy link
Contributor

elitak commented Feb 15, 2018

No. I just don't have merge privilege. Update the version again and push, and hopefully it will get noticed this time. I'll tag a maintainer if it doesn't.

@Baughn
Copy link
Contributor

Baughn commented Feb 15, 2018

No, this looks good to me, but I also don't have merge privilege.

@seanparsons
factorio: refactor derivation to add experimental branch and upgrade …
7229066 
…to 0.16.24.

Supports the experimental and stable branches of Factorio with a new configuration option "experimental".
@seanparsons seanparsons force-pushed the add-factorio-experimental-support branch from 0872948 to 7229066 Compare February 15, 2018 22:07
@seanparsons
Copy link
Contributor Author

Done.

@elitak
Copy link
Contributor

elitak commented Feb 16, 2018

@joachifm or @Mic92, could you merge this, please?

@joachifm joachifm merged commit 1628e40 into NixOS:master Feb 16, 2018
@seanparsons
Copy link
Contributor Author

@joachifm Is it possible for this to also make it into 17.09 as well please?

@joachifm
Copy link
Contributor

You can cherry-pick (with -x) and submit a PR against release-17.09, or wait and I'll do it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar left review comments

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@seanparsons @jtojnar @elitak @Baughn @joachifm @GrahamcOfBorg