https://bugzilla.redhat.com/show_bug.cgi?id=1535549
Bug ID: 1535549 Summary: Review Request: mupen64plus - Nintendo 64 Emulator Product: Fedora Version: rawhide Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: wberrier@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: http://berrier.org/tmp/mupen64plus.spec SRPM URL: http://berrier.org/tmp/mupen64plus-2.5-2.fc27.src.rpm Description: Nintendo 64 Emulator Fedora Account System Username: wberrier
https://bugzilla.redhat.com/show_bug.cgi?id=1535549
Robert-André Mauchin zebob.m@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |zebob.m@gmail.com
--- Comment #1 from Robert-André Mauchin zebob.m@gmail.com --- - Not needed in Fedora:
Group:
BuildRoot:
%defattr(***, root, root) in %files
- .desktop files must be validated in %install or %check. See https://fedoraproject.org/wiki/Packaging:Guidelines#desktop-file-install_usa...
desktop-file-validate %{buildroot}/%{_datadir}/applications/mupen64plus.desktop
- Since you are installing icons in hicolor, you should Requires: hicolor-icon-theme
- In the -devel subpackage, you should provide an unversioned shared library file linking to libmupen64plus.so.2.0.0. Just create the symlink in %install
ln -sf %{_libdir}/libmupen64plus.so.2.0.0 %{buildroot}%{_libdir}/libmupen64plus.so
And include it in -devel %files:
%files devel %{_includedir}/mupen64plus/ %{_libdir}/libmupen64plus.so
- There's various LICENSES files included in the source, add them all to %license. In order not to overwrite the same file, you need to rename them first.
In %prep:
cp -a source/mupen64plus-rsp-hle/LICENSES LICENSE-rsp-hle cp -a source/mupen64plus-rom/mupen64plus/assets/LICENSES LICENSE-assets cp -a source/mupen64plus-rom/LICENSES LICENSE-rom cp -a source/mupen64plus-input-sdl/LICENSES LICENSE-input-sdl cp -a source/mupen64plus-video-glide64mk2/LICENSES LICENSE-video-glide64mk2 cp -a source/mupen64plus-video-rice/LICENSES LICENSE-video-rice cp -a source/mupen64plus-ui-console/LICENSES LICENSE-ui-console cp -a source/mupen64plus-core/LICENSES LICENSE-core cp -a source/mupen64plus-audio-sdl/LICENSES LICENSE-audio-sdl
In %files:
%files %license LICENSE-rsp-hle LICENSE-assets LICENSE-rom LICENSE-input-sdl LICENSE-video-glide64mk2 LICENSE-video-rice LICENSE-core LICENSE-audio-sdl
- The assets in source/mupen64plus-rom/mupen64plus/assets/ are actually Creative Common Attribution-ShareAlike 3.0, so you should add that license to the License: field:
License: GPLv2+ and CC-BY-SA
- Since you're including a shared library, you must run ldconfig in %post and %postun:
%post -p /sbin/ldconfig
%postun -p /sbin/ldconfig
See https://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries
- You should own this directory: /usr/lib64/mupen64plus :
%dir %{_libdir}/%{name}
- In the %changelog, put a space before the version info, otherwise it's not recognized correctly:
* Thu Jan 11 2018 Wade Berrier wberrier@gmail.com - 2.5-2
- Use pkgconfig for your devel deps when you can:
BuildRequires: pkgconfig(SDL_ttf) BuildRequires: pkgconfig(lirc) BuildRequires: desktop-file-utils BuildRequires: pkgconfig(glu) BuildRequires: pkgconfig(samplerate) BuildRequires: pkgconfig(libpng) BuildRequires: pkgconfig(sdl) BuildRequires: pkgconfig(freetype2) BuildRequires: boost-devel BuildRequires: gzip BuildRequires: pkgconfig(glew) BuildRequires: binutils
Package Review ==============
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed
Issues: ======= - ldconfig called in %post and %postun if required. Note: /sbin/ldconfig not called in mupen64plus See: http://fedoraproject.org/wiki/Packaging/Guidelines#Shared_Libraries - Package installs a %{name}.desktop using desktop-file-install or desktop- file-validate if there is such a file.
===== MUST items =====
C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [!]: Development (unversioned) .so files in -devel subpackage, if present. Note: Unversioned so-files in private %_libdir subdirectory (see attachment). Verify they are not in ld path. [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs.
Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "GPL", "GPL (v2 or later)", "Unknown or generated", "MIT/X11 (BSD like)", "zlib/libpng", "BSD (2 clause)", "GPL (v2 or later) (with incorrect FSF address)", "GPL (with incorrect FSF address)", "LGPL (v2.1 or later)", "BSD (3 clause) GPL (v2 or later)", "GPL (v2)". 119 files have unknown license. Detailed output of licensecheck in /home/bob/packaging/review/mupen64plus/review- mupen64plus/licensecheck.txt [!]: License file installed when any subpackage combination is installed. [!]: Package requires other packages for directories it uses. Note: No known owner of /usr/lib64/mupen64plus [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/share/icons/hicolor/48x48, /usr/share/icons/hicolor/48x48/apps, /usr/lib64/mupen64plus, /usr/share/icons/hicolor/scalable/apps, /usr/share/icons/hicolor, /usr/share/icons/hicolor/scalable [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [!]: Each %files section contains %defattr if rpm < 4.4 Note: %defattr present but not needed [x]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [!]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: Package does not own files or directories owned by other packages. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Package contains desktop file if it is a GUI application. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 0 bytes in 0 files. [x]: Packages must not store files under /srv, /opt or /usr/local
===== SHOULD items =====
Generic: [!]: Buildroot is not present Note: Buildroot: present but not needed [-]: Avoid bundling fonts in non-fonts packages. Note: Package contains font files [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: Package should compile and build into binary rpms on all supported architectures. [-]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [!]: Spec use %global instead of %define unless justified. Note: %define requiring justification: %define debug_package %{nil} [x]: Reviewer should test that the package builds in mock. [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Fully versioned dependency in subpackages if applicable. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL.
===== EXTRA items =====
Generic: [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. Note: Arch-ed rpms have a total of 2007040 bytes in /usr/share [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Spec file according to URL is the same as in SRPM.
Rpmlint ------- Checking: mupen64plus-2.5-2.fc28.x86_64.rpm mupen64plus-devel-2.5-2.fc28.x86_64.rpm mupen64plus-2.5-2.fc28.src.rpm mupen64plus.x86_64: E: explicit-lib-dependency libpng mupen64plus.x86_64: E: explicit-lib-dependency libsamplerate mupen64plus.x86_64: W: incoherent-version-in-changelog -2.5-2 ['2.5-2.fc28', '2.5-2'] mupen64plus.x86_64: E: library-without-ldconfig-postin /usr/lib64/libmupen64plus.so.2.0.0 mupen64plus.x86_64: E: library-without-ldconfig-postun /usr/lib64/libmupen64plus.so.2.0.0 mupen64plus.x86_64: E: script-without-shebang /usr/share/icons/hicolor/scalable/apps/mupen64plus.svg mupen64plus.x86_64: E: script-without-shebang /usr/share/licenses/mupen64plus/LICENSE-assets mupen64plus.x86_64: E: script-without-shebang /usr/share/licenses/mupen64plus/LICENSE-audio-sdl mupen64plus.x86_64: E: incorrect-fsf-address /usr/share/licenses/mupen64plus/LICENSE-audio-sdl mupen64plus.x86_64: E: script-without-shebang /usr/share/licenses/mupen64plus/LICENSE-core mupen64plus.x86_64: E: script-without-shebang /usr/share/licenses/mupen64plus/LICENSE-input-sdl mupen64plus.x86_64: E: incorrect-fsf-address /usr/share/licenses/mupen64plus/LICENSE-input-sdl mupen64plus.x86_64: E: script-without-shebang /usr/share/licenses/mupen64plus/LICENSE-rom mupen64plus.x86_64: E: script-without-shebang /usr/share/licenses/mupen64plus/LICENSE-rsp-hle mupen64plus.x86_64: E: incorrect-fsf-address /usr/share/licenses/mupen64plus/LICENSE-rsp-hle mupen64plus.x86_64: E: script-without-shebang /usr/share/licenses/mupen64plus/LICENSE-video-glide64mk2 mupen64plus.x86_64: E: incorrect-fsf-address /usr/share/licenses/mupen64plus/LICENSE-video-glide64mk2 mupen64plus.x86_64: E: script-without-shebang /usr/share/licenses/mupen64plus/LICENSE-video-rice mupen64plus.x86_64: E: incorrect-fsf-address /usr/share/licenses/mupen64plus/LICENSE-video-rice mupen64plus.x86_64: W: spurious-executable-perm /usr/share/man/man6/mupen64plus.6.gz mupen64plus.x86_64: E: script-without-shebang /usr/share/mupen64plus/Glide64mk2.ini mupen64plus.x86_64: E: script-without-shebang /usr/share/mupen64plus/InputAutoCfg.ini mupen64plus.x86_64: E: script-without-shebang /usr/share/mupen64plus/RiceVideoLinux.ini mupen64plus.x86_64: E: script-without-shebang /usr/share/mupen64plus/mupen64plus.ini mupen64plus.x86_64: E: script-without-shebang /usr/share/mupen64plus/mupencheat.txt mupen64plus-devel.x86_64: W: no-documentation 3 packages and 0 specfiles checked; 23 errors, 3 warnings.
https://bugzilla.redhat.com/show_bug.cgi?id=1535549
Robert-André Mauchin zebob.m@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841 (FE-NEEDSPONSOR)
--- Comment #2 from Robert-André Mauchin zebob.m@gmail.com --- I don't see you in the packager group, you probably need to find a sponsor. See https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group for more info. Introduce yourself on the fedora-devel mailing list is also a good idea. You should mention that you are already a packager for RPMFusion or ask your sponsor there to also sponsor you for Fedora.
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1535549
Antonio Trande anto.trande@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |anto.trande@gmail.com Flags| |fedora-review?
--- Comment #3 from Antonio Trande anto.trande@gmail.com --- @Robert-André
Please, let me care this package; i have followed it from rpmfusion.
@Wade
Fix all issues pointed by Robert-André, then we can go on with the review.
https://bugzilla.redhat.com/show_bug.cgi?id=1535549
--- Comment #4 from Robert-André Mauchin zebob.m@gmail.com --- As you wish Antonio.
https://bugzilla.redhat.com/show_bug.cgi?id=1535549
--- Comment #5 from Wade Berrier wberrier@gmail.com --- Robert-André, thanks for the comprehensive review.
I believe I've made all of the requested changes, and have new updates here:
Spec URL: http://berrier.org/tmp/mupen64plus.spec SRPM URL: http://berrier.org/tmp/mupen64plus-2.5-3.fc27.src.rpm
I also did some additional cleanups with BuildRequires and Requires, and did a test mock build.
Question: I changed "%define debug_package" to use "global", is that correct? What do I need to do about a debuginfo package?
NOTE: rpmlint is warning about libsamplerate and libpng, but "rpm -qp --requires" doesn't list those dependencies, where as ldd /usr/lib64/mupen64plus/* does show those dependencies. So, I left those Requires in. Not sure what to do about that.
https://bugzilla.redhat.com/show_bug.cgi?id=1535549
--- Comment #6 from Robert-André Mauchin zebob.m@gmail.com --- You shouldn't patch the FSF address in the LICENSES files. It is allowed when it's just in the headers of a source file, but not for license files. See: https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address
https://bugzilla.redhat.com/show_bug.cgi?id=1535549
--- Comment #7 from Robert-André Mauchin zebob.m@gmail.com ---
Question: I changed "%define debug_package" to use "global", is that correct?
Yes
What do I need to do about a debuginfo package?
Indeed, the best would be to generate the debug infos but whatever I try, it still fails.
Anyhow you still should enable Fedora's default flags for the build:
%build export CFLAGS="%{optflags}" export CXXFLAGS="%{optflags}" export LDFLAGS="%{?__global_ldflags}" sh m64p_build.sh LIRC=1
- Another issue is that the build script install the libraries without the executable bits. I thought it was what's causing the debug info not to be generated (binaries must be executable for find-debuginfo to work) but that's still not it. Still set the executable bits for those:
%install
# NOTE: set LDCONFIG to true so it's not run during this script ./m64p_install.sh DESTDIR=%{buildroot} PREFIX=%{_prefix} MANDIR=%{_mandir} LIBDIR=%{_libdir} LDCONFIG='true' find %{buildroot}%{_libdir} -type f -name "*.so*" -exec chmod 0755 "{}" ;
- You don't need the following Requires:
Requires: SDL_ttf Requires: mesa-libGLU Requires: libsamplerate Requires: libpng Requires: freetype Requires: boost
They are correctly detected automatically, see:
$ rpm -q --requires -p mupen64plus-2.5-3.fc28.x86_64.rpm | sort -f | uniq -c 2 /sbin/ldconfig 1 hicolor-icon-theme 1 libboost_filesystem.so.1.64.0()(64bit) 1 libboost_system.so.1.64.0()(64bit) 1 libc.so.6()(64bit) 1 libc.so.6(GLIBC_2.11)(64bit) 1 libc.so.6(GLIBC_2.14)(64bit) 1 libc.so.6(GLIBC_2.2.5)(64bit) 1 libc.so.6(GLIBC_2.3)(64bit) 1 libc.so.6(GLIBC_2.3.4)(64bit) 1 libc.so.6(GLIBC_2.4)(64bit) 1 libc.so.6(GLIBC_2.7)(64bit) 1 libdl.so.2()(64bit) 1 libdl.so.2(GLIBC_2.2.5)(64bit) 1 libfreetype.so.6()(64bit) 1 libgcc_s.so.1()(64bit) 1 libgcc_s.so.1(GCC_3.0)(64bit) 1 libgcc_s.so.1(GCC_3.3.1)(64bit) 1 libGL.so.1()(64bit) 1 libGLU.so.1()(64bit) 1 liblirc_client.so.0()(64bit) 1 libm.so.6()(64bit) 1 libm.so.6(GLIBC_2.15)(64bit) 1 libm.so.6(GLIBC_2.2.5)(64bit) 1 libpng16.so.16()(64bit) 1 libpng16.so.16(PNG16_0)(64bit) 1 libpthread.so.0()(64bit) 1 libsamplerate.so.0()(64bit) 1 libsamplerate.so.0(libsamplerate.so.0.0)(64bit) 1 libsamplerate.so.0(libsamplerate.so.0.1)(64bit) 1 libSDL2-2.0.so.0()(64bit) 1 libstdc++.so.6()(64bit) 1 libstdc++.so.6(CXXABI_1.3)(64bit) 1 libstdc++.so.6(CXXABI_1.3.8)(64bit) 1 libstdc++.so.6(CXXABI_1.3.9)(64bit) 1 libstdc++.so.6(GLIBCXX_3.4)(64bit) 1 libstdc++.so.6(GLIBCXX_3.4.11)(64bit) 1 libstdc++.so.6(GLIBCXX_3.4.15)(64bit) 1 libstdc++.so.6(GLIBCXX_3.4.9)(64bit) 1 libz.so.1()(64bit) 1 rpmlib(CompressedFileNames) <= 3.0.4-1 1 rpmlib(FileDigests) <= 4.6.0-1 1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1 1 rpmlib(PayloadIsXz) <= 5.2-1 1 rtld(GNU_HASH)
https://bugzilla.redhat.com/show_bug.cgi?id=1535549
--- Comment #8 from Robert-André Mauchin zebob.m@gmail.com --- Ah I got it! I noticed there is a debug option in the Makefile:
Debugging Options:" @echo " PROFILE=1 == build gprof instrumentation into binaries for profiling" @echo " DEBUG=1 == add debugging symbols to binaries"
Of course I tried to add it to the build line, with no success. It's not needed there anyway since it's already taken care of by the default Fedora %optflags.
However at install time, this debug option enables or disables a "-s" flag for install. And what does "install -s" do? It strips the binaries, thus preventing find-debuginfo to find the debugging symbols.
TL;DR: add DEBUG=1 to the install line:
# NOTE: set LDCONFIG to true so it's not run during this script ./m64p_install.sh DESTDIR=%{buildroot} PREFIX=%{_prefix} MANDIR=%{_mandir} LIBDIR=%{_libdir} DEBUG=1 LDCONFIG='true
And voilà the debug packages are built.
https://bugzilla.redhat.com/show_bug.cgi?id=1535549
Antonio Trande anto.trande@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |NEW Assignee|anto.trande@gmail.com |nobody@fedoraproject.org Flags|fedora-review? |
--- Comment #9 from Antonio Trande anto.trande@gmail.com --- I'm leaving this package to Robert-André, who cannot do without comment this ticket.
https://bugzilla.redhat.com/show_bug.cgi?id=1535549
--- Comment #10 from Wade Berrier wberrier@gmail.com --- Ok, I've made the additional changes and posted them here:
http://berrier.org/tmp/mupen64plus.spec http://berrier.org/tmp/mupen64plus-2.5-4.fc27.src.rpm
(In reply to Robert-André Mauchin from comment #6)
You shouldn't patch the FSF address in the LICENSES files. It is allowed when it's just in the headers of a source file, but not for license files. See: https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address
So, what to do about this? In the meantime I've removed the patch.
https://bugzilla.redhat.com/show_bug.cgi?id=1535549
Robert-André Mauchin zebob.m@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |POST Assignee|nobody@fedoraproject.org |zebob.m@gmail.com Flags| |fedora-review+
--- Comment #11 from Robert-André Mauchin zebob.m@gmail.com ---
So, what to do about this?
Just notify upstream about it.
Package is approved.
https://bugzilla.redhat.com/show_bug.cgi?id=1535549
--- Comment #12 from Wade Berrier wberrier@gmail.com --- Upstream notified about the incorrect addresses in the license files:
https://github.com/mupen64plus/mupen64plus-core/issues/527
Git repo requests:
f27:
https://pagure.io/releng/fedora-scm-requests/issue/4359
f26:
https://pagure.io/releng/fedora-scm-requests/issue/4360
I've been following the instructions at https://fedoraproject.org/wiki/Package_Review_Process
Anything else other than what's in there that I need to do?
https://bugzilla.redhat.com/show_bug.cgi?id=1535549
--- Comment #13 from Wade Berrier wberrier@gmail.com --- I guess I need a sponsor?
My pagure.io requests were closed.
https://bugzilla.redhat.com/show_bug.cgi?id=1535549
Robert-André Mauchin zebob.m@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |ASSIGNED
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com