open-adventure: init at 1.20 #370955

EmanuelM153 · 2025-01-04T19:56:52Z

open-adventure is a forward-port of the Crowther/Woods Adventure 2.5 game from 1995

Things done

Add a 👍 reaction to pull requests you find important.

SigmaSquadron

Welcome to Nixpkgs! Thanks for contributing this game.

pkgs/by-name/op/open-adventure/package.nix

EmanuelM153 · 2025-01-05T02:33:54Z

@SigmaSquadron Thank you for your corrections! I've followed your suggestions and added the upstream tests.
Please let me know if there are any other things to improve 😉.

Btw, I am not quite sure if the commands I added under the preCheck attribute are the best solution.
Maybe the problem with cppcheck is solved using an older version, and I don't know if using the substituteInPlace command, to replace /bin/echo, is the best approach.

SigmaSquadron

substituteInPlace was indeed the right call. It's pretty normal to substitute FHS paths with Nix paths in patchPhase.

pkgs/by-name/op/open-adventure/package.nix

ethancedwards8 · 2025-01-05T16:01:12Z

`nixpkgs-review` result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 370955

`aarch64-darwin`

✅ 1 package built:

open-adventure

lucasew · 2025-01-06T17:22:19Z

`nixpkgs-review` result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 370955

`x86_64-linux`

✅ 1 package built:

open-adventure

EmanuelM153 · 2025-01-07T19:29:11Z

@SigmaSquadron I have followed your corrections, and I had also created an issue on upstream to apply the fix, however they haven't answered. Furthermore, I was reviewing other issues on their repository and lately they are taking a long time to solve them (even some weeks), probably because it is not a project under development. So, maybe the package can be added now (I'll be paying attention to the appliance of the fix, and will update the package accordingly).

Btw, I don't know if the commits should be squashed 😅.

Regards,
EmanuelM153

SigmaSquadron · 2025-01-12T19:37:38Z

Btw, I don't know if the commits should be squashed 😅.

When in doubt, squash! This PR should have two commits total.

SigmaSquadron · 2025-01-12T19:38:37Z

pkgs/by-name/op/open-adventure/package.nix

+
+  nativeBuildInputs = [
+    python3Packages.python
+    python3Packages.pyyaml


Suggested change

python3Packages.pyyaml

pyyaml isn't a program that can be executed, so it goes to buildInputs.

SigmaSquadron · 2025-01-12T19:52:19Z

pkgs/by-name/op/open-adventure/package.nix

+  postBuild = ''
+    make advent.6
+  '';


Please use buildFlags instead.

SigmaSquadron · 2025-01-12T19:53:51Z

pkgs/by-name/op/open-adventure/package.nix

+    # Fix the lack of the = symbol in the cli of cppcheck
+    substituteInPlace Makefile --replace-fail "--template " "--template="
+
+    # Replace /bin/echo with the actual path in the nix store


Suggested change

# Replace /bin/echo with the actual path in the nix store

I'm a fan of superfluous comments myself, but this is a pretty well-known pattern in Nixpkgs, and doesn't really need to be there. If you feel strongly about it, feel free to leave it here; this isn't a blocking comment.

SigmaSquadron · 2025-01-12T19:55:43Z

pkgs/by-name/op/open-adventure/package.nix

+  preInstall = ''
+    mkdir -vp "$out/bin" "$out/share/man/man6" "$out/share/applications/" "$out/share/icons/hicolor/scalable/apps"
+  '';
+
+  installPhase = ''
+    runHook preInstall
+    cp ./advent $out/bin
+    runHook postInstall
+  '';
+
+  postInstall = ''
+    cp ./advent.6 $out/share/man/man6
+    cp ./advent.desktop $out/share/applications
+    cp ./advent.svg $out/share/icons/hicolor/scalable/apps
+  '';


Suggested change

preInstall = ''

mkdir -vp "$out/bin" "$out/share/man/man6" "$out/share/applications/" "$out/share/icons/hicolor/scalable/apps"

'';

installPhase = ''

runHook preInstall

cp ./advent $out/bin

runHook postInstall

'';

postInstall = ''

cp ./advent.6 $out/share/man/man6

cp ./advent.desktop $out/share/applications

cp ./advent.svg $out/share/icons/hicolor/scalable/apps

'';

installPhase = ''

runHook preInstall

mkdir -vp "$out/bin" "$out/share/man/man6" "$out/share/applications/" "$out/share/icons/hicolor/scalable/apps"

cp ./advent $out/bin

cp ./advent.6 $out/share/man/man6

cp ./advent.desktop $out/share/applications

cp ./advent.svg $out/share/icons/hicolor/scalable/apps

runHook postInstall

'';

This should also use the install command whenever possible. I don't know the package filesystem layout to suggest an implementation.

EmanuelM153 · 2025-01-12T20:18:27Z

Thank you for your corrections, I've applied them now

SigmaSquadron · 2025-01-12T20:54:39Z

pkgs/by-name/op/open-adventure/package.nix

+  buildFlags = ''
+    advent.6
+  '';


Suggested change

buildFlags = ''

advent.6

'';

buildFlags = [

"advent.6"

];

buildFlags is a list.

EmanuelM153 · 2025-01-12T21:19:21Z

Ahh ok thank you.
I also forgot the use of the install command. It is now updated

SigmaSquadron

$ ./results/open-adventure-x86_64-linux/bin/advent

Welcome to Adventure!!  Would you like instructions?

`nixpkgs-review` result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 370955

`x86_64-linux`

✅ 1 package built:

open-adventure

SigmaSquadron · 2025-01-12T21:35:21Z

Gotta say, pretty fun game :)

EmanuelM153 · 2025-01-12T21:40:39Z

Jsjsj yeah. Thank you for guiding me in this process :)

nixos-discourse · 2025-02-11T15:17:24Z

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/2242

nixos-discourse · 2025-02-26T02:54:19Z

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

https://discourse.nixos.org/t/prs-in-distress/3604/98

TomaSajt

The changes look fine, left some possible improvement ideas.

TomaSajt · 2025-02-26T08:23:05Z

pkgs/by-name/op/open-adventure/package.nix

+    # https://gitlab.com/esr/open-adventure/-/issues/70
+    substituteInPlace Makefile --replace-fail "--template " "--template="
+
+    substituteInPlace tests/tapview --replace-fail "/bin/echo" ${lib.getExe' coreutils "echo"}


Does this only run during testing? If yes, I just recommend patching it to just be echo, since we know it's on our PATH.

TomaSajt · 2025-02-26T08:26:55Z

pkgs/by-name/op/open-adventure/package.nix

+    mkdir -vp "$out/bin" "$out/share/man/man6" "$out/share/applications/" "$out/share/icons/hicolor/scalable/apps"
+    install -m 555 ./advent $out/bin
+    install -m 444 ./advent.6 $out/share/man/man6
+    install -m 444 ./advent.desktop $out/share/applications
+    install -m 444 ./advent.svg $out/share/icons/hicolor/scalable/apps


A possible alternative that doesn't need mkdir. You don't need to do this if you don't want to.

Suggested change

mkdir -vp "$out/bin" "$out/share/man/man6" "$out/share/applications/" "$out/share/icons/hicolor/scalable/apps"

install -m 555 ./advent $out/bin

install -m 444 ./advent.6 $out/share/man/man6

install -m 444 ./advent.desktop $out/share/applications

install -m 444 ./advent.svg $out/share/icons/hicolor/scalable/apps

install -Dm555 ./advent -t $out/bin

install -Dm444 ./advent.6 -t $out/share/man/man6

install -Dm444 ./advent.desktop -t $out/share/applications

install -Dm444 ./advent.svg -t $out/share/icons/hicolor/scalable/apps

Also, man-pages should ideally be installed via installManPage from the installShellFiles helper script collection. IMO this is not a hard requirement, so you can keep it as is.

TomaSajt · 2025-02-26T08:32:51Z

pkgs/by-name/op/open-adventure/package.nix

+    python3Packages.pyyaml
+    libedit
+  ];
+


consider setting strictDeps = true; to ensure that you've added your deps to the correct place (native vs non-native inputs).

Suggested change

strictDeps = true;

TomaSajt · 2025-02-26T08:34:55Z

wow I just missed the merge by a minute :) Oh well, the improvements can still be made if you want to,

EmanuelM153 · 2025-02-26T13:22:26Z

Thanks! I will take into account this improvements in my future contributions :)

nixpkgs-ci · 2025-02-28T20:27:58Z

Successfully created backport PR for release-24.11:

[Backport release-24.11] open-adventure: init at 1.20 #385890

maintainers: add EmanuelM153

602583a

github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Jan 4, 2025

NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jan 4, 2025

github-actions bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Jan 4, 2025

SigmaSquadron reviewed Jan 4, 2025

View reviewed changes

SigmaSquadron reviewed Jan 5, 2025

View reviewed changes

pkgs/by-name/op/open-adventure/package.nix Outdated Show resolved Hide resolved

SigmaSquadron reviewed Jan 5, 2025

View reviewed changes

pkgs/by-name/op/open-adventure/package.nix Show resolved Hide resolved

pkgs/by-name/op/open-adventure/package.nix Outdated Show resolved Hide resolved

EmanuelM153 requested a review from SigmaSquadron January 11, 2025 16:36

SigmaSquadron reviewed Jan 12, 2025

View reviewed changes

EmanuelM153 force-pushed the add-open-adventure branch from bf9896c to fc15b8a Compare January 12, 2025 20:16

SigmaSquadron reviewed Jan 12, 2025

View reviewed changes

open-adventure: init at 1.20

1bd30bd

EmanuelM153 force-pushed the add-open-adventure branch from fc15b8a to 1bd30bd Compare January 12, 2025 21:17

SigmaSquadron approved these changes Jan 12, 2025

View reviewed changes

wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 14, 2025

ethancedwards8 approved these changes Feb 11, 2025

View reviewed changes

wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Feb 14, 2025

FliegendeWurst merged commit e7d5b81 into NixOS:master Feb 26, 2025
30 checks passed

TomaSajt reviewed Feb 26, 2025

View reviewed changes

EmanuelM153 deleted the add-open-adventure branch February 26, 2025 19:18

EmanuelM153 added the backport release-24.11 Backport PR automatically label Feb 28, 2025

nixpkgs-ci bot mentioned this pull request Feb 28, 2025

[Backport release-24.11] open-adventure: init at 1.20 #385890

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

open-adventure: init at 1.20 #370955

open-adventure: init at 1.20 #370955

EmanuelM153 commented Jan 4, 2025 •

edited

Loading

SigmaSquadron left a comment

EmanuelM153 commented Jan 5, 2025

SigmaSquadron left a comment

ethancedwards8 commented Jan 5, 2025

lucasew commented Jan 6, 2025

EmanuelM153 commented Jan 7, 2025

SigmaSquadron commented Jan 12, 2025

SigmaSquadron Jan 12, 2025

SigmaSquadron Jan 12, 2025

SigmaSquadron Jan 12, 2025

SigmaSquadron Jan 12, 2025

EmanuelM153 commented Jan 12, 2025 •

edited

Loading

SigmaSquadron Jan 12, 2025

EmanuelM153 commented Jan 12, 2025

SigmaSquadron left a comment •

edited

Loading

SigmaSquadron commented Jan 12, 2025

EmanuelM153 commented Jan 12, 2025

nixos-discourse commented Feb 11, 2025

nixos-discourse commented Feb 26, 2025

TomaSajt left a comment

TomaSajt Feb 26, 2025

TomaSajt Feb 26, 2025

TomaSajt Feb 26, 2025

TomaSajt commented Feb 26, 2025

EmanuelM153 commented Feb 26, 2025

nixpkgs-ci bot commented Feb 28, 2025

open-adventure: init at 1.20 #370955

open-adventure: init at 1.20 #370955

Conversation

EmanuelM153 commented Jan 4, 2025 • edited Loading

Things done

SigmaSquadron left a comment

Choose a reason for hiding this comment

EmanuelM153 commented Jan 5, 2025

SigmaSquadron left a comment

Choose a reason for hiding this comment

ethancedwards8 commented Jan 5, 2025

nixpkgs-review result

aarch64-darwin

lucasew commented Jan 6, 2025

nixpkgs-review result

x86_64-linux

EmanuelM153 commented Jan 7, 2025

SigmaSquadron commented Jan 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmanuelM153 commented Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

EmanuelM153 commented Jan 12, 2025

SigmaSquadron left a comment • edited Loading

Choose a reason for hiding this comment

nixpkgs-review result

x86_64-linux

SigmaSquadron commented Jan 12, 2025

EmanuelM153 commented Jan 12, 2025

nixos-discourse commented Feb 11, 2025

nixos-discourse commented Feb 26, 2025

TomaSajt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomaSajt commented Feb 26, 2025

EmanuelM153 commented Feb 26, 2025

nixpkgs-ci bot commented Feb 28, 2025

EmanuelM153 commented Jan 4, 2025 •

edited

Loading

`nixpkgs-review` result

`aarch64-darwin`

`nixpkgs-review` result

`x86_64-linux`

EmanuelM153 commented Jan 12, 2025 •

edited

Loading

SigmaSquadron left a comment •

edited

Loading

`nixpkgs-review` result

`x86_64-linux`