zxtune: init at r5054 #265519

EBADBEEF · 2023-11-04T20:26:32Z

Description of changes

zxtune is a cross-platform chiptunes music player.

Things done

FliegendeWurst

Thanks for packaging zxtune!

Package builds and zxtune-qt launches.

Result of nixpkgs-review pr 265519 run on x86_64-linux 1

1 package built:

zxtune

FliegendeWurst · 2023-11-12T17:14:23Z

pkgs/applications/audio/zxtune/default.nix

+in with lib;
+stdenv.mkDerivation rec {
+  pname = "zxtune";
+  version = "r5054";


Suggested change

version = "r5054";

version = "5054";

Usually the version attribute starts with a digit.

FliegendeWurst · 2023-11-12T17:15:01Z

pkgs/applications/audio/zxtune/default.nix

+  src = fetchFromBitbucket {
+    owner = "zxtune";
+    repo = "zxtune";
+    rev = "${version}";


Suggested change

rev = "${version}";

rev = "r${version}";

FliegendeWurst · 2023-11-12T17:37:41Z

pkgs/applications/audio/zxtune/default.nix

+  platformName = "linux";
+  staticBuildInputs = [ boost zlib ]
+    ++ lib.optional withQt (if (supportWayland) then qt5.qtwayland else qt5.qtbase);
+in with lib;


I'm not sure, but I believe with lib; should be avoided in front of package definitions. This one uses only a few lib.* functions.

EBADBEEF · 2023-11-12T19:34:58Z

Thanks for the review!

FliegendeWurst

Result of nixpkgs-review pr 265519 run on x86_64-linux 1

1 package built:

zxtune

I'm assuming you tested the off-by-default file/audio backends.

FliegendeWurst · 2023-11-12T21:11:19Z

pkgs/applications/audio/zxtune/default.nix

+    '';
+    homepage = "https://zxtune.bitbucket.io/";
+    license = licenses.gpl3;
+    platforms = platforms.linux;


This is set to Linux because you can't test on Darwin, right?

Exactly. I suspect there will need to be some more tricks in the makefile stuff.

Good to know (and perfectly acceptable, most maintainers can't test on Darwin). It was a bit confusing to see all those isLinux / isDarwin checks and then this line at the end.

I'd put a short comment above the "platforms =" line so future readers are not confused.

FliegendeWurst · 2023-11-12T21:13:01Z

pkgs/applications/audio/zxtune/default.nix

+      "support_${name}=" + (if (var) then "1" else "");
+    makeOptsCommon = [
+      ''-j$NIX_BUILD_CORES''
+      ''root.version=${version}''


Not r${version}?

Yeah, should be, will do.

EBADBEEF · 2023-11-12T23:15:45Z

Result of nixpkgs-review pr 265519 run on x86_64-linux 1

1 package built:
I'm assuming you tested the off-by-default file/audio backends.

Uh oh. You caught me! It doesn't build with SDL2 enabled.

EBADBEEF · 2023-11-13T20:00:51Z

OK, I think I have addressed all the review feedback now! Thanks for the thoughtful comments.

tested build and functionality with all optional flags
added comment about darwin/windows platform support

FliegendeWurst

Result of nixpkgs-review pr 265519 run on x86_64-linux 1

1 package built:

zxtune

nixos-discourse · 2023-11-14T15:08:01Z

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/2919

pbsds

Result of nixpkgs-review pr 265519 run on x86_64-linux 1

1 package built:

zxtune

Welcome to nixpkgs! This one is looking pretty good!

Runs fine on my end, just some small nits and improvements

pbsds · 2023-11-19T19:01:21Z

pkgs/applications/audio/zxtune/default.nix

+  preFixup = lib.optionalString withQt ''
+    wrapQtApp "$out/bin/zxtune-qt"
+  '';
+


Suggested change

passthru.updateScript = nix-update-script {

attrPath = "zxtune";

extraArgs = [ "--version-regex" "r(.*)" ];

};

pbsds · 2023-11-19T19:01:56Z

pkgs/applications/audio/zxtune/default.nix

+{ lib
+, stdenv
+, fetchFromBitbucket
+, boost


Suggested change

, boost

, nix-update-script

, boost

pbsds · 2023-11-19T19:03:18Z

pkgs/applications/audio/zxtune/default.nix

+  ];
+
+  postPatch = ''
+    sed -e 's@OpenAL/@AL/@g' -i src/sound/backends/gates/openal_api.h


Suggested change

sed -e 's@OpenAL/@AL/@g' -i src/sound/backends/gates/openal_api.h

substituteInPlace src/sound/backends/gates/openal_api.h \

--replace "OpenAL/" "AL/"

pbsds · 2023-11-19T19:05:30Z

pkgs/applications/audio/zxtune/default.nix

+    make ${builtins.toString makeOptsCommon} -C apps/xtractor
+    make ${builtins.toString makeOptsCommon} -C apps/zxtune123
+  '' + lib.optionalString withQt ''
+    make ${builtins.toString (makeOptsCommon ++ makeOptsQt)} -C apps/zxtune-qt


Suggested change

make ${builtins.toString makeOptsCommon} -C apps/xtractor

make ${builtins.toString makeOptsCommon} -C apps/zxtune123

'' + lib.optionalString withQt ''

make ${builtins.toString (makeOptsCommon ++ makeOptsQt)} -C apps/zxtune-qt

runHook preBuild

make ${builtins.toString makeOptsCommon} -C apps/xtractor

make ${builtins.toString makeOptsCommon} -C apps/zxtune123

'' + lib.optionalString withQt ''

make ${builtins.toString (makeOptsCommon ++ makeOptsQt)} -C apps/zxtune-qt

'' + ''

runHook postBuild

EBADBEEF · 2023-11-22T14:04:42Z

Changes:

Rebased (darn it is hard to track my changes with this, sorry... not really sure the best way to work with github here)
Moved to pkgs/by-name
Enabled flac, mp3, vorbis by default (it's already a big package, might as well add this functionality)
Followed @pbsds review suggestions (build hooks, substituteInPlace instead of sed, nix-update)
Regarding nix-update... I didn't know that existed, thanks! However, it doesn't support bitbucket! Hopefully that will change soon => Support bitbucket.org Mic92/nix-update#208 ;-) Also noticed I can't use "refs/tags" syntax for rev with nix-update.

pbsds · 2023-11-22T19:48:54Z

@ofborg eval

I've been listening to my old game soundtrack rips for nearly an hour now. Love this

pbsds · 2023-11-22T20:05:40Z

pkgs/by-name/zx/zxtune/package.nix

+    longDescription = ''
+      Player of computer music from ZX Spectrum, Amstrad, Sam Coupe, PC, Amiga,
+      Atari, Acorn, C64, SNES, Nes, Sega Master System, GameBoy, TurboGrafX,
+      MSX, NDS, and more.


Suggested change

MSX, NDS, and more.

MSX, NDS, and more. Powered by vgmstream.

For the sake of discoverability

(ofborg seems to need the force push anyway)

Nice idea. Updated description and included other notable libraries. Also bumped version to r5055.

github-actions · 2023-11-24T21:31:59Z

Successfully created backport PR for release-23.11:

[Backport release-23.11] zxtune: init at r5054 #269723

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

EBADBEEF force-pushed the master branch from 8c06c0f to 33e1dd5 Compare November 4, 2023 20:40

github-actions bot added the 6.topic: python label Nov 4, 2023

EBADBEEF force-pushed the master branch from 33e1dd5 to 811ee30 Compare November 4, 2023 20:41

github-actions bot removed the 6.topic: python label Nov 4, 2023

ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Nov 4, 2023

EBADBEEF force-pushed the master branch from 811ee30 to 877e13a Compare November 9, 2023 18:14

FliegendeWurst reviewed Nov 12, 2023

View reviewed changes

EBADBEEF force-pushed the master branch 2 times, most recently from d05c42a to f0e92db Compare November 12, 2023 19:34

FliegendeWurst approved these changes Nov 12, 2023

View reviewed changes

EBADBEEF force-pushed the master branch 2 times, most recently from 7a9afe6 to 855771c Compare November 13, 2023 19:59

FliegendeWurst approved these changes Nov 13, 2023

View reviewed changes

FliegendeWurst added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 13, 2023

pbsds reviewed Nov 19, 2023

View reviewed changes

maintainers: add EBADBEEF

d2bfc75

EBADBEEF force-pushed the master branch from 855771c to d0d5af2 Compare November 22, 2023 13:53

pbsds reviewed Nov 22, 2023

View reviewed changes

zxtune: init at r5055

97ac308

EBADBEEF force-pushed the master branch from d0d5af2 to 97ac308 Compare November 22, 2023 21:57

pbsds merged commit 2eb4426 into NixOS:master Nov 24, 2023

pbsds added the backport release-23.11 label Nov 24, 2023

github-actions bot mentioned this pull request Nov 24, 2023

[Backport release-23.11] zxtune: init at r5054 #269723

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zxtune: init at r5054 #265519

zxtune: init at r5054 #265519

EBADBEEF commented Nov 4, 2023

FliegendeWurst left a comment

FliegendeWurst Nov 12, 2023

EBADBEEF Nov 12, 2023

FliegendeWurst Nov 12, 2023

EBADBEEF Nov 12, 2023

FliegendeWurst Nov 12, 2023

EBADBEEF commented Nov 12, 2023

FliegendeWurst left a comment

FliegendeWurst Nov 12, 2023

EBADBEEF Nov 12, 2023

FliegendeWurst Nov 13, 2023

FliegendeWurst Nov 12, 2023 •

edited

Loading

EBADBEEF Nov 12, 2023

EBADBEEF commented Nov 12, 2023

EBADBEEF commented Nov 13, 2023

FliegendeWurst left a comment

nixos-discourse commented Nov 14, 2023

pbsds left a comment

pbsds Nov 19, 2023

pbsds Nov 19, 2023

pbsds Nov 19, 2023 •

edited

Loading

pbsds Nov 19, 2023

EBADBEEF commented Nov 22, 2023

pbsds commented Nov 22, 2023 •

edited

Loading

pbsds Nov 22, 2023

pbsds Nov 22, 2023 •

edited

Loading

EBADBEEF Nov 22, 2023

github-actions bot commented Nov 24, 2023

+  passthru.updateScript = nix-update-script {
+    attrPath = "zxtune";
+    extraArgs = [ "--version-regex" "r(.*)" ];
+  };

	sed -e 's@OpenAL/@AL/@g' -i src/sound/backends/gates/openal_api.h
	substituteInPlace src/sound/backends/gates/openal_api.h \
	--replace "OpenAL/" "AL/"

	MSX, NDS, and more.
	MSX, NDS, and more. Powered by vgmstream.

zxtune: init at r5054 #265519

zxtune: init at r5054 #265519

Conversation

EBADBEEF commented Nov 4, 2023

Description of changes

Things done

FliegendeWurst 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EBADBEEF commented Nov 12, 2023

FliegendeWurst 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

FliegendeWurst Nov 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EBADBEEF commented Nov 12, 2023

EBADBEEF commented Nov 13, 2023

FliegendeWurst left a comment

Choose a reason for hiding this comment

nixos-discourse commented Nov 14, 2023

pbsds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbsds Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EBADBEEF commented Nov 22, 2023

pbsds commented Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

pbsds Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 24, 2023

FliegendeWurst Nov 12, 2023 •

edited

Loading

pbsds Nov 19, 2023 •

edited

Loading

pbsds commented Nov 22, 2023 •

edited

Loading

pbsds Nov 22, 2023 •

edited

Loading