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

Merged
merged 7 commits into from
Jun 8, 2024
Merged

mpv: 0.37.0 -> 0.38.0 #304349

merged 7 commits into from
Jun 8, 2024

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.

@AndersonTorres AndersonTorres marked this pull request as ready for review April 15, 2024 19:32
@AndersonTorres AndersonTorres marked this pull request as draft April 15, 2024 23:05
@AndersonTorres AndersonTorres changed the title mpv: refactor mpv: 0.37.0 -> 0.38.0 Apr 19, 2024
@AndersonTorres AndersonTorres marked this pull request as ready for review April 19, 2024 02:44
@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
Contributor

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
Contributor

@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

Patch applied!

@AndersonTorres
Copy link
Member Author

AndersonTorres commented Jun 8, 2024

@Scrumplex I have patched the expression for sublime at #318319. Can you please test it?

@Scrumplex
Copy link
Member

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

2 packages failed to build:
  • ki
  • ki.dist
107 packages built:
  • adl
  • ani-cli
  • anilibria-winmaclinux
  • anime-downloader
  • anime-downloader.dist
  • anki
  • anki.dist
  • anki.doc
  • anki.man
  • castero
  • castero.dist
  • celluloid
  • cplay-ng
  • cplay-ng.dist
  • curseradio
  • curseradio.dist
  • deepin.dde-gsettings-schemas
  • deepin.deepin-movie-reborn
  • deepin.deepin-movie-reborn.dev
  • delfin
  • dmlive
  • dra-cla
  • ff2mpv
  • ff2mpv-go
  • flet-client-flutter
  • flet-client-flutter.debug
  • flet-client-flutter.pubcache
  • gonic
  • gtk-pipe-viewer
  • gtk-pipe-viewer.devdoc
  • haruna
  • hydrus
  • hydrus.doc
  • hypnotix
  • invidtui
  • jellyfin-media-player
  • jellyfin-mpv-shim
  • jellyfin-mpv-shim.dist
  • jftui
  • kdePackages.mpvqt
  • kdePackages.mpvqt.debug
  • kdePackages.mpvqt.dev
  • kdePackages.mpvqt.devtools
  • kdePackages.plasmatube
  • kdePackages.plasmatube.debug
  • kdePackages.plasmatube.dev
  • kdePackages.plasmatube.devtools
  • kdePackages.tokodon
  • kdePackages.tokodon.debug
  • kdePackages.tokodon.dev
  • kdePackages.tokodon.devtools
  • klipperscreen
  • libsForQt5.plasmatube
  • libsForQt5.tokodon
  • memento
  • minitube
  • mnemosyne
  • mnemosyne.dist
  • mov-cli
  • mov-cli.dist
  • mpc-qt
  • mpv
  • mpv-unwrapped
  • mpv-unwrapped.dev
  • mpv-unwrapped.doc
  • mpv-unwrapped.man
  • mpvScripts.acompressor
  • mpvScripts.autocrop
  • mpvScripts.autodeint
  • mpvScripts.autoload
  • mpvScripts.inhibit-gnome
  • mpvScripts.mpris
  • mpvpaper
  • photoqt
  • pipe-viewer
  • pipe-viewer.devdoc
  • plex-media-player
  • plex-mpv-shim
  • plex-mpv-shim.dist
  • python311Packages.flet
  • python311Packages.flet-runtime
  • python311Packages.flet-runtime.dist
  • python311Packages.flet.dist
  • python311Packages.mpv
  • python311Packages.mpv.dist
  • python312Packages.flet
  • python312Packages.flet-runtime
  • python312Packages.flet-runtime.dist
  • python312Packages.flet.dist
  • python312Packages.mpv
  • python312Packages.mpv.dist
  • qimgv
  • radioboat
  • somafm-cli
  • spotube
  • stremio
  • sublime-music
  • sublime-music.dist
  • subtitleedit
  • supersonic
  • supersonic-wayland
  • svp
  • tomato-c
  • wtwitch
  • youtube-tui
  • ytfzf
  • ytui-music

@Scrumplex
Copy link
Member

Seems to build fine now!

Copy link
Contributor

@marsam marsam left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@marsam marsam merged commit c95c1a5 into NixOS:master Jun 8, 2024
23 of 24 checks passed
@teto
Copy link
Member

teto commented Jun 11, 2024

