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.

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

keeperrl: init at alpha28 #67076

Merged
merged 1 commit into from Mar 23, 2020
Merged

keeperrl: init at alpha28 #67076

merged 1 commit into from Mar 23, 2020

Conversation

ghost
Copy link

@ghost ghost commented Aug 20, 2019

Motivation for this change

New package.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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 nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

pkgs/games/keeperrl/default.nix Outdated Show resolved Hide resolved
pkgs/games/keeperrl/default.nix Show resolved Hide resolved
Copy link
Contributor

@alexarice alexarice left a comment

Choose a reason for hiding this comment

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

Added some comments which are mainly style changes

pkgs/games/keeperrl/default.nix Show resolved Hide resolved
pkgs/games/keeperrl/default.nix Outdated Show resolved Hide resolved
pkgs/games/keeperrl/default.nix Outdated Show resolved Hide resolved

free-src = fetchFromGitHub {
owner = "miki151";
repo = "keeperrl";
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use repo = pname;

Copy link
Contributor

Choose a reason for hiding this comment

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

personal taste, some like it as an explicit string. I don't really feel strongly either way. (I know im not author, just my 2c :) )

Copy link
Author

Choose a reason for hiding this comment

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

I'd vote to go with the literal string, and so allow that the github repo name isn't the canonical package name. But I'm happy to change this.

Copy link
Contributor

Choose a reason for hiding this comment

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

there's plenty of expression that do pname, and plenty that don't. I would say it's maintainer's choice

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, this is fine with me then

pkgs/games/keeperrl/default.nix Show resolved Hide resolved
pkgs/games/keeperrl/default.nix Outdated Show resolved Hide resolved
pkgs/games/keeperrl/default.nix Outdated Show resolved Hide resolved
pkgs/games/keeperrl/default.nix Outdated Show resolved Hide resolved
pkgs/games/keeperrl/default.nix Outdated Show resolved Hide resolved
@jonringer
Copy link
Contributor

@Chattered you can "resolve" the conversations after you apply the fix, cleans up the conversation tab quite a bit :)

@ghost
Copy link
Author

ghost commented Aug 21, 2019

Ah missed that! Nice. :)

Copy link
Contributor

@alexarice alexarice left a comment

Choose a reason for hiding this comment

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

Builds with nix-review on Nixos and resulting binary appears to work in that it lets me start a game.

When I run nix-build -E 'with (import ./. {}); keeperrl.override{ unfree_assets=true; }' I get the expected message that the assets are missing. I cannot say whether they are put in the correct place as I do not have them to check.

The play-keeper wrapper does not run at all with error

FATAL sokoban_input.cpp:7 : input is false. Failed to load sokoban data from ./data_free/sokoban_input.txt

@alexarice
Copy link
Contributor

Same error, what is this meant to do?

@ghost
Copy link
Author

ghost commented Aug 21, 2019

play-keeper should no longer be under "bin" or on $PATH. Only the wrapper should be run.

@alexarice
Copy link
Contributor

so it's ok that it doesn't run?

@alexarice
Copy link
Contributor

Ah i got confused about which one was the wrapper

@ghost
Copy link
Author

ghost commented Aug 21, 2019

Ack. Sorry. Just noticed I don't need the wrapper if I provide the location of the data directories to make.

@alexarice
Copy link
Contributor

This works for me. The only other thing is I get a warning on startup

libpng warning: iCCP: known incorrect sRGB profile

I don't know if this is cause for concern

@ghost
Copy link
Author

ghost commented Aug 21, 2019

I've put in a fair number of hours of play, and haven't found any graphical issues, so I'm hoping I can ignore it.

@jonringer
Copy link
Contributor

jonringer commented Aug 21, 2019

@alexarice it's an issue with the latest versions of libpng being more strict. https://stackoverflow.com/questions/22745076/libpng-warning-iccp-known-incorrect-srgb-profile

Shouldn't cause any crashes, just a warning.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/weekly-leaderboards/2637/22

pkgs/games/keeperrl/default.nix Outdated Show resolved Hide resolved
@doronbehar
Copy link
Contributor

doronbehar commented Mar 23, 2020 via email

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

LGTM

game launches fine

[3 built, 0.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/67076
1 package built:
keeperrl

@jonringer jonringer merged commit 664c6e2 into NixOS:master Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants