Spec URL: https://yselkowitz.fedorapeople.org/vlc.spec SRPM URL: https://yselkowitz.fedorapeople.org/vlc-3.0.20-3.fc40.src.rpm Description: VLC media player is a highly portable multimedia player and multimedia framework capable of reading most audio and video formats as well as DVDs, Audio CDs VCDs, and various streaming protocols. It can also be used as a media converter or a server to stream in uni-cast or multi-cast in IPv4 or IPv6 on networks. Fedora Account System Username: yselkowitz
Taking this review.
Copr build: https://copr.fedorainfracloud.org/coprs/build/6751370 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2254460-vlc/fedora-rawhide-x86_64/06751370-vlc/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.
Initial spec review: > Release: 3%{?dist} You're using %autochangelog in %changelog, so you'll want to use %autorelease here too. > BuildRequires: lua5.1-devel, lua5.1 > [...] > sed -i -e 's/luac/luac-5.1/g' configure.ac This should not be necessary. VLC 3.x is compatible with Lua 5.4: https://code.videolan.org/videolan/vlc/-/merge_requests/2557 > %package plugin-gstreamer > Summary: VLC media player GStreamer codec plugin > Requires: %{name}-libs%{?_isa} = %{epoch}:%{version}-%{release} > Requires: %{name}-plugins-base%{?_isa} = %{epoch}:%{version}-%{release} > Requires: gstreamer1-plugins-good%{?_isa} > Requires: gstreamer1-plugins-bad-free%{?_isa} > Recommends: gstreamer1-plugin-libav%{?_isa} Can you also add "Recommends: gstreamer1-plugin-openh264%{?_isa}" here? > # GNOME 2 script, not compatible with GNOME 3+ rm -f %{buildroot}%{_datadir}/vlc/utils/gnome-vlc-default.sh Would this be used by MATE, since it's a GNOME 2 descendent? > %transfiletriggerin libs -- %{vlc_plugindir} %{_libdir}/vlc/vlc-cache-gen %{vlc_plugindir} &>/dev/null || : > > %transfiletriggerpostun libs -- %{vlc_plugindir} > %{_libdir}/vlc/vlc-cache-gen %{vlc_plugindir} &>/dev/null || : Do we need a posttrans for vlc-libs itself so that it runs on initial install? Or does this fire even when you initially install vlc?
(In reply to Neal Gompa from comment #3) > You're using %autochangelog in %changelog, so you'll want to use > %autorelease here too. This needs to be -3 to be an upgrade over the rpmfusion build, so iiuc that will have to wait until 3.0.21, or we'd need some bogus commits to force it to -3 or higher. > This should not be necessary. VLC 3.x is compatible with Lua 5.4: > https://code.videolan.org/videolan/vlc/-/merge_requests/2557 Confirmed; adding to next draft. > Can you also add "Recommends: gstreamer1-plugin-openh264%{?_isa}" here? Makes sense; adding to next draft. > > # GNOME 2 script, not compatible with GNOME 3+ > Would this be used by MATE, since it's a GNOME 2 descendent? Not unless MATE suddenly regressed to use GConf2 and /desktop/gnome namespace therein. > Do we need a posttrans for vlc-libs itself so that it runs on initial > install? Or does this fire even when you initially install vlc? vlc-libs, which is separate primarily so that vlc-devel doesn't pull in a bunch of plugin dependencies, has no need for the plugins.dat, and vlc-cache-gen errors out if no plugins are found. The trigger does fire on the initial installation, even of just vlc-plugins-base.
Created attachment 2004225 [details] The .spec file difference from Copr build 6751370 to 6751533
Copr build: https://copr.fedorainfracloud.org/coprs/build/6751533 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2254460-vlc/fedora-rawhide-x86_64/06751533-vlc/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 Yaakov Selkowitz from comment #4) > (In reply to Neal Gompa from comment #3) > > You're using %autochangelog in %changelog, so you'll want to use > > %autorelease here too. > > This needs to be -3 to be an upgrade over the rpmfusion build, so iiuc that > will have to wait until 3.0.21, or we'd need some bogus commits to force it > to -3 or higher. > Then let's not use autochangelog either until we actually can do that. But this can be fixed on import.
> rm -f aclocal.m4 m4/lib*.m4 m4/lt*.m4 > ./bootstrap Please move this to the %build stage so that "fedpkg prep" will work properly.
(In reply to Neal Gompa from comment #8) > Then let's not use autochangelog either until we actually can do that. But > this can be fixed on import. I'm not aware of anything saying that one cannot be used without the other, but I discovered %autorelease -b 3 accomplishes what is needed, so I've switched to that locally. (In reply to Neal Gompa from comment #9) > Please move this to the %build stage so that "fedpkg prep" will work > properly. By definition, fedpkg prep relies on the system to run any commands, so you would need bison, prep, gettext-devel, and libtool. While there may be some debate on where autoreconf belongs, it is quite common to be in %prep rather than %build, and I see nothing in the packaging guidelines to the contrary.
(In reply to Yaakov Selkowitz from comment #10) > (In reply to Neal Gompa from comment #8) > > Then let's not use autochangelog either until we actually can do that. But > > this can be fixed on import. > > I'm not aware of anything saying that one cannot be used without the other, > but I discovered %autorelease -b 3 accomplishes what is needed, so I've > switched to that locally. > > (In reply to Neal Gompa from comment #9) > > Please move this to the %build stage so that "fedpkg prep" will work > > properly. > > By definition, fedpkg prep relies on the system to run any commands, so you > would need bison, prep, gettext-devel, and libtool. While there may be some > debate on where autoreconf belongs, it is quite common to be in %prep rather > than %build, and I see nothing in the packaging guidelines to the contrary. *technically* the FPC has banned running thinkgs like %configure in %prep: https://pagure.io/packaging-committee/issue/1159 Not sure why this ticket was closed without actually making any changes to the guidelines.
(In reply to Fabio Valentini from comment #11) > *technically* the FPC has banned running thinkgs like %configure in %prep: > https://pagure.io/packaging-committee/issue/1159 > > Not sure why this ticket was closed without actually making any changes to > the guidelines. %configure != autoreconf though.
For the discussion whether they belong in %prep or not, they're *quite* similar. And IMO, both belong in %build.
The comments in that ticket make clear that no guidelines have been set wrt autoreconf, and that attempting to do so would involve much debate. There are many packages which go one way or the other, so there is clearly no consensus. As such, neither way can be viewed as incorrect.
Review notes: * Package follows Fedora Packaging Guidelines * Package builds and installs * Package licensing is correctly handled * No serious issues from rpmlint Note: this was a bit more effort to evaluate because %load broke fedora-review. PACKAGE APPROVED.
The Pagure repository was created at https://src.fedoraproject.org/rpms/vlc
FEDORA-2023-f346d56ebc has been submitted as an update to Fedora 40. https://bodhi.fedoraproject.org/updates/FEDORA-2023-f346d56ebc
FEDORA-2023-f6b280748b has been submitted as an update to Fedora 39. https://bodhi.fedoraproject.org/updates/FEDORA-2023-f6b280748b
FEDORA-2023-1947ad7443 has been submitted as an update to Fedora 38. https://bodhi.fedoraproject.org/updates/FEDORA-2023-1947ad7443
FEDORA-EPEL-2023-b5f125688c has been submitted as an update to Fedora EPEL 9. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2023-b5f125688c
FEDORA-2023-f346d56ebc has been pushed to the Fedora 40 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2023-f6b280748b 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-2023-f6b280748b \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2023-f6b280748b See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2023-1947ad7443 has been pushed to the Fedora 38 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2023-1947ad7443 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2023-1947ad7443 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-EPEL-2023-b5f125688c 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-2023-b5f125688c See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2023-1947ad7443 has been pushed to the Fedora 38 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2023-f6b280748b has been pushed to the Fedora 39 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-EPEL-2023-b5f125688c has been pushed to the Fedora EPEL 9 stable repository. If problem still persists, please make note of it in this bug report.