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.

Sign up for GitHub

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

Jump to bottom

mpv-unwrapped: 0.34.1 -> 0.35.0 #200914

Closed
wants to merge 2 commits into from
Closed

mpv-unwrapped: 0.34.1 -> 0.35.0 #200914

wants to merge 2 commits into from

Conversation

SuperSandro2000
Copy link
Member

./result/bin/mpv https://www.youtube.com/watch?v=dQw4w9WgXcQ works

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@SuperSandro2000
Copy link
Member Author

Can't request a review from @adisbladis, then so be it.

@ofborg ofborg bot requested review from tadeokondrak, fpletz, Ma27 and globin November 12, 2022 18:28
@mweinelt
Copy link
Member

mweinelt commented Nov 12, 2022 •
edited
Loading

https://github.com/mpv-player/mpv/releases/tag/v0.35.0

  • We probably want to migrate to meson
  • We definitely want pipewire support on linux

@jansol
Copy link
Contributor

jansol commented Nov 12, 2022

./result/bin/mpv https://www.youtube.com/watch?v=dQw4w9WgXcQ works

nice

  • We probably want to migrate to meson
  • We definitely want to support pipewire on linux

very agreed on both accounts

@AndersonTorres
Copy link
Member

  • We probably want to migrate to meson

Yes, I want it!

  • We definitely want pipewire support on linux

Good.

@mweinelt
Copy link
Member

Motivated me enough to get #200922 going.

AndersonTorres
AndersonTorres approved these changes Nov 12, 2022
View reviewed changes
Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

We can change it to Meson tomorrow.

@SuperSandro2000
Copy link
Member Author

  • We definitely want pipewire support on linux

Totally missed that because my brain ignored everything that looked like git log.

  • We probably want to migrate to meson

I can give it a try but if it is buggy then I'll leave the fixing to someone else.

@SuperSandro2000
Copy link
Member Author

SuperSandro2000 commented Nov 12, 2022 •
edited
Loading

@ofborg build mpv-unwrapped mpv

@ofborg ofborg bot requested a review from AndersonTorres November 12, 2022 22:44
@AndersonTorres
Copy link
Member

Aaaaand... Darwin blows up again

https://logs.nix.ci/?key=nixos/nixpkgs.200914&attempt_id=ce2e3323-3003-4ddc-96f1-cec2ab675e41

@AndersonTorres
Copy link
Member

I suggest to split this commit into two: the one updating version and hash, and the next updating the build system. After all, the thing still builds with waf.

@toonn
Copy link
Contributor

toonn commented Nov 13, 2022

Took a look at this, needs xcodebuild for Darwin but seems to then require Swift as well and that's not packaged for Darwin yet, which turns this into a bit of a blocker : / Being worked on in #189977 though \o/

@jansol
Copy link
Contributor

jansol commented Nov 13, 2022

FWIW: mpv-player/mpv#10859
Seems to require an unconventional setup to actually trigger but it's probably worth waiting a bit to see where this goes.

@adisbladis adisbladis force-pushed the mpv-unwrapped branch 2 times, most recently from 75b66e3 to 4ba409d Compare November 14, 2022 02:52
@adisbladis
Copy link
Member

I took the liberty of fixing up this PR:

  • Commits are split into two, one for the version bump and another for the meson changes
    We could drop the meson changes from this PR and deal with meson in a follow-up PR if it's a blocker on darwin.
  • dvdbinSupport is a typo, it should be dvbinSupport
  • pipewireSupport shouldn't be guarded by config.pulseaudio, just check stdenv.isLinux instead.
  • Used the mesonFeatureFlag implementation from @AndersonTorres as it's more readable.
  • Removed the top-level with lib; statement as it breaks static analysis.

