| Summary: | Review Request: pcsx2 - Playstation 2 Emulator | ||
|---|---|---|---|
| Product: | Package Reviews | Reporter: | Artem <ego.cordatus> |
| Component: | Review Request | Assignee: | t-rpmfusion |
| Status: | RESOLVED FIXED | ||
| Severity: | enhancement | CC: | leigh123linux, rpmfusion-package-review, sergio, t-rpmfusion |
| Priority: | P1 | Flags: | t-rpmfusion:
fedora-review+
|
| Version: | Current | ||
| Hardware: | i386 | ||
| OS: | GNU/Linux | ||
| namespace: | nonfree | ||
| Attachments: | New PCSX2 build | ||
|
Description
Artem
2020-02-05 18:11:13 CET
Doesn't look to bad already! Few questions/remarks: - I don't think including CONTRIBUTING.md in %doc makes sense from a distribution's standpoint - Why remove locale/ar_SA/? - It looks like you are not including the copyrighted ROM, correct? PS: would you be interested in a review swap with Bug 5522 (it's OK to say no)? There is already a pcsx2 package in fedora. Can you submit a diff against the fedora spec file as a mean for reviewing your changes ? (and forward this change to the current pcsx2 maintainer in a dedicated bug ?) Thx in advance. I've found https://src.fedoraproject.org/rpms/libretro-pcsx-rearmed (which looks to be ARM focused) and https://src.fedoraproject.org/rpms/pcsxr (a different fork), but no pcsx2. Are you referring to one of the above, or is there another one? (In reply to t-rpmfusion from comment #1) > - I don't think including CONTRIBUTING.md in %doc makes sense from a > distribution's standpoint Agree. It may sense to include this file in some -devel sub packages maybe but in PCSX2 it not needed. > - Why remove locale/ar_SA/? To make rpm linter happier a little bit, but maybe it is linter itself so need to investigate more this... > - It looks like you are not including the copyrighted ROM, correct? Yep. Including it would be awesome indeed but this look like illegal and PCSX2 FAQ clearly says this: https://pcsx2.net/support/links/faq.html#S2Q11 > PS: would you be interested in a review swap with Bug 5522 (it's OK to say > no)? LGTM. I'll do 'fedora-review' and try to make review... :) Created attachment 2138 [details] New PCSX2 build (In reply to Nicolas Chauvet from comment #2) > There is already a pcsx2 package in fedora. > Can you submit a diff against the fedora spec file as a mean for reviewing > your changes ? Attached patch. Also i found now one interesting thing in old package which was already in RPM Fusion. I'll try it myself now and also add all old change log for sure and credits to old maintainer... (In reply to t-rpmfusion from comment #3) ... > Are you referring to one of the above, or is there another one? Sorry, I meant rpmfusion one. So of "course" you need to rebase your patch on top of the existing one as we won't switch history and the package structure. (too bad that you didn't searched for existing package earlier) Hi, the history , I haven't read all bug report. PCSX2 was removed because the last tag on github have 4 years an since then we have 3411 commits , so the package was retired by lack of support https://atim.fedorapeople.org/for-review/pcsx2.spec https://atim.fedorapeople.org/for-review/pcsx2-1.5.0-3.20200205git5308be3.fc31.src.rpm Sérgio Basto(In reply to Sérgio Basto from comment #8) > Hi, the history , I haven't read all bug report. > > PCSX2 was removed because the last tag on github have 4 years an since then > we have 3411 commits , so the package was retired by lack of support Hi, yes, the old one 1.4 very buggy but new one much better. Even if 1.6 not released yet i thought it would be nice to package 1.5 from master branch (1.5 = dev version of 1.6). I tried to run 3 games and they all run in "one click" without any issue 60 FPS even on my slow AMD CPU. Would be nice if you test my build and advice how to improve something. (In reply to Artem from comment #9) > https://atim.fedorapeople.org/for-review/pcsx2.spec Okay for me, so anyone to start reviewing this new version ? I'll review it tomorrow, when I have access to a 32 bit machine. It's getting late here and I need to get up early tomorrow. JFYI: you can run fedora-review on x64 machine for this package: fedora-review -m fedora-rawhide-i386 -u 'https://bugzilla.rpmfusion.org/show_bug.cgi?id=5524' Why not using -langpacks instead of -translation ? Seems to be used by glibc at least to provide translation. Another point is that I don't see the reason to split the package into -data as the software is available only on i686 and -data is mandatory, so It won't save any disk space. (In reply to Nicolas Chauvet from comment #13) > Why not using -langpacks instead of -translation ? > Seems to be used by glibc at least to provide translation. Fixed. Great tip. (In reply to Nicolas Chauvet from comment #14) > Another point is that I don't see the reason to split the package into -data > as the software is available only on i686 and -data is mandatory, so It > won't save any disk space. Fixed. Spec URL: https://atim.fedorapeople.org/for-review/pcsx2.spec SRPM URL: https://atim.fedorapeople.org/for-review/pcsx2-1.5.0-4.20200205git5308be3.fc31.src.rpm Issues:
- rpm -qlp pcsx2-1.5.0-4.20200205git5308be3.fc32.i686.rpm
includes /usr/lib/.build-id and subdirectories. is this intentional?
while you're at it:
- stricter glob for man page (e.g %{appname}.*) and pixmaps (e.g. %{appname}.xpm) please
- fedora-review suggests requiring `%{name}%{?_isa} = %{version}-%{release}` in
`%package langpacks` (you're missing %{?_isa})
- Package must own all directories that it creates.
Note: Directories without known owners:
/usr/share/locale/ar_SA/LC_MESSAGES, /usr/share/locale/ar_SA
I think you can add it below %files ... langpacks and it will be picked up.
Package Review
==============
Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed
===== MUST items =====
C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: 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]: If your application is a C or C++ application you must list a
BuildRequires against gcc, gcc-c++ or clang.
[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: Cannot run licensecheck: Command 'licensecheck -r
/var/lib/mock/fedora-
rawhide-i386/root/builddir/build/BUILD/pcsx2-5308be3c4d1c5b4026ce9cd70ba0b591e9b95f68'
returned non-zero exit status 2.
[x]: License file installed when any subpackage combination is installed.
[-]: If the package is under multiple licenses, the licensing breakdown
must be documented in the spec.
[?]: Package must own all directories that it creates.
Note: Directories without known owners:
/usr/share/locale/ar_SA/LC_MESSAGES, /usr/share/locale/ar_SA
[x]: Package does not own files or directories owned by other packages.
Note: Dirs in package are owned also by: /usr/lib/games/pcsx2(locale,,
defaulting, Failed, set, to, C), /usr/share/doc/pcsx2(locale,,
defaulting, Failed, set, to, C), /usr/share/games/pcsx2(locale,,
defaulting, Failed, set, to, C), /usr/share/licenses/pcsx2(locale,,
defaulting, Failed, set, to, C)
[x]: %build honors applicable compiler flags or justifies otherwise.
[-]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: The spec file handles locales properly.
[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.
[x]: Useful -debuginfo package or justification otherwise.
[-]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
(~1MB) or number of files.
Note: Documentation size is 225280 bytes in 3 files.
[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 requires other packages for directories it uses.
[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]: Package installs a %{name}.desktop using desktop-file-install or
desktop-file-validate if there is such a file.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[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]: Packages must not store files under /srv, /opt or /usr/local
===== SHOULD items =====
Generic:
[-]: 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).
[-]: Fully versioned dependency in subpackages if applicable.
Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in
pcsx2-langpacks
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Scriptlets must be sane, if used.
[-]: Sources are verified with gpgverify first in %prep if upstream
publishes signatures.
Note: gpgverify is not used.
[x]: Description and summary sections in the package spec file contains
translations for supported Non-English languages, if available.
[-]: %check is present and all tests pass.
[?]: Packages should try to preserve timestamps of original installed
files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[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]: 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.
[x]: Package should compile and build into binary rpms on all supported
architectures.
[x]: Spec use %global instead of %define unless justified.
===== EXTRA items =====
Generic:
[-]: Large data in /usr/share should live in a noarch subpackage if package
is arched.
Note: Arch-ed rpms have a total of 2908160 bytes in /usr/share
[x]: Rpmlint is run on debuginfo package(s).
Note: No rpmlint messages.
[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.
(In reply to t-rpmfusion from comment #16) > Issues: > - rpm -qlp pcsx2-1.5.0-4.20200205git5308be3.fc32.i686.rpm > includes /usr/lib/.build-id and subdirectories. is this intentional? Help wanted here. Maybe this is linter bug? > while you're at it: > - stricter glob for man page (e.g %{appname}.*) and pixmaps (e.g. > %{appname}.xpm) please Fixed. > - fedora-review suggests requiring `%{name}%{?_isa} = %{version}-%{release}` > in > `%package langpacks` (you're missing %{?_isa}) %{?_isa} only for arched packages and noarch package shouldn't require arched. RPM linter will say this is error. > - Package must own all directories that it creates. > Note: Directories without known owners: > /usr/share/locale/ar_SA/LC_MESSAGES, /usr/share/locale/ar_SA > I think you can add it below %files ... langpacks and it will be picked up. Fixed. --- Spec URL: https://atim.fedorapeople.org/for-review/pcsx2.spec SRPM URL: https://atim.fedorapeople.org/for-review/pcsx2-1.5.0-5.20200205git5308be3.fc31.src.rpm (In reply to Artem from comment #17) > (In reply to t-rpmfusion from comment #16) > > Issues: > > - rpm -qlp pcsx2-1.5.0-4.20200205git5308be3.fc32.i686.rpm > > includes /usr/lib/.build-id and subdirectories. is this intentional? > > Help wanted here. Maybe this is linter bug? Is a defualt of builds now, is not a problem (In reply to Sérgio Basto from comment #18) > (In reply to Artem from comment #17) > > (In reply to t-rpmfusion from comment #16) > > > Issues: > > > - rpm -qlp pcsx2-1.5.0-4.20200205git5308be3.fc32.i686.rpm > > > includes /usr/lib/.build-id and subdirectories. is this intentional? > > > > Help wanted here. Maybe this is linter bug? > > Is a defualt of builds now, is not a problem My bad, i should clarify this: this .build-id of course is OK from F29, but i did my previous 'fedora-review' and rpm linter warned me about some .build-id dups and i never seen such issue before. But i already removed my old review and now no this issue anyway according to 'fedora-review'. (In reply to Sérgio Basto from comment #18) > Is a defualt of builds now, is not a problem Thanks for the clarification! ----- Package approved. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: 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]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [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: Cannot run licensecheck: Command 'licensecheck -r /var/lib/mock/fedora- rawhide-i386/root/builddir/build/BUILD/pcsx2-5308be3c4d1c5b4026ce9cd70ba0b591e9b95f68' returned non-zero exit status 2. [x]: License file installed when any subpackage combination is installed. [-]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [x]: Package does not own files or directories owned by other packages. Note: Dirs in package are owned also by: /usr/share/locale/ar_SA(C, set, Failed, to, locale,, defaulting), /usr/share/locale/ar_SA/LC_MESSAGES(C, set, Failed, to, locale,, defaulting), /usr/lib/games/pcsx2(C, set, Failed, to, locale,, defaulting), /usr/share/doc/pcsx2(C, set, Failed, to, locale,, defaulting), /usr/share/games/pcsx2(C, set, Failed, to, locale,, defaulting), /usr/share/licenses/pcsx2(C, set, Failed, to, locale,, defaulting) [x]: %build honors applicable compiler flags or justifies otherwise. [-]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: The spec file handles locales properly. [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. [x]: Useful -debuginfo package or justification otherwise. [-]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 225280 bytes in 3 files. [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 requires other packages for directories it uses. [x]: Package must own all directories that it creates. [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]: Package installs a %{name}.desktop using desktop-file-install or desktop-file-validate if there is such a file. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [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]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [-]: 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). [-]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in pcsx2-langpacks [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Scriptlets must be sane, if used. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [x]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [-]: %check is present and all tests pass. [?]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [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]: 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. [x]: Package should compile and build into binary rpms on all supported architectures. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [-]: Large data in /usr/share should live in a noarch subpackage if package is arched. Note: Arch-ed rpms have a total of 2908160 bytes in /usr/share [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [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. Thank you for review and help fixing issues. I guess i shouldn't request now new package an we can co-maintain with Sérgio old one? Add me as co-maintainer and we ready to build it. (In reply to Artem from comment #21) > Thank you for review and help fixing issues. I guess i shouldn't request now > new package an we can co-maintain with Sérgio old one? Add me as > co-maintainer and we ready to build it. I have resurrected the package, you can checkout using rfpkg co nonfree/pcsx2 (In reply to leigh scott from comment #22) > (In reply to Artem from comment #21) > > Thank you for review and help fixing issues. I guess i shouldn't request now > > new package an we can co-maintain with Sérgio old one? Add me as > > co-maintainer and we ready to build it. > > I have resurrected the package, you can checkout using > > rfpkg co nonfree/pcsx2 Thanks a lot. Building... |