shouldn't there be an alias/redirection instead of hitting:

    error: attribute 'wrapMpv' missing
       at /nix/store/1jnssw33bwg5cywr9arnd4pg02pdx4l3-source/modules/programs/mpv.nix:59:5:
           58|   else
           59|     pkgs.wrapMpv pkgs.mpv-unwrapped { scripts = cfg.scripts; };

rycee pushed a commit to nix-community/home-manager that referenced this pull request Jun 11, 2024
`wrapMpv` was removed from nixpkgs since this PR:
NixOS/nixpkgs#304349

Co-authored-by: Mario Rodas <marsam@users.noreply.github.com>
@sersorrel
Copy link
Contributor

Some note about how to fix breakage resulting from this would have been nice :)

I came up with something like:

     programs.mpv = {
       enable = true;
-      package = pkgs.wrapMpv (pkgs.mpv-unwrapped.override {
-        ffmpeg = pkgs.ffmpeg-full;
-      }) {
+      package = (pkgs.mpv-unwrapped.wrapper {
+        mpv = pkgs.mpv-unwrapped.override {
+          ffmpeg = pkgs.ffmpeg-full;
+        };
+      }).override {
         scripts = [
           pkgs.mpvScripts.mpris

@AndersonTorres
Copy link
Member Author

AndersonTorres commented Jun 12, 2024

shouldn't there be an alias/redirection instead of hitting:

    error: attribute 'wrapMpv' missing
       at /nix/store/1jnssw33bwg5cywr9arnd4pg02pdx4l3-source/modules/programs/mpv.nix:59:5:
           58|   else
           59|     pkgs.wrapMpv pkgs.mpv-unwrapped { scripts = cfg.scripts; };

The interface changed in the new release. Alias would not work
However why wasn't this catched by ofBorg? o.O

P.S.: oh, another case for phagocyte.

@eclairevoyant
Copy link
Contributor

The interface changed in the new release. Alias would not work

At least a throw would be ideal to point at the new interface

why wasn't this catched by ofBorg

borgo doesn't test code in files that you didn't touch, unless you tell it to, or there's some package test, etc

@sersorrel
Copy link
Contributor

there was a test, but it seems like it wasn't set up properly so ofborg could run it: #318588 (comment)

seems like there was little/no other usage of wrapMpv in nixpkgs itself, so nixpkgs-review would not have helped either

The interface changed in the new release. Alias would not work

wouldn't something like this work? (completely untested)

wrapMpv = unwrapped: (pkgs.mpv-unwrapped.wrapper { mpv = unwrapped; }).override;

@AndersonTorres
Copy link
Member Author

borgo doesn't test code in files that you didn't touch, unless you tell it to, or there's some package test, etc

indeed this code was external, from home-manager.

msfjarvis added a commit to msfjarvis/dotfiles that referenced this pull request Jun 16, 2024
msfjarvis added a commit to msfjarvis/dotfiles that referenced this pull request Jun 18, 2024
@bobby285271 bobby285271 mentioned this pull request Jul 7, 2024
13 tasks
sinanmohd pushed a commit to sinanmohd/home-manager that referenced this pull request Jul 16, 2024
`wrapMpv` was removed from nixpkgs since this PR:
NixOS/nixpkgs#304349

Co-authored-by: Mario Rodas <marsam@users.noreply.github.com>
@Profpatsch
Copy link
Member

Hi, why didn’t you add a backwards-compat layer for wrapMpv? Or a deprecation period?

@AndersonTorres
Copy link
Member Author

We did not ponder it at that time.

Besides, the discussions about, ehr, breakages on unstable are very hot:

https://discourse.nixos.org/t/can-we-please-stop-breaking-stuff-willy-nilly/48496

@winterqt
Copy link
Member

How is "people are discussing how to handle breakages" an excuse for breaking more things? At least stick with the first reason of "nobody realized," which is at least logical.

@AndersonTorres
Copy link
Member Author

AndersonTorres commented Jul 22, 2024

I was not saying this as an excuse, in case you did not notice.

I merely pointed this Discourse link because we do not have anything remotely similar to a deprecation policy (besides "not breaking stable, tagged NixOS releases").

And that lack of a deprecation policy is a recurrent issue that is receiving a little bit more attention after each iteration:

@Profpatsch
Copy link
Member

I don’t particularly care about people writing books on discourse on this topic (they have been doing that every few months for the last ~decade or so).

Just adding a little “hey this was replaced with xy, here’s an easy example how to fix it in your config” throw would be enough.

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