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

steam: Added extraEmulators option to chroot env. #26190

Merged
merged 1 commit into from Jun 2, 2017
Merged

steam: Added extraEmulators option to chroot env. #26190

merged 1 commit into from Jun 2, 2017

Conversation

izuk
Copy link
Contributor

@izuk izuk commented May 28, 2017

Motivation for this change

I like to use steam to run all my games, including ones that work through dosbox. This lets me include dosbox in the chroot environment, by doing this in config.nix:

steam = pkgs.steam.override { extraEmulators = [ pkgs.dosbox ]; };

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

@mention-bot
Copy link

@izuk, thanks for your PR! By analyzing the history of the files in this pull request, we identified @abbradar, @hrdinka and @the-kenny to be potential reviewers.

@FRidh
Copy link
Member

FRidh commented May 30, 2017

why extraEmulators? You can add any package here, so it would make more sense to say extraPkgs. But then you have to be more specific and say (e.g. with a comment) where those extra packages are added to.

@izuk
Copy link
Contributor Author

izuk commented May 30, 2017

extraPkgs is fine. Can you point me to an example of the kind of comment you mean?

@izuk
Copy link
Contributor Author

izuk commented Jun 1, 2017

ping

@abbradar
Copy link
Member

abbradar commented Jun 2, 2017

@FRidh to be honest I also didn't fully get what you meant by this comment -- perhaps to mention somewhere about existence of this option?

@FRidh
Copy link
Member

FRidh commented Jun 2, 2017

An extraPkgs option would add extra packages...but where? Looking at buildFHSUserEnv there's targetPkgs and multiPkgs. In this case, extraPkgs will be added to both so it is less relevant.

@abbradar
Copy link
Member

abbradar commented Jun 2, 2017

@FRidh Ah, got it! I think we can either make two arguments (extraPkgs + extraMultiPkgs) or leave one and add them to targetPkgs (with a comment then).

@izuk
Copy link
Contributor Author

izuk commented Jun 2, 2017

PTAL; I've changed it to extraPkgs and added a comment.

abbradar
abbradar requested changes Jun 2, 2017
View reviewed changes
pkgs/games/steam/chrootenv.nix Outdated
@@ -2,6 +2,7 @@
, steam-runtime, steam-runtime-i686 ? null
, withJava ? false
, withPrimus ? false
, extraPkgs ? [ ] # extra packages to add to targetPkgs and multiPkgs
Copy link
Member

Choose a reason for hiding this comment

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

Please make this a function:

extraPkgs ? pkgs: []

Also, they actually would be added only to targetPkgs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (I think).

pkgs/games/steam/chrootenv.nix Outdated
@@ -37,7 +38,8 @@ let
# Zoneinfo
etc-zoneinfo
] ++ lib.optional withJava jdk
++ lib.optional withPrimus primus2;
++ lib.optional withPrimus primus2
++ extraPkgs;
Copy link
Member

Choose a reason for hiding this comment

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

And here pass pkgs to the function: extraPkgs pkgs.

This way you will use the same package set buildFHSEnv passes to the function above.

@abbradar abbradar merged commit ad7bee4 into NixOS:master Jun 2, 2017
@abbradar
Copy link
Member

abbradar commented Jun 2, 2017

Thanks and sorry for taking a long time!

@izuk
Copy link
Contributor Author

izuk commented Jun 2, 2017

No problem. Now I can get back to playing Tie Fighter.

@lukateras lukateras mentioned this pull request Dec 20, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abbradar abbradar abbradar requested changes

Assignees
No one assigned
Labels
6.topic: steam
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@izuk @mention-bot @FRidh @abbradar @lukateras