@ofborg ofborg bot requested a review from AndersonTorres November 14, 2022 03:16
AndersonTorres
AndersonTorres suggested changes Nov 15, 2022
View reviewed changes
pkgs/applications/video/mpv/default.nix Outdated Show resolved Hide resolved
pkgs/applications/video/mpv/default.nix
@@ -226,6 +215,6 @@ in stdenv.mkDerivation rec {
'';
license = licenses.gpl2Plus;
maintainers = with maintainers; [ AndersonTorres fpletz globin ma27 tadeokondrak ];
platforms = platforms.darwin ++ platforms.linux;
platforms = platforms.unix;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
platforms = platforms.unix;
platforms = platforms.unix;
broken = stdenv.isDarwin;

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's acceptable to break such a widely-used package on Darwin.

Copy link
Member

Choose a reason for hiding this comment

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

If anyone can fix this, go ahead. For now this is our current place.

Copy link
Member

Choose a reason for hiding this comment

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

AFAICT, it's unnecessary though? WAF is still supported by upstream and meson was only just added in this release. Why do we need to switch right now?

Proper swift support for Darwin seems to be a prerequisite for MPV via meson and that is coming soon. Chances are it's being worked on as we speak.

We can delay the switch to meson until a later date. Remaining on WAF isn't the right way in the long run but we don't need to rush switching to meson either.

Atemu
Atemu requested changes Nov 15, 2022
View reviewed changes
Copy link
Member

@Atemu Atemu left a comment •
edited
Loading

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 200914 run on aarch64-darwin 1

7 packages marked as broken and skipped:
  • anki
  • hydrus
  • jellyfin-mpv-shim
  • mnemosyne
  • mpc-qt
  • plex-mpv-shim
  • sublime-music
8 packages failed to build:
  • ani-cli
  • curseradio
  • dmlive
  • mpv-unwrapped
  • python310Packages.mpv
  • python39Packages.mpv
  • somafm-cli
  • ytfzf

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

8 packages marked as broken and skipped:
  • anki
  • hydrus
  • jellyfin-media-player
  • jellyfin-mpv-shim
  • mnemosyne
  • mpc-qt
  • plex-mpv-shim
  • sublime-music
8 packages failed to build:
  • ani-cli
  • curseradio
  • dmlive
  • mpv-unwrapped
  • python310Packages.mpv
  • python39Packages.mpv
  • somafm-cli
  • ytfzf

This cannot be merged in its current state. It breaks Darwin support entirely and this is a widely-used package.

I'm generally in favour of using meson over legacy build systems but it seems that must wait until we support its dependencies on Darwin in this case.
In the case of swift, that will be quite some time; at the very least one staging-next cycle which will probably be delayed due to the release.

The update and pipewire support should be possible to realise without meson. The update is important, meson support is merely a technical nice-to-have.

pkgs/applications/video/mpv/default.nix Show resolved Hide resolved
@Atemu
Copy link
Member

Atemu commented Nov 15, 2022

IMO this should also not be backported to stable; too much room for breakage.

@AndersonTorres
Copy link
Member

I'm generally in favour of using meson over legacy build systems but it seems that must wait until we support its dependencies on Darwin in this case.

Seems like a nice idea after all!

@mweinelt
Copy link
Member

The whole idea was to recognize the release notes when doing updates. There was no indication that darwin would break in a non-fixable way.

@SuperSandro2000
Copy link
Member Author

Lets see how happy fruit corp is with just the version bump #201367

@ofborg ofborg bot requested review from fpletz and AndersonTorres November 15, 2022 20:04
@AndersonTorres
Copy link
Member

Nice idea implemented. Thanks you all!

@SuperSandro2000
Copy link
Member Author

SuperSandro2000 commented Nov 15, 2022 •
edited
Loading

No, it isn't. The meson change is still open and now I must open a new PR because I force pushed the closed PR.

#201379

@AndersonTorres
Copy link
Member

Yes it is. Now we can focus on making meson work, while the end users can use mpv on Darwin.

Looks like an absolute win to me.

@SuperSandro2000
Copy link
Member Author

My plan was to repurpose this PR to keep the feedback on the meson switch since it is not as trivial as first thought.

@OPNA2608 OPNA2608 mentioned this pull request Nov 17, 2022
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Atemu Atemu Atemu requested changes

@jansol jansol jansol left review comments

@peterhoeg peterhoeg peterhoeg left review comments

@mweinelt mweinelt mweinelt left review comments

@tadeokondrak tadeokondrak Awaiting requested review from tadeokondrak

@Ma27 Ma27 Awaiting requested review from Ma27

@globin globin Awaiting requested review from globin

@fpletz fpletz Awaiting requested review from fpletz

@AndersonTorres AndersonTorres Awaiting requested review from AndersonTorres

Assignees
No one assigned
Labels
10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@SuperSandro2000 @mweinelt @jansol @AndersonTorres @toonn @adisbladis @Atemu @fpletz @peterhoeg