Bug 2124244

Summary:	Review Request: mpv - Movie player playing most video formats and DVDs
Product:	[Fedora] Fedora	Reporter:	Vitaly <vitaly>
Component:	Package Review	Assignee:	Neal Gompa <ngompa13>
Status:	CLOSED ERRATA	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	medium	Docs Contact:
Priority:	medium
Version:	rawhide	CC:	ngompa13, package-review
Target Milestone:	---	Flags:	ngompa13: fedora-review+
Target Release:	---
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:		Doc Type:	If docs needed, set a value
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2022-09-14 00:20:15 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:

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 1 Vitaly 2022-09-05 12:12:23 UTC

Fixed rpmlint warnings.

Spec URL: https://xvitaly.fedorapeople.org/for-review/mpv.spec
SRPM URL: https://xvitaly.fedorapeople.org/for-review/mpv-0.34.1-11.fc36.src.rpm

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 5 Vitaly 2022-09-05 15:08:40 UTC

Spec URL: https://xvitaly.fedorapeople.org/for-review/mpv.spec
SRPM URL: https://xvitaly.fedorapeople.org/for-review/mpv-0.34.1-11.fc36.src.rpm

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.