Bug 2256940

This package built on koji:  https://koji.fedoraproject.org/koji/taskinfo?taskID=111330160

Copr build:
https://copr.fedorainfracloud.org/coprs/build/6866211
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2256940-smplayer/fedora-rawhide-x86_64/06866211-smplayer/fedora-review/review.txt

Found issues:

- Not a valid SPDX expression 'GPLv2+'. It seems that you are using the old Fedora license abbreviations. Try `license-fedora2spdx' for converting it to SPDX.
  Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Spec URL: https://sergiomb.fedorapeople.org/smplayer/smplayer.spec
SRPM URL: https://sergiomb.fedorapeople.org/smplayer/smplayer-23.12.0-2.fc40.src.rpm

Migrated to SPDX license

Created attachment 2007370 [details]
The .spec file difference from Copr build 6866211 to 6866645

Copr build:
https://copr.fedorainfracloud.org/coprs/build/6866645
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2256940-smplayer/fedora-rawhide-x86_64/06866645-smplayer/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Spec URL: https://sergiomb.fedorapeople.org/smplayer/smplayer.spec
SRPM URL: https://sergiomb.fedorapeople.org/smplayer/smplayer-23.12.0-2.fc40.src.rpm

Change the themes subpackage to noarch

Created attachment 2007379 [details]
The .spec file difference from Copr build 6866645 to 6866979

Copr build:
https://copr.fedorainfracloud.org/coprs/build/6866979
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2256940-smplayer/fedora-rawhide-x86_64/06866979-smplayer/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Taking this review.

Initial spec review:

> %if 0%{?rhel} >= 9
> %bcond_with system_qtsingleapplication
> %else
> %bcond_without system_qtsingleapplication
> %endif

QtSingleApplication is in EPEL 9, so this conditional must be dropped.

> # smplayer without mplayer is quite useless
> %if 0%{?fedora} || 0%{?rhel} > 7
> Recommends:     smtube
> Requires:       mplayer-backend
> Suggests:       mpv
> %else
> Requires:       mplayer
> %endif

This conditional should be dropped since the fedora || rhel > 7 conditional is always true in Fedora.

> %if 0%{?fedora}
> # we only have yt-dlp on fedora
> # it is used by the playlist
> Requires:       yt-dlp
> %endif

yt-dlp is present in EPEL 9.

> %{?kf5_kinit_requires}

Why is this here? This doesn't depend on KF5 from what I can see.

> Requires(post): desktop-file-utils
> Requires(postun): desktop-file-utils
> [...]
> %if (0%{?rhel} && 0%{?rhel} <= 7)
> %post
> /usr/bin/update-desktop-database &> /dev/null || :
> /bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null || :
> 
> %postun
> /usr/bin/update-desktop-database &> /dev/null || :
> if [ $1 -eq 0 ] ; then
>     /bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null
>     /usr/bin/gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || :
> fi
> 
> %posttrans
> /usr/bin/gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || :
> %endif

This package is only valid in EPEL 9 and Fedora, which doesn't need any of this. Drop it.

> Requires: smplayer

This should be "Requires: %{name} = %{version}-%{release}".

> %setup -q -a3 -a4
> [...]
> %patch -P0 -p1 -b .desktop-files
> %patch -P1 -p1 -b .qtsingleapplication

This should be swapped for "%autosetup -p1 -a3 -a4".

> %if 0%{?fedora}
>     sed -i 's/DEFINES += YT_CODEDOWNLOADER/DEFINES -= YT_CODEDOWNLOADER/' smplayer.pro
> %endif

This works on EPEL 9 too. Drop the conditional.

> %if (0%{?rhel} && 0%{?rhel} <= 7)
> mkdir -p %{buildroot}/usr/share/appdata/
> mv %{buildroot}/usr/share/metainfo/%{name}.appdata.xml %{buildroot}%{_metainfodir}/%{name}.appdata.xml
> %endif

We're not building for RHEL 7 here, drop it.

> #https://bugzilla.redhat.com/show_bug.cgi?id=1830923
> %if (0%{?rhel} == 0)
> appstream-util validate-relax --nonet %{buildroot}%{_metainfodir}/%{name}.appdata.xml
> %endif

This bug is not valid on RHEL 9, so this conditional can be dropped and metainfo validation must be done unconditionally.

# autosetup doesn't work with 2 sources                                                                                                                      
# https://github.com/rpm-software-management/rpm/issues/1204     

and why not support epel 8 and epel 7 ? 

I'm fixing the others 

%{?kf5_kinit_requires} is an old hack, initially for kde 4 (https://github.com/smplayer-dev/smplayer/blob/f2270fed4dfbbe3051b7cb228f70a6cfe22dce16/smplayer.spec#L43) , I'm not sure if it is needed when is just gnome installed ,

Spec URL: https://sergiomb.fedorapeople.org/smplayer/smplayer.spec
SRPM URL: https://sergiomb.fedorapeople.org/smplayer/smplayer-23.12.0-2.fc40.src.rpm

enable system_qtsingleapplication and yt-dlp on epel 9 

-%{?kf5_kinit_requires}

-Requires: smplayer
+Requires: %{name} = %{version}-%{release}

-%if (0%{?rhel} == 0)
+%if 0%{?fedora} || 0%{?rhel} > 8
appstream-util validate-relax --nonet %{buildroot}%{_metainfodir}/%{name}.appdata.xml

I'd like keep epel 8 and epel 7 support, I'd like keep builds of smplayer on that releases.

Created attachment 2008175 [details]
The .spec file difference from Copr build 6866979 to 6880716

Copr build:
https://copr.fedorainfracloud.org/coprs/build/6880716
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2256940-smplayer/fedora-rawhide-x86_64/06880716-smplayer/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

(In reply to Sergio Basto from comment #12)
>
> -%if (0%{?rhel} == 0)
> +%if 0%{?fedora} || 0%{?rhel} > 8
> appstream-util validate-relax --nonet
> %{buildroot}%{_metainfodir}/%{name}.appdata.xml
> 
> I'd like keep epel 8 and epel 7 support, I'd like keep builds of smplayer on
> that releases.

Neither of those targets are possible in Fedora, though, and it adds complexity for no benefit. Especially for EPEL 7 which is EOL in six months.

(In reply to Sergio Basto from comment #11)
> # autosetup doesn't work with 2 sources                                     
> 
> # https://github.com/rpm-software-management/rpm/issues/1204     
> 

You can use "%autopatch -p1" after "%setup" then.

smplayer is to add to epel9 , epel8 and epel7 , as is RPMFusion , I just saw that mpv is not available on epel 8 , but is epel 9 

we can't use %autopatch because one of the patches [1] is just applied if we build with system_qtsingleapplication 

%if %{with system_qtsingleapplication}
%patch -P1 -p1 -b .qtsingleapplication
%endif

Spec URL: https://sergiomb.fedorapeople.org/smplayer/smplayer.spec
SRPM URL: https://sergiomb.fedorapeople.org/smplayer/smplayer-23.12.0-4.fc40.src.rpm


Removed Requires : mplayer

Removed support to epel-7 and epel-8 because we don't have mpv in that repos

There seems to be some problem with the following file.
SRPM URL: https://sergiomb.fedorapeople.org/smplayer/smplayer-23.12.0-4.fc40.src.rpm
Fetching it results in a 404 Not Found error.
Please make sure the URL is correct and publicly available.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Spec URL: https://sergiomb.fedorapeople.org/smplayer/smplayer.spec
SRPM URL: https://sergiomb.fedorapeople.org/smplayer/smplayer-23.12.0-4.fc41.src.rpm

[fedora-review-service-build]

[fedora-review-service-build]

Created attachment 2030300 [details]
The .spec file difference from Copr build 6880716 to 7393742

Copr build:
https://copr.fedorainfracloud.org/coprs/build/7393742
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2256940-smplayer/fedora-rawhide-x86_64/07393742-smplayer/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Review notes:

* Packaging complies with the guidelines
* Package builds and installs
* No serious issues from rpmlint
* Licensing is correct and license files are correctly installed

PACKAGE APPROVED.

The Pagure repository was created at https://src.fedoraproject.org/rpms/smplayer

FEDORA-2024-51f649032a (smplayer-23.12.0-4.fc40) has been submitted as an update to Fedora 40.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-51f649032a

FEDORA-2024-06a997a645 (smplayer-23.12.0-4.fc39) has been submitted as an update to Fedora 39.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-06a997a645

FEDORA-EPEL-2024-b2bf9b752c (smplayer-23.12.0-4.el9) has been submitted as an update to Fedora EPEL 9.
https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2024-b2bf9b752c

FEDORA-2024-51f649032a has been pushed to the Fedora 40 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2024-51f649032a \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-51f649032a

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

FEDORA-EPEL-2024-b2bf9b752c has been pushed to the Fedora EPEL 9 testing repository.

You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2024-b2bf9b752c

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

FEDORA-2024-06a997a645 has been pushed to the Fedora 39 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2024-06a997a645 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-06a997a645

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

FEDORA-2024-51f649032a (smplayer-23.12.0-4.fc40) has been pushed to the Fedora 40 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-2024-06a997a645 (smplayer-23.12.0-4.fc39) has been pushed to the Fedora 39 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-EPEL-2024-b2bf9b752c (smplayer-23.12.0-4.el9) has been pushed to the Fedora EPEL 9 stable repository.
If problem still persists, please make note of it in this bug report.