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-unwrapped: 0.34.1 -> 0.35.0 #201367

Merged
merged 1 commit into from
Nov 15, 2022
Merged

Conversation

SuperSandro2000
Copy link
Member

Co-authored-by: adisbladis [email protected]

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

@ofborg build mpv-unwrapped

@AndersonTorres
Copy link
Member

Darwin is happy.

@AndersonTorres AndersonTorres merged commit 506f41b into NixOS:master Nov 15, 2022
@SuperSandro2000 SuperSandro2000 deleted the mpv-0-35-0 branch November 15, 2022 22:12
@github-actions
Copy link
Contributor

Backport failed for release-22.05, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin release-22.05
git worktree add -d .worktree/backport-201367-to-release-22.05 origin/release-22.05
cd .worktree/backport-201367-to-release-22.05
git checkout -b backport-201367-to-release-22.05
ancref=$(git merge-base 026f64fc17be49e481af3e0ec6833a55e582b853 b97cda7d44aa78fe915df7b0c18e3d6ed9edd157)
git cherry-pick -x $ancref..b97cda7d44aa78fe915df7b0c18e3d6ed9edd157

1 similar comment
@github-actions
Copy link
Contributor

Backport failed for release-22.05, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin release-22.05
git worktree add -d .worktree/backport-201367-to-release-22.05 origin/release-22.05
cd .worktree/backport-201367-to-release-22.05
git checkout -b backport-201367-to-release-22.05
ancref=$(git merge-base 026f64fc17be49e481af3e0ec6833a55e582b853 b97cda7d44aa78fe915df7b0c18e3d6ed9edd157)
git cherry-pick -x $ancref..b97cda7d44aa78fe915df7b0c18e3d6ed9edd157

@Atemu
Copy link
Member

Atemu commented Nov 16, 2022

Why should this be backported? It's pretty large release with lots of potential for breakage. That shouldn't go to stable, especially not this late in the support cycle.

@AndersonTorres
Copy link
Member

Why should this be backported?

Because many people use it.

Indeed you yourself suggested that Meson support should be postponed, because the breakage on Darwin was unnacceptable.

That shouldn't go to stable, especially not this late in the support cycle.

The legacy(?) waf was building it OK. It looks like a good enough reason to a backport, given that it doesn't introduce breaking changes.

Nonetheless the backport bot failed. I will not pursue it further.

@Atemu
Copy link
Member

Atemu commented Nov 16, 2022

I do not mean the build system change, I mean the feature changes.

MPV has had lots of changes since its last release, some of which are breaking. Stable should not receive breaking changes unless there are very good reasons (i.e. security issues).
Feature updates on stable, while allowed, are not in any way necessary or required; even for popular packages.

See https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#criteria-for-backporting-changes

@AndersonTorres
Copy link
Member

MPV has had lots of changes since its last release, some of which are breaking.

Not so much, I would argue. But, as I have said, I will not pursue it further.

Feature updates on stable, while allowed, are not in any way necessary or required; even for popular packages.

They aren't prohibited either. The only semblance of a hard prohibition is about breaking changes.

See https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#criteria-for-backporting-changes

I will wait a Portuguese translation. Thanks.

@OPNA2608
Copy link
Contributor

This borked MPV for me: mpv-player/mpv#10859 (this was linked in the original PR #200914 (comment))

$ mpv path/to/file.ogg
 (+) Audio --aid=1 (vorbis 2ch 44100Hz)
[ao/pipewire] Could not connect to context '(null)': Host is down!
'!impl->polling' failed at ../spa/plugins/support/loop.c:872 impl_clear()
fish: Job 1, 'mpv path/to/file.ogg' terminated by signal SIGABRT (Abbruch)

I'm not using pipewire on my system, so I assume the new pipewireSupport ? stdenv.isLinux is causing this.

@Ma27
Copy link
Member

Ma27 commented Nov 18, 2022

Looks like an absolute win to me.

The thing is, it isn't nixpkgs's sole responsibility to be up-to-date. If things have to be sorted out, nobody is hurt if we wait for a few days. In fact, people seem to have problems with it now.

I will wait a Portuguese translation. Thanks.

Your way of communicating with other maintainers here and the fact that you openly ignore our contribution guidelines seems fairly problematic to me: first of all, if you don't care, why do you even have commit access? Also, what are regular (or drive-by) contributors supposed to think of that?

@AndersonTorres
Copy link
Member

AndersonTorres commented Nov 18, 2022

First of all, why did you bring another conversation here? I was searching the context in order to write my answer, and I got confused. Please keep the things in their places (or at least provide a link in such case(s)).

The thing is, it isn't nixpkgs's sole responsibility to be up-to-date.

Until that point in the time, the version bump (without the meson part) was working as expected. Indeed I suggested to not bump meson immediately and open a pull request only for the meson part; only after many unacceptable bugs being found the PR was split.

If things have to be sorted out, nobody is hurt if we wait for a few days.

I aggree. I should not have acted so hasty.

In fact, people seem to have problems with it now.

The bug was found now. Certainly I have a portion of guilt by this, however the bug was completely unexpected.

Your way of communicating with other maintainers here

Linking the CONTRIBUTING.md file to me was a clear act of debauchery. I responded accordingly.

and the fact that you openly ignore our contribution guidelines

Allow me a brief disagreement.

I have not ignored the guidelines - quite the opposite, I have argued all the time that I was following the guidelines. (Further, linking the backport guidelines to me when I was arguing about the backporting guidelines is raw mockery, remind that.)

I even conceded the part about breaking changes and ceased to pursue the issue further.

Recapitulating, I was specifically arguing about backporting MPV, exposing why I have labeled this "worthy of backport". As I have said before, I have read the backporting criteria section, and I judged that there was no impediments; so I give it a shot.

if you don't care, why do you even have commit access?

I do care.

Also, what are regular (or drive-by) contributors supposed to think of that?

I have exposed my points above.

@AndersonTorres
Copy link
Member

I'm not using pipewire on my system, so I assume the new pipewireSupport ? stdenv.isLinux is causing this.

About this specifically, have you tried to force another output? I will upload a patch to render this option false by default.

@SuperSandro2000
Copy link
Member Author

I'm not using pipewire on my system, so I assume the new pipewireSupport ? stdenv.isLinux is causing this.

According to the thread this should be fixed for pipewire 0.3.60 which is in staging-next right now.

@Atemu
Copy link
Member

Atemu commented Nov 18, 2022

@SuperSandro2000 Will that hit master before branch-off?

@OPNA2608
Copy link
Contributor

About this specifically, have you tried to force another output?

Yes, --ao=pulseworks.

According to the thread this should be fixed for pipewire 0.3.60 which is in staging-next right now.

After an hour of building stuff with the bump cherry-picked it does fix the failing assertion & SIGABRT, so we can just wait 3 days for the merge.

@SuperSandro2000
Copy link
Member Author

@SuperSandro2000 Will that hit master before branch-off?

yes, it should be the last staging run before branch-off IIRC.

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.

5 participants