[Bug 2455] Review request: pcsx2 - A Sony Playstation2 emulator

[RPM Fusion Bugzilla]

https://bugzilla.rpmfusion.org/show_bug.cgi?id=2455

--- Comment #4 from Andrea Musuruane <musuruan at gmail.com> 2012-08-26 19:10:30 CEST ---
(In reply to comment #3)
> Hi Andrea, Thank-you very much for looking at my package and offering your
> advice. 

You are welcome.

> I've complied with what you've said in most cases but have just a few
> queries. I'll reply to your comments one by one:
> 
> The License field has been updated (shortened!)

It seems the License should be "LGPLv3+". Note the plus at the end. It means
GNU Lesser General Public License either version 3 of the License, or (at your
option) any later version. That is what it's written in the LICENSE file and
the source files I have opened. 

But I'd really like to have a look again at the audition you made on the source
files to be more precise. Unluckily I don't have the old spec file anymore.

You must also make sure you ***don't use any bundled libraries***. You must
remove the 3rdparty directory in %prep and eventually patch pcsx2 to use system
libraries.

> I have followed the revision control packaging guidelines. I think the Name
> realease version is correct as I have specifically packaged a revision 1.0,
> rather than a subversion revision, as this should now see no further activity.
> The release has been named (2) because it is an update of the rpm that I make
> available on the PCSX2 forums

Let's make a step backward. I didn't know pcsx2 v1.0 has been release at the
beginning of this month. So the first question here is why do you base the
package on a revision control checkout instead of a stable release? Usually you
base your packages on stable versions unless there are really good reasons.

> I am not sure about the use of ExclusiveArch. PCSX2 will both compile and run
> on x86_64 and i686, but the code needs to use the i686 libraries. I have tried
> specifying these by giving a full description in BuildRequires (for example
> "Cg.i686") but this does not work. Is there a way to specify to compile only
> from i686 libraries?

Yes. Specify "ExclusiveArch: i686" :). 

A comment specifying why it cannot be compiled and run on other architectures
must be written.

You can build it on x64_64 using mock:
$ mock -r fedora-17-i386-rpmfusion_nonfree pcsx2-1.0-2.fc17.src.rpm

NB: pcsx2 will have to go to rpmfusion-nonfree because it depends on Cg.

> Evince was the suggestion of the fedora packager who helped me in the forums. I
> guess it is for the docs. The program will run fine without the contents of
> %doc.

There is no need to specify evince. Please get rid of the "Requires".

> Again, my mistake to call cmake twice

The %cmake macro already sets CMAKE_INSTALL_PREFIX, therefore do not specify
it. 

Call %cmake with "%cmake . <options>" not "%cmake CmakeList.txt <options>":
https://fedoraproject.org/wiki/Packaging:Cmake

The flags used while compiling are not only the one passed by %cmake (ie the
ones specified in CFLAGS) but there are others that conflict with them ("-m32
-msse -msse2 -march=i686"). You must patch pcsx2 to remove them.

The debuginfo package is too small. The binary files have been stripped. Change
CMAKE_BUILD_TYPE to -DCMAKE_BUILD_TYPE=Debug.

At this point perfoming rpmlint on the debug package, you'll get tons of:
"incorrect-fsf-address". You must report this errors upstream:
https://fedoraproject.org/wiki/Common_Rpmlint_issues#incorrect-fsf-address

> I have moved the part changing permissions to %prep, I think the build places
> some documents in the wrong place for Fedora, so I have left the parts of the
> spec that move them - maybe I should write a patch instead? I will check the
> code and see what I can do.

You can just specify something like this instead of moving and renaming docs.
%doc %{name}/Docs/*.txt

Anyway, the best thing would be to specify the correct DOCDIR in %cmake.

> I have changed the desktop file install and moved the icons to the correct
> place - Oh; now I have just noticed the comment about the icon cache
> scriptlets. I will read about this now, but it will take me a few days to get
> around to making any changes - I will get back to you on this problem

If you have just an icon it is fine to leave it in /usr/share/pixmap. In this
way you won't need the icon cache scriptlets.

There is already a desktop file installed. Why do you provide your own? There
is already an icon installed. Why do you provide your own? Usually you want to
use upstream ones and patch them if needed.

Do not use the following syntax:
%{__mkdir_p}

Just use "mkdir -p". It's more consistent with the style used in the spec file.

Never use the %makeinstall macro. Use "make install".
https://fedoraproject.org/wiki/Packaging/Guidelines#Why_the_.25makeinstall_macro_should_not_be_used

For handling locale files, have a look here:
https://fedoraproject.org/wiki/Packaging/Guidelines#Handling_Locale_Files

Please read carefully what you need to do when you have a program that is NOT
called %{name}.

-- 
Configure bugmail: https://bugzilla.rpmfusion.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.
You are the assignee for the bug.