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
keeperrl: init at alpha28 #67076
Conversation
There was a problem hiding this 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
|
||
free-src = fetchFromGitHub { | ||
owner = "miki151"; | ||
repo = "keeperrl"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use repo = pname;
There was a problem hiding this comment.
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 :) )
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
@Chattered you can "resolve" the conversations after you apply the fix, cleans up the conversation tab quite a bit :) |
Ah missed that! Nice. :) |
There was a problem hiding this 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
Same error, what is this meant to do? |
play-keeper should no longer be under "bin" or on $PATH. Only the wrapper should be run. |
so it's ok that it doesn't run? |
Ah i got confused about which one was the wrapper |
Ack. Sorry. Just noticed I don't need the wrapper if I provide the location of the data directories to make. |
This works for me. The only other thing is I get a warning on startup
I don't know if this is cause for concern |
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. |
@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. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
There was a problem hiding this 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
Motivation for this change
New package.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)