Skip to content
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

corsix-th: darwin build, add maintainer #318535

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

matteo-pacini
Copy link
Contributor

@matteo-pacini matteo-pacini commented Jun 9, 2024

Description of changes

  • Added darwin build to corsix-th.
  • Added myself as second maintainer for Darwin.
  • Added nix-update-script as passthru.updateScript

Tested on aarch64-darwin. Also rebuilt on aarch64-linux to ensure that my changes did not break stuff.

Screenshot 2024-06-09 at 15 15 14

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Jun 9, 2024
@matteo-pacini matteo-pacini requested a review from hughobrien June 9, 2024 14:28
@nixos-discourse
Copy link

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

@matteo-pacini matteo-pacini marked this pull request as draft June 10, 2024 13:43
@matteo-pacini matteo-pacini marked this pull request as ready for review June 10, 2024 20:17
@matteo-pacini matteo-pacini marked this pull request as draft June 10, 2024 20:19
@matteo-pacini matteo-pacini removed the 8.has: package (new) This PR adds a new package label Jun 10, 2024
@matteo-pacini matteo-pacini marked this pull request as ready for review June 10, 2024 21:03
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jun 10, 2024
Copy link
Contributor

@paparodeo paparodeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • i don't see a reason to remove the finalAttrs pattern
  • can put lua.withPackages in the buildInputs. no need for luaEnv attribute
  • i think lua.withPackages brings in lua so can remove lua from buildInputs
suggested changes
diff --git a/pkgs/games/corsix-th/default.nix b/pkgs/games/corsix-th/default.nix
index d50c55a0718d..885de2612361 100644
--- a/pkgs/games/corsix-th/default.nix
+++ b/pkgs/games/corsix-th/default.nix
@@ -18,14 +18,14 @@
 , nix-update-script
 }:
 
-stdenv.mkDerivation rec {
+stdenv.mkDerivation (finalAttrs: {
   pname = "corsix-th";
   version = "0.67";
 
   src = fetchFromGitHub {
     owner = "CorsixTH";
     repo = "CorsixTH";
-    rev = "v${version}";
+    rev = "v${finalAttrs.version}";
     hash = "sha256-WA/VJqHXzBfVUBNtxCVsGBRzSRQ0pvDvAy03ntc0KZE=";
   };
 
@@ -33,14 +33,12 @@ stdenv.mkDerivation rec {
     ./darwin-cmake-no-fixup-bundle.patch
   ];
 
-  luaEnv = lua.withPackages(p: with p; [ luafilesystem lpeg luasec luasocket ]);
   nativeBuildInputs = [ cmake doxygen makeWrapper ];
 
   buildInputs = [
     ffmpeg
     freetype
-    lua
-    luaEnv
+    (lua.withPackages(p: with p; [ luafilesystem lpeg luasec luasocket ]))
     SDL2
     SDL2_mixer
     timidity
@@ -74,4 +72,4 @@ stdenv.mkDerivation rec {
     maintainers = with maintainers; [ hughobrien matteopacini ];
     platforms = platforms.linux ++ platforms.darwin;
   };
-}
+})

alternatively if don't want to expand luaEnv can put it in a let.

buildInputs = let
  luaEnv = lua.withPackages(p: with p; [ luafilesystem lpeg luasec luasocket ]);
in
[
  ...
  luaEnv
  ...
];

@matteo-pacini
Copy link
Contributor Author

@paparodeo removing lua yields an error during the game startup - not sure why, so I put it back in.

Screenshot 2024-06-11 at 08 39 01

Copy link
Contributor

@paparodeo paparodeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result of nixpkgs-review pr 318535 run on x86_64-darwin 1

1 package built:
  • corsix-th

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

1 package built:
  • corsix-th

@nixos-discourse
Copy link

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

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

@matteo-pacini matteo-pacini added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jun 11, 2024
@matteo-pacini matteo-pacini added needs_merger (old Marvin label, do not use) 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 Jun 12, 2024
@nixos-discourse
Copy link

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

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

@wegank
Copy link
Member

wegank commented Jun 14, 2024

Result of nixpkgs-review pr 318535 run on aarch64-darwin 1

1 package built:
  • corsix-th

@wegank wegank merged commit 316a00e into NixOS:master Jun 14, 2024
27 checks passed
@matteo-pacini matteo-pacini deleted the corsixth-darwin branch June 21, 2024 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: darwin Running or building packages on Darwin 6.topic: games 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 2 This PR was reviewed and approved by two reputable people needs_merger (old Marvin label, do not use)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants