Bug 2124244 - Review Request: mpv - Movie player playing most video formats and DVDs
Summary: Review Request: mpv - Movie player playing most video formats and DVDs
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Neal Gompa
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2022-09-05 11:52 UTC by Vitaly
Modified: 2022-09-14 00:20 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2022-09-14 00:20:15 UTC
Type: ---
Embargoed:
ngompa13: fedora-review+


Attachments (Terms of Use)

Description Vitaly 2022-09-05 11:52:23 UTC
Spec URL: https://xvitaly.fedorapeople.org/for-review/mpv.spec
SRPM URL: https://xvitaly.fedorapeople.org/for-review/mpv-0.34.1-10.fc36.src.rpm
Description: Movie player playing most video formats and DVDs
Fedora Account System Username: xvitaly

Moving this package from the RPM Fusion repository. I'm its maintainer in RPM Fusion.

Comment 2 Neal Gompa 2022-09-05 13:58:10 UTC
Taking this review.

Comment 3 Neal Gompa 2022-09-05 14:08:20 UTC
Initial spec review:

> %if 0%{?fedora}
> BuildRequires:  libshaderc-devel
> BuildRequires:  pkgconfig(vulkan)
> BuildRequires:  pkgconfig(libplacebo)
> %else
> %ifarch x86_64 
> BuildRequires:  pkgconfig(vulkan)
> %endif
> %endif

All this conditional logic can be removed. The dependencies listed in the Fedora case also exist in EPEL 9, and Vulkan is not locked to x86_64 on RHEL 9.

> %if 0%{?fedora}
> Recommends:     (yt-dlp or youtube-dl)
> %else
> Recommends:     youtube-dl
> %endif

Rich dependencies work in all versions of RHEL that support weak dependencies, so you can rip this conditional out and use just the Fedora one for everything.

> %package libs-devel

The "libs-devel" name is... odd. I've not seen very many examples of this. Consider renaming it to "mpv-devel" or adding Provides to make it fit common conventions. 

> %files
> %docdir %{_docdir}/%{name}/
> %{_docdir}/%{name}/

Wouldn't "%doc %{_docdir}/%{name}/" do the same thing here?

> %files libs
> %{_libdir}/lib%{name}.so.*

The mpv library soname needs to be tracked.

Cf. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_shared_libraries

Comment 4 Vitaly 2022-09-05 15:07:40 UTC
> All this conditional logic can be removed. The dependencies listed in the Fedora case also exist in EPEL 9, and Vulkan is not locked to x86_64 on RHEL 9.

Removed.

> Rich dependencies work in all versions of RHEL that support weak dependencies, so you can rip this conditional out and use just the Fedora one for everything.

Fixed.

> The "libs-devel" name is... odd. I've not seen very many examples of this. Consider renaming it to "mpv-devel" or adding Provides to make it fit common conventions. 

Fixed.

> Wouldn't "%doc %{_docdir}/%{name}/" do the same thing here?

No. %docdir qualifier is required to mark all files from %{_docdir}/%{name}/ as documentation on RPM level.

> The mpv library soname needs to be tracked.

Done.

Comment 6 Neal Gompa 2022-09-05 16:45:03 UTC
Package review notes:

* Package is named properly per packaging guidelines
* Package builds and installs
* Package licensing is correct and license files are correctly installed
* No serious issues from rpmlint

PACKAGE APPROVED.

Comment 7 Gwyn Ciesla 2022-09-06 15:16:25 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/mpv

Comment 8 Fedora Update System 2022-09-06 17:21:49 UTC
FEDORA-2022-2cd1e85046 has been submitted as an update to Fedora 37. https://bodhi.fedoraproject.org/updates/FEDORA-2022-2cd1e85046

Comment 9 Fedora Update System 2022-09-06 20:30:50 UTC
FEDORA-2022-2cd1e85046 has been pushed to the Fedora 37 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2022-2cd1e85046 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-2cd1e85046

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 10 Fedora Update System 2022-09-14 00:20:15 UTC
FEDORA-2022-2cd1e85046 has been pushed to the Fedora 37 stable repository.
If problem still persists, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.