-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
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
Xjump: init at 2.9.3 #34706
Xjump: init at 2.9.3 #34706
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.
Seems to work, but few minor issues as described.
pkgs/games/xjump/default.nix
Outdated
@@ -0,0 +1,27 @@ | |||
{ stdenv, fetchFromGitHub, gcc, autoconf, automake, xorg, ... }: |
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 don't know that it's problematic, but the "..." is certainly not idiomatic unless needed or extra arguments expected.
Please remove or explain :).
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.
Also, callPackage
automatically searches xorg
, so it might be better to have libX11
, libXt
, libXpm
and libXaw
as expression arguments.
pkgs/games/xjump/default.nix
Outdated
rev = "e7f20fb8c2c456bed70abb046c1a966462192b80"; | ||
sha256 = "0hq4739cvi5a47pxdc0wwkj2lmlqbf1xigq0v85qs5bq3ixmq2f7"; | ||
}; | ||
buildInputs = [ gcc autoconf automake xorg.libX11 xorg.libXt xorg.libXpm xorg.libXaw ]; |
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.
autoconf & automake should be nativeBuildInputs.
Consider replacing these (and the preConfigure attribute) with autoreconfHook (I haven't tested if this works).
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.
Oh--and why is "gcc" a buildInput? It almost never should be, especially in code that isn't itself a compiler or runtime of some sort.
pkgs/games/xjump/default.nix
Outdated
description = "The falling tower game"; | ||
maintainers = with maintainers; [ pmeunier ]; | ||
platforms = platforms.linux; | ||
}; |
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.
Is it known to not work on other platforms? Just curious, might be worth letting builders find out. Deps on basic Xlibs suggest it might.
More importantly, please add the license which appears to be GPL2: https://github.com/hugomg/xjump/blob/e7f20fb8c2c456bed70abb046c1a966462192b80/COPYING
pkgs/games/xjump/default.nix
Outdated
preConfigure = '' | ||
autoreconf --install | ||
''; | ||
installPhase = '' |
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.
Doesn't look like a custom installPhase is needed, please remove.
Using default installation behavior also installs a manpage, icon, and .desktop file, which are all nice to include.
It also installs a "var/xjump/highscores" file which may be problematic, possibly needing to be removed in a postInstall or something-- although probably should check to ensure either way that it doesn't attempt to read/write that location regardless.
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.
Up to you if you want to fix the highscores situation, I think ignoring it for now is fine.
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.
Thanks for the review! I just fixed my mistakes (at least I hope), including the highscores, which are now configurable. I don't know what is the best way to install a system-wide highscores file, the behaviour now is to default to /var/xjump, and let the user create that directory and give it the appropriate permissions.
@GrahamcOfBorg build xjump |
License? |
Failure on x86_64-linux (full log) Partial log (click to expand)
|
Failure on x86_64-darwin (full log) Partial log (click to expand)
|
Failure on aarch64-linux (full log) Partial log (click to expand)
|
The failures are because installation tries to create the score directory. |
@GrahamcOfBorg build xjump |
Failure on x86_64-linux (full log) Partial log (click to expand)
|
Failure on aarch64-linux (full log) Partial log (click to expand)
|
Failure on x86_64-darwin (full log) Partial log (click to expand)
|
It looks like macOS failure is not because of the scorefile, so it's OK to also set |
@GrahamcOfBorg build xjump |
1 similar comment
@GrahamcOfBorg build xjump |
Failure on aarch64-linux (full log) Partial log (click to expand)
|
Failure on x86_64-darwin (full log) Partial log (click to expand)
|
Failure on x86_64-linux (full log) Partial log (click to expand)
|
Ok, the failure is my mistake, can we rebuild? |
@GrahamcOfBorg build xjump |
Success on x86_64-linux (full log) Partial log (click to expand)
|
Success on aarch64-linux (full log) Partial log (click to expand)
|
Success on x86_64-darwin (full log) Partial log (click to expand)
|
Yay! |
Motivation for this change
I had too much time in my life, so I decided to waste it playing this classic addictive game.
Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)