-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
mpv: 0.37.0 -> 0.38.0 #304349
Conversation
@AndersonTorres The darwin patch shouldn't be needed anymore. This also means the package requires Edit: It doesn't build on darwin, it looks like this update might require a more recent apple SDK. ( |
|
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 |
Currently mpv uses that dreadful |
@ofborg build mpv-unwrapped |
|
No, it does not. Saving my fingers: |
Yeah, I just tested and ended up on the same error. |
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
] |
Let's try. P.S.: can I put this on the account of splicing? |
Patch applied! |
@Scrumplex I have patched the expression for sublime at #318319. Can you please test it? |
Result of 2 packages failed to build:
107 packages built:
|
Seems to build fine now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you!
shouldn't there be an alias/redirection instead of hitting:
|
`wrapMpv` was removed from nixpkgs since this PR: NixOS/nixpkgs#304349 Co-authored-by: Mario Rodas <marsam@users.noreply.github.com>
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 |
The interface changed in the new release. Alias would not work P.S.: oh, another case for phagocyte. |
At least a
borgo doesn't test code in files that you didn't touch, unless you tell it to, or there's some package test, etc |
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
wouldn't something like this work? (completely untested) wrapMpv = unwrapped: (pkgs.mpv-unwrapped.wrapper { mpv = unwrapped; }).override; |
indeed this code was external, from home-manager. |
`wrapMpv` was removed from nixpkgs since this PR: NixOS/nixpkgs#304349 Co-authored-by: Mario Rodas <marsam@users.noreply.github.com>
Hi, why didn’t you add a backwards-compat layer for wrapMpv? Or a deprecation period? |
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 |
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. |
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:
|
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” |
Description of changes
closes #305164
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.