endless sky: init at 0.9.4 #21010
endless sky: init at 0.9.4 #21010
Conversation
| @@ -1550,6 +1550,7 @@ in | |||
|
|
|||
| emscriptenStdenv = stdenv // { mkDerivation = buildEmscriptenPackage; }; | |||
|
|
|||
| endlessSky = callPackage ../games/endless-sky { }; | |||
There was a problem hiding this comment.
Should be endless-sky, also please move the field to the ### GAMES part of the file. I believe it should end up between these two lines:
egoboo = callPackage ../games/egoboo { };
eternity = callPackage ../games/eternity-engine { };
There was a problem hiding this comment.
Fixed, thanks for the feedback!
| homepage = "https://endless-sky.github.io/"; | ||
| license = with licenses; [ | ||
| gpl3Plus cc-by-sa-30 cc-by-sa-40 publicDomain | ||
| ]; |
There was a problem hiding this comment.
Do you want to maintain this package?
There was a problem hiding this comment.
Yeah, I don't see any reason why not. Should I add myself to lib/maintainers.nix and refer to that for it?
lheckemann
force-pushed
the
master
branch
2 times, most recently
from
f1154a8
to
d09de65
Compare
|
I guess I'll need to disable OSX as a platform? Or should it be included even though upstream provides it neatly prepackaged? |
|
If the package builds under OSX then I think it is good practice to include it. You'll need a Edit: Actually I'm uncertain how mesaPlatforms should be used. Only when mesa is an explicit dependency or when the package uses OpenGL? Maybe platforms.all would be fine here? |
|
@rycee it doesn't currently build on OSX (hence the travis failure). It's certainly possible, but I'm just wondering if it's worth the extra effort given that upstream provides binaries. I also don't know how to fix it myself. |
|
Sorry, I didn't actually pay attention to travis. But yeah if it doesn't build under OSX then put something like in the |
| license = with licenses; [ | ||
| gpl3Plus cc-by-sa-30 cc-by-sa-40 publicDomain | ||
| ]; | ||
| maintainers = with lib.maintainers; [ lheckemann ]; |
There was a problem hiding this comment.
This should be without lib.: with maintainers; [ lheckemann ]
The with stdenv.lib; { above already introduced the stuff inside lib (including maintainers).
|
In any case, this looks like a fine bit of packaging so if you fix the maintainers and platforms fields in the meta section I'd be happy to merge. |
|
Made all the changes, thanks for the feedback! EDIT: what about testing on non-nixos linux? |
|
For non-nixos I feel that travis is sufficient and it is always possible to improve the package in the future. I have sometimes done testing in virtual machines running other distributions but for most packages I don't worry so much. In any case, I have rebased this PR into master in commit 9aca55e. Many thanks for your contribution! |
|
Awesome, thanks! |
|
Nitpick: this way |
|
@vcunat, I guess it should have been |
|
Maybe I'll replace |
|
Should it not be possible to determine that the package won't build on a platform that doesn't have OpenGL anyway, because GLEW won't build? (or will it...) |
|
Unless you overrode settings like |
Motivation for this change
It's a fun game!
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"— not sure I fully understand this one, but seeing as it's a new application with no dependents it should be fine?./result/bin/)