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

mpv: 0.37.0 -> 0.38.0 #304349

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

AndersonTorres
Copy link
Member

@AndersonTorres AndersonTorres commented Apr 15, 2024

Description of changes

closes #305164

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.05 Release Notes (or backporting 23.05 and 23.11 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.

@davidkna
Copy link
Contributor

davidkna commented Apr 19, 2024

@AndersonTorres The darwin patch shouldn't be needed anymore. This also means the package requires darwin.codesign instead of rcodesign again.

Edit: It doesn't build on darwin, it looks like this update might require a more recent apple SDK. (error: value of type 'NSCondition' has no member 'withLock')

@AndersonTorres
Copy link
Member Author

  1. I will then delete the patch
  2. Still needs Darwin testers, since I am not one of them :)

pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
pkgs/applications/video/mpv/default.nix Show resolved Hide resolved
pkgs/applications/video/mpv/default.nix Show resolved Hide resolved
pkgs/applications/video/mpv/default.nix Outdated Show resolved Hide resolved
pkgs/applications/video/mpv/default.nix Outdated Show resolved Hide resolved
pkgs/applications/video/mpv/default.nix Outdated Show resolved Hide resolved
@AndersonTorres
Copy link
Member Author

What is happening that this expression is not building?

@JohnRTitor
Copy link
Contributor

JohnRTitor commented May 11, 2024

What is happening that this expression is not building?

If I had to guess it could be related to wrapping mpv? You could try moving it to pkgs/by-name. So there is no import issue.

@AndersonTorres
Copy link
Member Author

AndersonTorres commented May 11, 2024

by-name - related errors are catched by a specific script, not by ofBorg.

Currently mpv uses that dreadful darwin.apple_sdk.callPackage instead of plain callPackage. It makes the migration impossible.

@AndersonTorres
Copy link
Member Author

AndersonTorres commented May 13, 2024

@ofborg build mpv-unwrapped

@eclairevoyant
Copy link
Member

eclairevoyant commented May 14, 2024

Currently mpv uses that dreadful darwin.apple_sdk.callPackage instead of plain callPackage. It makes the migration impossible.

It can still be migrated, just changing the path to callPackage accordingly

@AndersonTorres
Copy link
Member Author

It can still be migrated, just changing the path to callPackage accordingly

No, it does not. Saving my fingers:

#305509 (comment)

@eclairevoyant
Copy link
Member

@marsam
Copy link
Contributor

marsam commented May 15, 2024

I'm not completely sure why it fails to eval, but you can use the following as a workaround:

--- i/pkgs/applications/video/mpv/default.nix
+++ w/pkgs/applications/video/mpv/default.nix
@@ -11,6 +11,7 @@
   ninja,
   pkg-config,
   python3,
+  sigtool,
   ffmpeg,
   freefont_ttf,
   freetype,
@@ -210,7 +211,7 @@ stdenv'.mkDerivation (finalAttrs: {
       pkg-config
     ]
     ++ lib.optionals stdenv.isDarwin [
-      darwin.sigtool
+      sigtool
       xcbuild.xcrun
     ]
     ++ lib.optionals swiftSupport [ swift ]
--- i/pkgs/top-level/all-packages.nix
+++ w/pkgs/top-level/all-packages.nix
@@ -33005,6 +33005,7 @@ with pkgs;
 
   mpv-unwrapped = darwin.apple_sdk_11_0.callPackage ../applications/video/mpv {
     stdenv = if stdenv.isDarwin then swiftPackages.stdenv else stdenv;
+    inherit (darwin) sigtool;
     inherit lua;
   }; 

or

--- c/pkgs/applications/video/mpv/default.nix
+++ i/pkgs/applications/video/mpv/default.nix
@@ -22,6 +22,7 @@
   xcbuild,
   rcodesign,
+  buildPackages,
 
   waylandSupport ? stdenv.isLinux,
@@ -210,7 +211,7 @@ stdenv'.mkDerivation (finalAttrs: {
       pkg-config
     ]
     ++ lib.optionals stdenv.isDarwin [
-      darwin.sigtool
+      buildPackages.darwin.sigtool
       xcbuild.xcrun
     ]

@AndersonTorres
Copy link
Member Author

AndersonTorres commented May 15, 2024

Let's try.

P.S.: can I put this on the account of splicing?

@AndersonTorres
Copy link
Member Author

OfBorg is now all-green.
Anything more to be done?

@marsam
Copy link
Contributor

marsam commented May 16, 2024

Anything more to be done?

please mark it as broken on Darwin. I'll try to fix it later

@marsam
Copy link
Contributor

marsam commented May 20, 2024

LGTM, Thank you!
But since it seems to break the package on Darwin, I think it should be merged after the 24.05 branch-off (planned to happen in a couple of days).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update request: mpv 0.37.0 → 0.38.0
6 participants