open-adventure: init at 1.20 by EmanuelM153 · Pull Request #370955 · NixOS/nixpkgs

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.

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.

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

Suggested change

python3Packages.pyyaml

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

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

Please use buildFlags instead.

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

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

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

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

+    # 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

+    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

+    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 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 4, 2025

SigmaSquadron reviewed Jan 4, 2025

View reviewed changes

SigmaSquadron reviewed Jan 5, 2025

View reviewed changes

Comment thread pkgs/by-name/op/open-adventure/package.nix Outdated

SigmaSquadron reviewed Jan 5, 2025

View reviewed changes

Comment thread pkgs/by-name/op/open-adventure/package.nix Outdated

Comment thread pkgs/by-name/op/open-adventure/package.nix Outdated

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 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 persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Feb 14, 2025

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

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 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

Merged

1 task

-  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
+  '';

-  buildFlags = ''
-    advent.6
-  '';
+  buildFlags = [
+    "advent.6"
+  ];

Uh oh!

Conversation

EmanuelM153 commented Jan 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Things done

Uh oh!

SigmaSquadron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EmanuelM153 commented Jan 5, 2025

Uh oh!

SigmaSquadron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ethancedwards8 commented Jan 5, 2025

nixpkgs-review result

aarch64-darwin

Uh oh!

lucasew commented Jan 6, 2025

nixpkgs-review result

x86_64-linux

Uh oh!

EmanuelM153 commented Jan 7, 2025

Uh oh!

SigmaSquadron commented Jan 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EmanuelM153 commented Jan 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EmanuelM153 commented Jan 12, 2025

Uh oh!

SigmaSquadron left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

nixpkgs-review result

x86_64-linux

Uh oh!

SigmaSquadron commented Jan 12, 2025

Uh oh!

EmanuelM153 commented Jan 12, 2025

Uh oh!

nixos-discourse commented Feb 11, 2025

Uh oh!

nixos-discourse commented Feb 26, 2025

Uh oh!

TomaSajt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomaSajt commented Feb 26, 2025

Uh oh!

EmanuelM153 commented Feb 26, 2025

Uh oh!

nixpkgs-ci Bot commented Feb 28, 2025

Uh oh!

Reviewers

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`