Bug 1379798

Summary: Review Request: pcsxr - A plugin based PlayStation (PSX) emulator with high compatibility
Product: [Fedora] Fedora Reporter: Jeremy Newton <alexjnewt>
Component: Package ReviewAssignee: MartinKG <mgansser>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: isage.dna, ngompa13, package-review
Target Milestone: ---Keywords: Reopened
Target Release: ---Flags: mgansser: fedora-review+
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: 2016-12-23 04:37:08 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 1364745    

Description Jeremy Newton 2016-09-27 16:55:34 UTC
I currently maintain pcsxr in RPMFusion, but since the guidelines have loosened on emulators, I would like to move this into Fedora proper.

Spec URL: https://pkgs.rpmfusion.org/cgit/free/pcsxr.git/plain/pcsxr.spec
SRPM URL: http://download1.rpmfusion.org/free/fedora/development/rawhide/Everything/source/SRPMS/p/pcsxr-1.9.94-2.fc25.src.rpm
Description:
PCSX-Reloaded is an advanced PlayStation (PSX) emulator, which uses a plugin
architecture to provide full support for all components of the PSX. It has full
emulation support for game pads, videos, sound, memory cards, and other
important PSX components, and is able to play many games without problems.
Fedora Account System Username: mystro256

Some comments:
-I am using the alpha version of pcsxr (1.9.94) just because it includes some fixes for mac and linux builds. The release page can be found here:
https://pcsxr.codeplex.com/releases/view/112026
-Note that the troublesome source URL should and will be fixed prior to uploading:
Source: https://pcsxr.codeplex.com/downloads/get/756488#/%{name}-%{svnversion}.zip
-The 64bit assert patch just removes an assert for the x86_64 specific code. I reported this upstream, but they seem to want to keep it in, which is strange because it doesn't exist in the equivalent logic for x86_32. This error can be safely ignored from the user end and should really be wrapped in a DEBUG build condition, but upstream doesn't care to do so. This works around a few crashes in some games and does not cause any regression, thus it seems to make sense to keep it.
-I believe that exclusive arch needs to be added, but I haven't looked into it:
ExclusiveArch: %{ix86} x86_64 ppc

Comment 1 Jeremy Newton 2016-11-22 03:10:46 UTC
Reuploading and disabling hardened build

SPEC URL:
https://dl.dropboxusercontent.com/u/42480493/pcsxr.spec

SRPM URL:
https://dl.dropboxusercontent.com/u/42480493/pcsxr-1.9.94-3.fc25.src.rpm

Also, koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=16558025

Comment 2 MartinKG 2016-11-22 07:59:07 UTC
please modify your spec file, 

replace in build section:
make %{?_smp_mflags}
by
%make_build

and in install section:
make %{?_smp_mflags} install DESTDIR=%{buildroot}
by
%make_install

Comment 3 Jeremy Newton 2016-11-23 14:05:02 UTC
(In reply to MartinKG from comment #2)
> please modify your spec file, 
> 
> replace in build section:
> make %{?_smp_mflags}
> by
> %make_build
> 
> and in install section:
> make %{?_smp_mflags} install DESTDIR=%{buildroot}
> by
> %make_install

Done!

SPEC URL:
https://dl.dropboxusercontent.com/u/42480493/pcsxr.spec

SRPM URL:
https://dl.dropboxusercontent.com/u/42480493/pcsxr-1.9.94-4.fc24.src.rpm

Comment 4 Neal Gompa 2016-11-23 14:34:53 UTC
> %configure --prefix=/usr --enable-libcdio --enable-opengl

Please remove --prefix, as that's already set by %configure.

Comment 5 MartinKG 2016-11-23 15:55:02 UTC
please remove --prefix, as Neal mentioned, because the macro
%configure includes already the correct prefix.


rpm -E "%configure"

  .....

  ./configure --build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu \
	--program-prefix= \
	--disable-dependency-tracking \
==>>	--prefix=/usr \
	--exec-prefix=/usr \
	--bindir=/usr/bin \
	--sbindir=/usr/sbin \
	--sysconfdir=/etc \
	--datadir=/usr/share \
	--includedir=/usr/include \
	--libdir=/usr/lib64 \
	--libexecdir=/usr/libexec \
	--localstatedir=/var \
	--sharedstatedir=/var/lib \
	--mandir=/usr/share/man \
	--infodir=/usr/share/info

Comment 6 Jeremy Newton 2016-11-23 16:33:05 UTC
Nice catch!

Fixed! I also switched to "%autosetup" to clean things up.
I just re-uploaded the spec file (no version bump), because I'm not on a computer that can rebuild the SRPM right now. I'll bump it and rebuild the SRPM if you find any more issues; neither change would affect the binary anyway.

Comment 7 MartinKG 2016-11-23 18:58:31 UTC
change also %file section:

from

%files -f %{name}.lang
%doc doc/keys.txt doc/tweaks.txt AUTHORS COPYING README

to 

%files -f %{name}.lang
%doc doc/keys.txt doc/tweaks.txt AUTHORS README
%license COPYING

Comment 8 MartinKG 2016-11-23 19:30:18 UTC
also put the following line into %install section, because .la files should not be included, see: https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#StaticLibraries

# Libtool archives, foo.la files, should not be included.
find %{buildroot} -name '*.la' -exec rm -f {} ';'

Comment 9 Jeremy Newton 2016-11-24 04:16:32 UTC
(In reply to MartinKG from comment #8)
> also put the following line into %install section, because .la files should
> not be included, see:
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#StaticLibraries
> 
> # Libtool archives, foo.la files, should not be included.
> find %{buildroot} -name '*.la' -exec rm -f {} ';'

Done

SPEC URL:
https://dl.dropboxusercontent.com/u/42480493/pcsxr.spec

SRPM URL:
https://dl.dropboxusercontent.com/u/42480493/pcsxr-1.9.94-5.fc24.src.rpm

Comment 10 Neal Gompa 2016-11-24 06:41:43 UTC
> %description
> #modified from debian files

Move the comment to ABOVE "%description", as it gets rendered as part of the description if you don't.

Comment 12 Jeremy Newton 2016-11-24 14:07:17 UTC
(In reply to Neal Gompa from comment #10)
> > %description
> > #modified from debian files
> 
> Move the comment to ABOVE "%description", as it gets rendered as part of the
> description if you don't.

Indeed, it gets rendered as a blank line preceding the description. Fixed and reuploaded (no version bump because it's not a big deal IMHO)
SPEC URL:
https://dl.dropboxusercontent.com/u/42480493/pcsxr.spec

SRPM URL:
https://dl.dropboxusercontent.com/u/42480493/pcsxr-1.9.94-5.fc24.src.rpm

(In reply to MartinKG from comment #11)
> pcsxr is already present in rpmfusion and cannot be at the same time in
> fedora repo and should be dropped from rpmfusion !
> 
> http://mirror.yandex.ru/fedora/rpmfusion/free/fedora/releases/25/Everything/
> x86_64/os/Packages/p/pcsxr-1.9.94-3.fc25.x86_64.rpm
> 
> http://mirror.yandex.ru/fedora/rpmfusion/free/fedora/development/rawhide/
> Everything/x86_64/os/Packages/p/pcsxr-1.9.94-4.fc26.x86_64.rpm

Yes I know, I maintain that package. I was planning on retiring it as soon as this was approved. I just need to make sure I the release version is larger than the rpmfusion package.

Comment 13 MartinKG 2016-11-24 14:41:44 UTC
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]: 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: "FSF All Permissive", "GPL (v2 or later)", "GPL (v3 or later)",
     "Unknown or generated", "BSD (3 clause)", "*No copyright* GPL (v2 or
     later)". 89 files have unknown license. Detailed output of
     licensecheck in /home/martin/rpmbuild/SPECS/pcsxr/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[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.
[-]: 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.
[-]: 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.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 4 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 does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[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 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).
[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in pcsxr-
     debuginfo
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[x]: SourceX tarball generation or download is documented.
     Note: Package contains tarball without URL, check comments
[x]: 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.
[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]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: No rpmlint messages.
[x]: Rpmlint is run on all installed packages.
     Note: No rpmlint messages.
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Package should not use obsolete m4 macros


Rpmlint
-------
Checking: pcsxr-1.9.94-5.fc26.x86_64.rpm
          pcsxr-debuginfo-1.9.94-5.fc26.x86_64.rpm
          pcsxr-1.9.94-5.fc26.src.rpm
pcsxr.src: W: invalid-url Source0: pcsxr-87788.zip
3 packages and 0 specfiles checked; 0 errors, 1 warnings.


Rpmlint (debuginfo)
-------------------
Checking: pcsxr-debuginfo-1.9.94-5.fc26.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.


Rpmlint (installed packages)
----------------------------
2 packages and 0 specfiles checked; 0 errors, 0 warnings.


Requires
--------
pcsxr (rpmlib, GLIBC filtered):
    libGL.so.1()(64bit)
    libSDL2-2.0.so.0()(64bit)
    libX11.so.6()(64bit)
    libXext.so.6()(64bit)
    libXtst.so.6()(64bit)
    libXv.so.1()(64bit)
    libXxf86vm.so.1()(64bit)
    libatk-1.0.so.0()(64bit)
    libc.so.6()(64bit)
    libcairo-gobject.so.2()(64bit)
    libcairo.so.2()(64bit)
    libcdio.so.16()(64bit)
    libcdio.so.16(CDIO_16)(64bit)
    libdl.so.2()(64bit)
    libgdk-3.so.0()(64bit)
    libgdk_pixbuf-2.0.so.0()(64bit)
    libgio-2.0.so.0()(64bit)
    libglib-2.0.so.0()(64bit)
    libgobject-2.0.so.0()(64bit)
    libgtk-3.so.0()(64bit)
    libm.so.6()(64bit)
    libnsl.so.1()(64bit)
    libpango-1.0.so.0()(64bit)
    libpangocairo-1.0.so.0()(64bit)
    libpthread.so.0()(64bit)
    libz.so.1()(64bit)
    rtld(GNU_HASH)

pcsxr-debuginfo (rpmlib, GLIBC filtered):


Provides
--------
pcsxr:
    application()
    application(pcsxr.desktop)
    libBladeSio1.so()(64bit)
    libDFCdrom.so()(64bit)
    libDFInput.so()(64bit)
    libDFNet.so()(64bit)
    libDFSound.so()(64bit)
    libDFXVideo.so()(64bit)
    libpeopsxgl.so()(64bit)
    pcsxr
    pcsxr(x86-64)

pcsxr-debuginfo:
    pcsxr-debuginfo
    pcsxr-debuginfo(x86-64)


Unversioned so-files
--------------------
pcsxr: /usr/lib64/games/psemu/libBladeSio1.so
pcsxr: /usr/lib64/games/psemu/libDFCdrom.so
pcsxr: /usr/lib64/games/psemu/libDFInput.so
pcsxr: /usr/lib64/games/psemu/libDFNet.so
pcsxr: /usr/lib64/games/psemu/libDFSound.so
pcsxr: /usr/lib64/games/psemu/libDFXVideo.so
pcsxr: /usr/lib64/games/psemu/libpeopsxgl.so

Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -m fedora-rawhide-x86_64 -rn ../SRPMS/pcsxr-1.9.94-5.fc25.src.rpm
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

Comment 14 Jeremy Newton 2016-11-24 15:54:46 UTC
Thanks!

Comment 15 Neal Gompa 2016-11-24 18:13:50 UTC
Could you please also add an AppData file so it'll show up in GNOME Software and Plasma Discover? Otherwise most users won't be able to find it without having to get down to the package manager level.

See for details: https://fedoraproject.org/wiki/Packaging:AppData

Comment 16 Jeremy Newton 2016-11-24 18:33:47 UTC
(In reply to Neal Gompa from comment #15)
> Could you please also add an AppData file so it'll show up in GNOME Software
> and Plasma Discover? Otherwise most users won't be able to find it without
> having to get down to the package manager level.
> 
> See for details: https://fedoraproject.org/wiki/Packaging:AppData

Will do, thanks for reminding me

Comment 17 Epifanov Ivan 2016-11-26 02:02:52 UTC
Please note that pcsxr in fedora proper will lack compressed CDDA support. And judging from spec-file, compressed DATA tracks support too.

Comment 18 Jeremy Newton 2016-11-26 02:16:10 UTC
Indeed(In reply to Epifanov Ivan from comment #17)
> Please note that pcsxr in fedora proper will lack compressed CDDA support.
> And judging from spec-file, compressed DATA tracks support too.

Indeed, anything requiring ffmpeg will be disabled, as ffmpeg is not allowed into Fedora proper.

Comment 19 Epifanov Ivan 2016-11-26 16:00:29 UTC
BTW, are you sure that assert patch is (still) required?
Removing this assert can lead to undefined behavior. The root problem of it, however, seems to be fixed long time ago.

Comment 20 Jeremy Newton 2016-11-26 16:14:23 UTC
(In reply to Epifanov Ivan from comment #19)
> BTW, are you sure that assert patch is (still) required?
> Removing this assert can lead to undefined behavior. The root problem of it,
> however, seems to be fixed long time ago.

Well IMHO, hard asserts should not be included in release code, so regardless of it being fixed, it would be hidden behind a DEBUG flag. I can modify the patch to throw a warning on stderr if you feel that would be more adequate. As well, I have a few games I can confirm that throw this assert on boot, which seems absolutely harmless to gameplay when ignored, yet will freeze on boot if not ignored.

If this is fixed for these games in a future release, I will gladly remove this patch.

Comment 21 Epifanov Ivan 2016-11-26 16:24:49 UTC
Care to share names of those games on https://pcsxr.codeplex.com/workitem/8567 ?
Thing is, hard asserts allow to strictly find offending code, while removing them can lead to unexpected bugs later, which users will (wrongly) report.

Comment 22 Jeremy Newton 2016-11-26 16:43:16 UTC
O(In reply to Epifanov Ivan from comment #21)
> Care to share names of those games on
> https://pcsxr.codeplex.com/workitem/8567 ?
> Thing is, hard asserts allow to strictly find offending code, while removing
> them can lead to unexpected bugs later, which users will (wrongly) report.

Megaman Legends 2 was one of the games. I'll retest with and without the patch. It may have been fixed without me realising.

I'll comment back there if I can reproduce it again.

Thanks for letting me know!

Comment 23 Jeremy Newton 2016-11-26 18:21:14 UTC
Looks like it was fixed in 1.99.3, I can no longer reproduce this. I must have missed this when I updated and tested it on RPMFusion. Thanks again for your input!

Updated spec and srpm (removed assert patch, added appdata)
SPEC URL:
https://dl.dropboxusercontent.com/u/42480493/pcsxr.spec
SRPM URL:
https://dl.dropboxusercontent.com/u/42480493/pcsxr-1.9.94-6.fc24.src.rpm

I'm just waiting for SCM approval before I can import this package.

Comment 24 Gwyn Ciesla 2016-11-28 14:12:29 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/pcsxr

Comment 25 Fedora Update System 2016-11-28 16:38:21 UTC
pcsxr-1.9.94-6.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-013c2bf04c

Comment 26 Fedora Update System 2016-11-28 16:38:31 UTC
pcsxr-1.9.94-6.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2016-2b999e631a

Comment 27 Jeremy Newton 2016-11-28 16:53:20 UTC
Built in rawhide.

Also see:
https://bugzilla.rpmfusion.org/show_bug.cgi?id=4360

Comment 28 Fedora Update System 2016-12-02 06:27:13 UTC
pcsxr-1.9.94-6.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-013c2bf04c

Comment 29 Fedora Update System 2016-12-02 18:54:45 UTC
pcsxr-1.9.94-6.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-2b999e631a

Comment 30 Fedora Update System 2016-12-09 16:41:41 UTC
pcsxr-1.9.94-6.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2016-2b999e631a

Comment 31 Fedora Update System 2016-12-10 00:26:00 UTC
pcsxr-1.9.94-6.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

Comment 32 Fedora Update System 2016-12-10 03:55:55 UTC
pcsxr-1.9.94-6.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-2b999e631a

Comment 33 Fedora Update System 2016-12-22 05:23:05 UTC
pcsxr-1.9.94-6.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.