Bug 2098

Summary: Review request: dolphin-emu - Gamecube / Wii / Triforce Emulator
Product: Package Reviews Reporter: Jeremy Newton <alexjnewt>
Component: Review RequestAssignee: RPM Fusion Package Review <rpmfusion-package-review>
Status: RESOLVED FIXED    
Severity: normal CC: belegdol, chillermillerlong, d.bz-rpmfusion, hobbes1069, leamas.alec, musuruan, rpmfusion-package-review
Priority: P5    
Version: Current   
Hardware: All   
OS: GNU/Linux   
See Also: https://bugzilla.redhat.com/show_bug.cgi?id=810059
namespace:
Bug Depends on:    
Bug Blocks: 4    
Attachments: Fedora 18 build fix

Description Jeremy Newton 2011-12-18 19:38:28 CET
Dolphin is a Gamecube, Wii and Triforce (the arcade machine based on the Gamecube) emulator which supports many extra features and abilities not present on the original consoles.

Why it's not in Fedora: Emulators of video game systems seem to have legal problems, which is not a new story for such applications, thus this excluded from the fedora repos.

I left a bunch of comments in the SPEC file for easer review. As well, no source code changes should be required between distribution versions (e.g. f15 and f16), thus the source rpm should be quite agnostic. Although, it does need to be build every major change in dependency versions (e.g. SDL 1.2.X to 1.3.X or glew 1.6.X to 1.7.X), which should only happen every distribution upgrade anyway.

This package is already posted in the Wishlist by the way, and is licensed as FOSS (GPL2). This is my first RPM Fusion package and I would assume I would want a sponsor? Though I'm not sure what that is to be honest.

SPEC URL:
http://dl.dropbox.com/u/42480493/dolphin-emu.spec
SOURCE RPM URL: (though f16, should be distribution agnostic)
http://dl.dropbox.com/u/42480493/dolphin-emu-3.0-1.fc16.src.rpm


RPMLINT WARNINGS:

Gamecube is not in the dictionary, hence this warning:
dolphin-emu.x86_64: W: spelling-error Summary(en_US) Gamecube -> Game cube, Game-cube, Gamecock

I'm pretty sure this is required, hence why I left it in:
dolphin-emu.x86_64: E: zero-length /usr/share/dolphin-emu/user/GameConfig/WBEEJV.ini

I don't believe a manpage is required or even existent. Should I convert the readme file to a manpage?
dolphin-emu.x86_64: W: no-manual-page-for-binary dolphin-emu

I'm unfamiliar with this error and rpmlint -I file-not-in-%lang provides no explanation. Dolphin seems to run fine, but any help/feedback would be great.
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/ar/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/ca/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/cs/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/de/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/el/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/en/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/es/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/fr/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/he/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/hu/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/it/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/ja/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/ko/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/nb/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/nl/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/pl/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/pt/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/pt_BR/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/ru/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/sr/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/tr/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/zh_CN/LC_MESSAGES/dolphin-emu.mo
dolphin-emu.x86_64: W: file-not-in-%lang /usr/share/locale/zh_TW/LC_MESSAGES/dolphin-emu.mo
Comment 1 Jeremy Newton 2011-12-18 19:41:36 CET
I also forgot the source RPM warnings:


Gamecube is not in dictionary:
dolphin-emu.src: W: spelling-error Summary(en_US) Gamecube -> Game cube, Game-cube, Gamecock


I didn't think this mattered:
dolphin-emu.src:40: W: setup-not-quiet
Comment 2 Jeremy Newton 2011-12-18 21:23:27 CET
Also, note that I tried using %find_lang but generated many errors. I left it commented out and did not know what to do with it. From what I could figure out in the last hour, I'm told the locale warnings are caused by not using find_lang.
Comment 3 Julian Sikorski 2011-12-19 11:48:30 CET
I cannot sponsor you, but these are my initial comments:
1. Library requirements should be picked up automatically, not be explicit: 
https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires
2. SFML and libsoil:
https://bugzilla.redhat.com/show_bug.cgi?id=759057
https://bugzilla.redhat.com/show_bug.cgi?id=759059
https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries
3. I think it would be better to spin your own tarball from upstream git than to rely on other distributions.
4. There are some licensing issues with dolphin:
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=535073#29
Comment 4 Jeremy Newton 2011-12-20 02:43:38 CET
I have a few questions if you're able to answer them:

1) Which requires are considered explicit? All of them? or just the ones that the package needs that aren't added by rpmbuild?

2) Unless I mistaken, isn't this a suitable way to build dolphin until fedora until SFML and libsoil are packaged?

3) I can make a spin but the only place I can post it is on my drop box; is there another place I can post it or is this just fine?

4) If I remove wiiuse from the dep's, shouldn't this be alright? I realize that Debian has strict rules against free libraries that link to non-free ones without a special license, but would this really be a problem with RPM Fusion?
Comment 5 Julian Sikorski 2011-12-20 13:00:31 CET
(In reply to comment #4)
> I have a few questions if you're able to answer them:
> 
> 1) Which requires are considered explicit? All of them? or just the ones that
> the package needs that aren't added by rpmbuild?

Normally everything should be picked up by RPM. So, unless it misses something, do not put it into Requires. In your case, at a first glance it seems like all your Requires can be dropped.

> 
> 2) Unless I mistaken, isn't this a suitable way to build dolphin until fedora
> until SFML and libsoil are packaged?

Bring this to the rpmfusion-developers list to get an exception. I would say it is, but I have no casting vote on the matter.

> 
> 3) I can make a spin but the only place I can post it is on my drop box; is
> there another place I can post it or is this just fine?

You don't even need an url. Put just the filename in the source0, and put the commands you used to spin it in the comments above.

> 
> 4) If I remove wiiuse from the dep's, shouldn't this be alright? I realize that
> Debian has strict rules against free libraries that link to non-free ones
> without a special license, but would this really be a problem with RPM Fusion?

Again, bring this to the list please. Dolphin definitely needs to go to nonfree, but other than that I don't know what will be decided.
Comment 6 Jeremy Newton 2011-12-20 17:36:31 CET
Thanks for your help! I'll update it when I have a chance :)

Also, just to make sure, this is there list you spoke of right?
http://lists.rpmfusion.org/mailman/listinfo/rpmfusion-developers
Comment 7 Julian Sikorski 2011-12-20 17:37:18 CET
Yes, exactly.
Comment 8 Jeremy Newton 2011-12-20 20:42:52 CET
Done with the SPEC modifications, I also fixed most of the rpmlint errors. Next thing to do is ask for an exception on the developer list, but I'll have to do that when I have time.

Here's the current RPMLINT errors/warnings:

Again, Gamecube is not in the dictionary:
dolphin-emu.x86_64: W: spelling-error Summary(en_US) Gamecube -> Game cube, Game-cube, Gamecock

I don't want to remove this in case it's required, I can do some testing if it's necessary:
dolphin-emu.x86_64: E: zero-length /usr/share/dolphin-emu/user/GameConfig/WBEEJV.ini

I'm not sure why it thinks the desktop is a script, so I ignored this.
dolphin-emu.x86_64: E: script-without-shebang /usr/share/applications/dolphin-emu.desktop

Again, I don't think this is required
dolphin-emu.x86_64: W: no-manual-page-for-binary dolphin-emu

Source0 is not an URL but a local source package, hence this error:
dolphin-emu.src: W: invalid-url Source0: dolphin-emu-3.0.tar.gz


The packages are on my dropbox, thus the download URL's should be the same as the URL's in the description.

Note to reviewer(s): I included my own spin on the source, where the differences are listed in the SPEC file (above the source near the top). The original source was grabbed from git, commands of getting such is also listed in the SPEC file (under %build). For quick reference, the commands to grab the original source are:

    git clone https://code.google.com/p/dolphin-emu/ dolphin-emu-3.0
    cd dolphin-emu-3.0
    git checkout 3.0
Comment 9 Julian Sikorski 2012-01-13 22:13:23 CET
FYI, the reviews of SOIL and SFML have been finished, the packages are in -testing now.
Comment 10 Jeremy Newton 2012-01-14 00:44:36 CET
Thanks to Julian Sikorski, I was able to remake the SPEC to not bundle SFML and SOIL. Also thanks to the tips given by me by Andrea Musuruane, I managed to improve the SPEC greatly.

Here's the links:
SPEC:
http://dl.dropbox.com/u/42480493/dolphin-emu.spec
SRPM:
http://dl.dropbox.com/u/42480493/dolphin-emu-3.0-2.fc16.src.rpm

Unfortunately one more issue still exists; when I change cmake to %cmake (line 55), the build fails just before it finishes (says 100%ish). I'll post the output of the build here for anyone who understands what went wrong, as it went right over my head. I also plan to ask a dev and do some research. Anyone who can help, it would be greatly appreciated.

Also, as a side question for anyone who can help: should I change the license to "GPLv2 with Non-free dependency" or something along those lines, or is it fine as is?
Comment 11 Jeremy Newton 2012-01-23 05:21:36 CET
I forgot to post the output, but it doesn't matter much because I fixed the issue. As well, I made various fixes and reorganized the SPEC.

Unfortunately I do still get some RPMLINT issues, but I'm not sure exactly how to deal with them:

dolphin-emu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPHWInterface.cpp
dolphin-emu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPIntUtil.h
dolphin-emu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPStacks.h
dolphin-emu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPInterpreter.cpp
dolphin-emu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/disassemble.h
dolphin-emu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPStacks.cpp
dolphin-emu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPCore.cpp
dolphin-emu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/disassemble.cpp
dolphin-emu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPHWInterface.h
dolphin-emu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPMemoryMap.h
dolphin-emu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPMemoryMap.cpp
dolphin-emu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPCore.h
dolphin-emu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPIntExtOps.h
1 packages and 0 specfiles checked; 13 errors, 0 warnings.

As well here's the updated package links:
SPEC:
http://dl.dropbox.com/u/42480493/dolphin-emu.spec
SRPM:
http://dl.dropbox.com/u/42480493/dolphin-emu-3.0-3.fc16.src.rpm
Comment 12 Jeremy Newton 2012-01-25 21:42:51 CET
Just I contacted upstream about the FSF address issue, so there seems to be no further issues with my dolphin package to my knowledge.
Comment 13 Jeremy Newton 2012-01-30 01:15:02 CET
I've update the SPEC to allow building with encoding frame dumping enabled, as it is enabled by default in the vanilla source and maybe useful to some users.

SPEC:
http://dl.dropbox.com/u/42480493/dolphin-emu.spec
SRPM:
http://dl.dropbox.com/u/42480493/dolphin-emu-3.0-4.fc16.src.rpm
Comment 14 Alec Leamas 2012-02-20 22:29:48 CET
I'm about to make a more complete, informal review. But first two remarks:

srpm doesn't build on koji, f16 and f17. Links:
   http://koji.fedoraproject.org/koji/taskinfo?taskID=3805865
   http://koji.fedoraproject.org/koji/taskinfo?taskID=3805914

The source comment doesn't describe the complete procedure to build the tarball. As a consequence, I cannot verify md5sum of srpm's source vs upstream.
Comment 15 Alec Leamas 2012-02-21 11:32:11 CET
More complete review. Hope this is not bad news :)

--a

[OK] Package meets naming guidelines
[OK] Spec file matches base package name.
[OK] Spec has consistent macro usage.
[E ] Meets Packaging Guidelines.
        Bundled libraries, source URL not properly commented.
[OK] License field OK
[E ] License field in spec matches source
        Some files are BSD
[OK] License file included in package
[OK] Spec in American English
[OK] Spec is legible.
[? ] Sources match upstream md5sum:
        Can't build tarball from upstream, missing input in comment.
        md5sum of tarball in srpm: 0b5f8cde0295d0742547010472854eaa
[? ] Package needs ExcludeArch
        Possibly just x86 due to SSE2 requirement.
[E ] BuildRequires correct
        Koji builds fails on F16-F17 due to BR.
[OK] Spec handles locales/find_lang
[NA] Package is relocatable and has a reason to be.
[OK] Package has no %defattr or permissions.
[OK] Package has a no  %clean section.
[OK] Package has no buildroot
[OK] Package is code or permissible content.
[NA] Doc subpackage needed/used.
[OK] Packages %doc files don't affect runtime.
[OK] Package is a GUI app and has a .desktop file
[OK] Package compiles and builds on at least one arch.
        f15/i386
[OK] Package has no duplicate files in %files.
[OK] Package doesn't own any directories other packages own.
[OK] Package owns all the directories it creates.
[OK] No rpmlint output.
        Output below. It can be ignored besides the Source URL warning,
        iff upstream has a report about the bad FSF addresses.
[OK] final provides and requires are sane:
        See below
SHOULD Items:

[E ] Should build in mock.
        f16,f17 fails in koji.
[? ] Should build on all supported archs
        Not tested
[OK] Should function as described.
        Starts, menus seems to work. Not much of a test.
[OK] Should have sane scriptlets.
[NA] Should have subpackages require base package with fully versioned depend.
[OK] Should have dist tag
[OK] Should package latest version
[NA] check for outstanding bugs on package. (For core merge reviews)

Issues:
1.  Bundles bochs/disasm in Externals
    http://bochs.sourceforge.net/cgi-bin/lxr/source/disasm.
2.  Bundles clrun, from clunix in Externals
    http://www.cmap.polytechnique.fr/~sylvain/clunix.
3.  Source/Core/Core/Src/IPC_HLE contains the kernel headers hci.h
    and l2cap.h, which seems like a bad bundling.
4.  From README, dolphin-emu needs SSE2 support. Does this rule out
    non-x86 architectures, which needs to be excluded?
5.  Source URL is not completely commented on how to generate tarball.
    Why don't you just use upstream tarball as-is, add the add-on files
    as SourceX: and removes Externals/misc in %prep? This is more
    transparent and makes source verification simple.
6.  A number of files have BSD licenses. Shouldn't these be part of
    License tag like in "GPLv2 and BSD" or similar?
7.  Koji build fails on F16/F17, bad BR.
8.  No review remark as such, but the install icons part is a little messy,
    could possibly be rewritten like:
	for size in 16 32 48 128 256; do
	    dim="${size}x${size}"
	    install -p -D -m 0644 %{name}$size.png \
		%{buildroot}%{_datadir}/icons/hicolor/$dim/apps/%{name}.png
	done
    Similarly, the %files section icons could be written:
        %{_datadir}/icons/hicolor/*/apps/%{name}.png
    However, this boils down to personal preferences.


Rpmlint:

[leamas] $ rpmlint ~/rpmbuild/RPMS/i686/dolphin-emu-3.0-4.fc15.i686.rpm ~/rpmbuild/RPMS/i686/dolphin-emu-debuginfo-3.0-4.fc15.i686.rpm dolphin-emu ~/rpmbuild/SPECS/dolphin-emu.spec | uniq
dolphin-emu.i686: W: spelling-error Summary(en_US) Gamecube -> Game cube, Game-cube, Gamecock
dolphin-emu.i686: E: zero-length /usr/share/dolphin-emu/user/GameConfig/WBEEJV.ini
dolphin-emu.i686: W: no-manual-page-for-binary dolphin-emu
~/rpmbuild/SPECS/dolphin-emu.spec: W: invalid-url Source0: dolphin-emu-3.0.tar.gz
dolphin-emu-debuginfo.i686: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPHWInterface.cpp
  [repeated 11 times]
3 packages and 1 specfiles checked; 13 errors, 5 warnings


Final Requires/Provides:
	dolphin-emu-3.0-4.fc15.i686.rpm
	dolphin-emu = 3.0-4.fc15
	dolphin-emu(x86-32) = 3.0-4.fc15
    =
	/bin/sh
	libCg.so
	libCgGL.so
	libGL.so.1
	libGLEW.so.1.5
	libGLU.so.1
	libICE.so.6
	libSDL-1.2.so.0
	libSM.so.6
	libSOIL.so.1
	libX11.so.6
	libXext.so.6
	libXrandr.so.2
	libao.so.4
	libasound.so.2
	libasound.so.2(ALSA_0.9)
	libasound.so.2(ALSA_0.9.0rc4)
	libatk-1.0.so.0
	libavcodec.so.52
	libavcodec.so.52(LIBAVCODEC_52)
	libavformat.so.52
	libavformat.so.52(LIBAVFORMAT_52)
	libavutil.so.50
	libavutil.so.50(LIBAVUTIL_50)
	libbluetooth.so.3
	libcairo.so.2
	libdl.so.2
	libfreetype.so.6
	libgcc_s.so.1
	libgcc_s.so.1(GCC_3.0)
	libgdk-x11-2.0.so.0
	libgdk_pixbuf-2.0.so.0
	libglib-2.0.so.0
	libgobject-2.0.so.0
	libgomp.so.1
	libgomp.so.1(GOMP_1.0)
	libgomp.so.1(OMP_1.0)
	libgtk-x11-2.0.so.0
	liblzo2.so.2
	libopenal.so.1
	libpango-1.0.so.0
	libportaudio.so.2
	libpulse.so.0
	libpulse.so.0(PULSE_0)
	libsfml-network.so.1.6
	libstdc++.so.6
	libstdc++.so.6(CXXABI_1.3)
	libswscale.so.0
	libswscale.so.0(LIBSWSCALE_0)
	libwx_baseu-2.8.so.0
	libwx_baseu-2.8.so.0(WXU_2.8)
	libwx_gtk2u_adv-2.8.so.0
	libwx_gtk2u_adv-2.8.so.0(WXU_2.8)
	libwx_gtk2u_aui-2.8.so.0
	libwx_gtk2u_aui-2.8.so.0(WXU_2.8)
	libwx_gtk2u_aui-2.8.so.0(WXU_2.8.9)
	libwx_gtk2u_core-2.8.so.0
	libwx_gtk2u_core-2.8.so.0(WXU_2.8)
	libz.so.1

	dolphin-emu-debuginfo-3.0-4.fc15.i686.rpm
	dolphin-emu-debuginfo = 3.0-4.fc15
	dolphin-emu-debuginfo(x86-32) = 3.0-4.fc15
    =
Comment 16 Jeremy Newton 2012-02-21 17:53:43 CET
(In reply to comment #14)
> I'm about to make a more complete, informal review. But first two remarks:
> 
> srpm doesn't build on koji, f16 and f17. Links:
>    http://koji.fedoraproject.org/koji/taskinfo?taskID=3805865
>    http://koji.fedoraproject.org/koji/taskinfo?taskID=3805914
> 
> The source comment doesn't describe the complete procedure to build the
> tarball. As a consequence, I cannot verify md5sum of srpm's source vs upstream.

Interesting, I had no issue building this on F16, I'll have to look into this.

(In reply to comment #15)
> More complete review. Hope this is not bad news :)

It's quite alright, it's all a part of the review process ;)

> Issues:
> 1.  Bundles bochs/disasm in Externals
>     http://bochs.sourceforge.net/cgi-bin/lxr/source/disasm.
> 2.  Bundles clrun, from clunix in Externals
>     http://www.cmap.polytechnique.fr/~sylvain/clunix.

Alright, dully noted, I'll remove these when I get a chance.

> 3.  Source/Core/Core/Src/IPC_HLE contains the kernel headers hci.h
>     and l2cap.h, which seems like a bad bundling.

Hmmm that seem problematic... I may have to contact upstream about this.

> 4.  From README, dolphin-emu needs SSE2 support. Does this rule out
>     non-x86 architectures, which needs to be excluded?

I don't think this is an issue, as RPMFusion only supports x86 archs.

> 5.  Source URL is not completely commented on how to generate tarball.
>     Why don't you just use upstream tarball as-is, add the add-on files
>     as SourceX: and removes Externals/misc in %prep? This is more
>     transparent and makes source verification simple.

Unfortunately there is no upstream tarball to my knowledge, so that is why I included a git command. What would you suggest with the %prep section? I'm using a spin as suggested by Julian in comment 3:

>>>I think it would be better to spin your own tarball from upstream git than
>>>to rely on other distributions.

> 6.  A number of files have BSD licenses. Shouldn't these be part of
>     License tag like in "GPLv2 and BSD" or similar?

I must have missed that, I'll look into that and fix it.

> 8.  No review remark as such, but the install icons part is a little messy,
>     could possibly be rewritten like:
>     for size in 16 32 48 128 256; do
>         dim="${size}x${size}"
>         install -p -D -m 0644 %{name}$size.png \
>         %{buildroot}%{_datadir}/icons/hicolor/$dim/apps/%{name}.png
>     done
>     Similarly, the %files section icons could be written:
>         %{_datadir}/icons/hicolor/*/apps/%{name}.png
>     However, this boils down to personal preferences.

Good point, I that would clean things up.

> 
> Rpmlint:
> 
> dolphin-emu.i686: E: zero-length
> /usr/share/dolphin-emu/user/GameConfig/WBEEJV.ini

That reminds me, I need to contact upstream about the value of this file.

> dolphin-emu-debuginfo.i686: E: incorrect-fsf-address
> /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPHWInterface.cpp
>   [repeated 11 times]

I have reported this and it should be fixed in the next stable version



Thanks for your help, I'll look into fixing these issues :)
Comment 17 Jeremy Newton 2012-02-21 18:13:18 CET
(In reply to comment #14)
> I'm about to make a more complete, informal review. But first two remarks:
> 
> srpm doesn't build on koji, f16 and f17. Links:
>    http://koji.fedoraproject.org/koji/taskinfo?taskID=3805865
>    http://koji.fedoraproject.org/koji/taskinfo?taskID=3805914
> 
> The source comment doesn't describe the complete procedure to build the
> tarball. As a consequence, I cannot verify md5sum of srpm's source vs upstream.

These aren't building on koji because Dolphin requires Cg from RPMFusion non-free; it's failing because it can't find this package, not because it's not building.
Comment 18 Julian Sikorski 2012-02-21 18:15:06 CET
Also, please don't use koji to build packages which are not allowed in Fedora proper :)
Comment 19 Alec Leamas 2012-02-21 20:11:43 CET
(In reply to comment #17)
> (In reply to comment #14)
> > I'm about to make a more complete, informal review. But first two remarks:
> > 
> > srpm doesn't build on koji, f16 and f17. Links:
> >    http://koji.fedoraproject.org/koji/taskinfo?taskID=3805865
> >    http://koji.fedoraproject.org/koji/taskinfo?taskID=3805914
> > 
> > The source comment doesn't describe the complete procedure to build the
> > tarball. As a consequence, I cannot verify md5sum of srpm's source vs upstream.
> 
> These aren't building on koji because Dolphin requires Cg from RPMFusion
> non-free; it's failing because it can't find this package, not because it's not
> building.

Idiot! Talking of me.
Comment 20 Jeremy Newton 2012-02-21 20:25:11 CET
(In reply to comment #19)
> (In reply to comment #17)
> > (In reply to comment #14)
> > > I'm about to make a more complete, informal review. But first two remarks:
> > > 
> > > srpm doesn't build on koji, f16 and f17. Links:
> > >    http://koji.fedoraproject.org/koji/taskinfo?taskID=3805865
> > >    http://koji.fedoraproject.org/koji/taskinfo?taskID=3805914
> > > 
> > > The source comment doesn't describe the complete procedure to build the
> > > tarball. As a consequence, I cannot verify md5sum of srpm's source vs upstream.
> > 
> > These aren't building on koji because Dolphin requires Cg from RPMFusion
> > non-free; it's failing because it can't find this package, not because it's not
> > building.
> 
> Idiot! Talking of me.

It's quite alright, just check your logs next time ;)

But as Julian said, use koji to build Fedora packages not RPMFusion packages, as RPMFusion packages tend to depend on things that aren't a part of Fedora.
Comment 21 Alec Leamas 2012-02-21 20:33:39 CET
Let's forget this sad koji story...

> > 3.  Source/Core/Core/Src/IPC_HLE contains the kernel headers hci.h
> >     and l2cap.h, which seems like a bad bundling.
> 
> Hmmm that seem problematic... I may have to contact upstream about this.
Talking to upstream is of course  never wrong. In the meantime, can't you just remove these and replace with symlinks to system headers in %prep?


[cut]
> Unfortunately there is no upstream tarball to my knowledge, so that is why I
> included a git command. What would you suggest with the %prep section? I'm
> using a spin as suggested by Julian in comment 3:
I would interpret Julians "spin" differently, but I'm Swede, you know... In the word "spin" I would understand a git clone at a given state, without adding anything I didn't get from the git repo. Those things (desktop, icons) still are handled better separately, perhaps in a small tar archive (Source1:) which is unpacked in  %prep. But of course it's a possible way to mix it in a source combo. But you then need to document it, and that can be messy. One way is to write a script which creates the combo, and adding that script to the srpm. Then the comment could be straight-forward.

Git is no problem at all. But you need to show to complete sequence of commands leading up to the tarball. Usually something like

git clone whatever
cd whatever; git reset --hard commit; cd ..
tar czf whatever.tar.gz --exclude=.git whatever

but obviously  something else in your case, The overall requirement on the source comment is that you should be able to create you own source from upstream using it.
Comment 22 Julian Sikorski 2012-02-21 20:40:56 CET
> But as Julian said, use koji to build Fedora packages not RPMFusion packages,
> as RPMFusion packages tend to depend on things that aren't a part of Fedora.

That, and in extreme cases it might put Fedora at legal risk since the questionable content is being hosted on their servers.
Comment 23 Alec Leamas 2012-02-21 20:55:23 CET
(In reply to comment #22)
> > But as Julian said, use koji to build Fedora packages not RPMFusion packages,
> > as RPMFusion packages tend to depend on things that aren't a part of Fedora.
> 
> That, and in extreme cases it might put Fedora at legal risk since the
> questionable content is being hosted on their servers.

I got the message... It was a mistake, probably caused by inexperience combined with switching between RH and RPMFusion too rapidly, too many things in the air. I am sorry, really.
Comment 24 Julian Sikorski 2012-02-21 20:57:33 CET
It's OK, this is how we all learn. I just happened to ask spot for this once and thus I know what their opinion is. Same goes for fedorapeople.org IIRC.
Comment 25 Jeremy Newton 2012-02-21 21:38:00 CET
(In reply to comment #24)
> Same goes for fedorapeople.org IIRC.

Yeah fedorapeople.org has the same rules as the fedora repos.

(In reply to comment #23)
> (In reply to comment #22)
> > > But as Julian said, use koji to build Fedora packages not RPMFusion packages,
> > > as RPMFusion packages tend to depend on things that aren't a part of Fedora.
> > 
> > That, and in extreme cases it might put Fedora at legal risk since the
> > questionable content is being hosted on their servers.
> 
> I got the message... It was a mistake, probably caused by inexperience combined
> with switching between RH and RPMFusion too rapidly, too many things in the
> air. I am sorry, really.

Haha, it's alright, it's quite alright, no harm done and nothing that couldn't have been fixed anyway. :)

(In reply to comment #21)
>Git is no problem at all. But you need to show to complete sequence of commands
>leading up to the tarball.

Fair enough, that makes sense; I'll include that in my next rev. I'll just use second source, as that probably makes the most amount of sense.

>Talking to upstream is of course  never wrong. In the meantime, can't you just
>remove these and replace with symlinks to system headers in %prep?

Yeah I was going to try that first but I'm not sure if that will work to be honest. I think it may mess up the debug package but that is a bit of a blind guess lol. None the less, I'm going to report it upstream in hopes it gets fixed in the next stable version.
Comment 26 Alec Leamas 2012-02-22 09:28:43 CET
Woke up, had a bad feeling about these bundled headers.

First, hci.h and l2cap.h is possibly just the tip of the iceberg. If you run script below, you will find a list of candidates. There are plenty of false positives, but also some files to take care about. Note that the script requires relevant -devel packages installed, but I guess you have :)

Secondly, symlinking was a bad idea. What you should do is to just remove these files in %prep and setup compiler flags so the system headers are used instead. A long time since I used cmake, but I *think* something like this could work:

export CFLAGS="-I/usr/include/ffmpeg -I/usr/include/bluetooth ..."
%cmake  ...

Find headers hack:

#!/bin/bash
for header in $(find  Source -name \*.h); do
    basename=$( basename $header );
    sys_headers=$( find /usr/include -name $basename)
    test -n "$sys_headers" && {
        echo "$header"
        for sh in $( echo $sys_headers ); do
            echo "    $sh"
        done
    }
done
Comment 27 Jeremy Newton 2012-02-24 00:58:20 CET
(In reply to comment #26)
> Woke up, had a bad feeling about these bundled headers.
> 
> First, hci.h and l2cap.h is possibly just the tip of the iceberg. If you run
> script below, you will find a list of candidates. There are plenty of false
> positives, but also some files to take care about. Note that the script
> requires relevant -devel packages installed, but I guess you have :)
> 
> Secondly, symlinking was a bad idea. What you should do is to just remove these
> files in %prep and setup compiler flags so the system headers are used instead.
> A long time since I used cmake, but I *think* something like this could work:
> 
> export CFLAGS="-I/usr/include/ffmpeg -I/usr/include/bluetooth ..."
> %cmake  ...
> 
> Find headers hack:
> 
> #!/bin/bash
> for header in $(find  Source -name \*.h); do
>     basename=$( basename $header );
>     sys_headers=$( find /usr/include -name $basename)
>     test -n "$sys_headers" && {
>         echo "$header"
>         for sh in $( echo $sys_headers ); do
>             echo "    $sh"
>         done
>     }
> done

Though I thank you for the script, after running this, I can't confirm any bundled headers. In fact all the headers that came up were false positives including those Bluetooth files. Not only were they not equivalent code, but also were dolphin specific and had two sets of different licenses, copyrights and authors.

At the moment, if I am not mistaken, I don't see any bundled libraries besides those listed in Externals. I'll have to look into packaging these libraries or getting an exception for Dolphin, as I have yet to look into what these libraries are.

(In reply to comment #16)
> > 6.  A number of files have BSD licenses. Shouldn't these be part of
> >     License tag like in "GPLv2 and BSD" or similar?
> 
> I must have missed that, I'll look into that and fix it.
> 

It seems that in hunting for those BSD licensed files, I found some OpenSSL licensed files and IIRC, OpenSSL conflicts with GPL. Depending on how the code works, this may be a big problem; I'll have to contact upstream about this.
Comment 28 Xiao-Long Chen 2012-02-24 08:41:44 CET
Hmm...I had no idea that Dolphin-emu was already being packaged. It could have saved me some time from writing my own spec :)

Anyway, I'd just like to post my spec file here. For the most part, it's identical to Jeremy's spec file, but I've included a few things:

1. I use a script to create the tarball from git. This is similar to the way Fedora does their git packages (at least for some).

2. I've included Glenn Rice's manual page (licensed under GPLv2), which solves that rpmlint error.

3. I've added my own desktop file, which includes English and Simplified Chinese translations.

4. Dolphin's source already includes an XPM icon, so I just install that in /usr/share/pixmaps, rather than installing separate icons in /usr/share/icons/hicolor-icon-theme.

Here's the spec and source files: https://github.com/chenxiaolong/Fedora-SRPMS/tree/master/dolphin-emu It builds cleanly in Mock with the Fedora, rpmfusion-free, and rpmfusion-nonfree repositories enabled. Rpmlint results on the SRPM and RPM's built in mock: http://paste.kde.org/428198/
Comment 29 Xiao-Long Chen 2012-02-24 08:43:43 CET
By the way, the licenses of the Dolphin source code are identified in Glenn Rice's Ubuntu packaging file, debian/copyright, which is in the *.debian.tar.gz files in the Dolphin PPA: http://ppa.launchpad.net/glennric/dolphin-emu/ubuntu/pool/main/d/dolphin-emu/
Comment 30 Jeremy Newton 2012-02-24 15:51:25 CET
(In reply to comment #28)
> Hmm...I had no idea that Dolphin-emu was already being packaged. It could have
> saved me some time from writing my own spec :)
> 
> Anyway, I'd just like to post my spec file here. For the most part, it's
> identical to Jeremy's spec file, but I've included a few things:
> 
> 1. I use a script to create the tarball from git. This is similar to the way
> Fedora does their git packages (at least for some).
> 
> 2. I've included Glenn Rice's manual page (licensed under GPLv2), which solves
> that rpmlint error.
> 
> 3. I've added my own desktop file, which includes English and Simplified
> Chinese translations.
> 
> 4. Dolphin's source already includes an XPM icon, so I just install that in
> /usr/share/pixmaps, rather than installing separate icons in
> /usr/share/icons/hicolor-icon-theme.
> 
> Here's the spec and source files:
> https://github.com/chenxiaolong/Fedora-SRPMS/tree/master/dolphin-emu It builds
> cleanly in Mock with the Fedora, rpmfusion-free, and rpmfusion-nonfree
> repositories enabled. Rpmlint results on the SRPM and RPM's built in mock:
> http://paste.kde.org/428198/

(In reply to comment #29)
> By the way, the licenses of the Dolphin source code are identified in Glenn
> Rice's Ubuntu packaging file, debian/copyright, which is in the *.debian.tar.gz
> files in the Dolphin PPA:
> http://ppa.launchpad.net/glennric/dolphin-emu/ubuntu/pool/main/d/dolphin-emu/

Oh wow, that definitely saves me a lot of time of looking around. Thanks a lot for the help :)

I just need to get bochs_disasm and clrun packaged, and figure out the purpose of /usr/share/dolphin-emu/user/GameConfig/WBEEJV.ini.
Comment 31 Xiao-Long Chen 2012-02-24 18:32:18 CET
The WBEEJV.ini was just removed in git 53 minutes ago :)

I've added a line to my spec file to remove it.
Comment 32 Jeremy Newton 2012-02-24 18:42:21 CET
(In reply to comment #31)
> The WBEEJV.ini was just removed in git 53 minutes ago :)
> 
> I've added a line to my spec file to remove it.

Haha it does seem like WBEEJV.ini is useless, so I removed the file after build to silence the rpmlint error. Also I incorporated a bunch of additions and clean-up thanks to your spec/sources.

Note that the 2 bundled libraries have yet to be removed, as it's still a work in progress and specs will have to be submitted to Fedora.

Here's the spec and SRPM:
SPEC:
http://dl.dropbox.com/u/42480493/dolphin-emu.spec
SRPM:
http://dl.dropbox.com/u/42480493/dolphin-emu-3.0-6.fc16.src.rpm

RPMLint warnings can now all be effectively ignored :)

dolphin-emu.src: W: spelling-error Summary(en_US) Gamecube -> Game cube, Game-cube, Gamecock
dolphin-emu.src: W: invalid-url Source1: dolphin-emu-extra.tar.xz
dolphin-emu.src: W: invalid-url Source0: dolphin-emu-3.0.tar.xz
dolphin-emu.x86_64: W: spelling-error Summary(en_US) Gamecube -> Game cube, Game-cube, Gamecock
3 packages and 0 specfiles checked; 0 errors, 4 warnings.
Comment 33 Xiao-Long Chen 2012-02-28 22:24:46 CET
bochs is already packaged in Fedora, but it's not compiled with '--enable-disasm'. I've filed a bug about thie: https://bugzilla.redhat.com/show_bug.cgi?id=798437 :D
Comment 34 Jeremy Newton 2012-02-29 00:23:37 CET
(In reply to comment #33)
> bochs is already packaged in Fedora, but it's not compiled with
> '--enable-disasm'. I've filed a bug about thie:
> https://bugzilla.redhat.com/show_bug.cgi?id=798437 :D

Seems like you beat me to it ;)

I actually attempted to look into this a few nights ago; I think there's legal issues with this, although I'm by no means certain. If it gets a wontfix, it shouldn't be too hard to add an alternative version of bochs in RPM Fusion.



As for CLrun, I'm having trouble finding the website or the hosting for the latest code. I found opencl-utils on Google code but it looks like it's no longer active and the bundled dolphin version is much later than the svn in the source tab.
Comment 35 Xiao-Long Chen 2012-02-29 00:55:13 CET
Okay, I think we'll need to make an exception for disasm. The bochs build system compiles disasm statically and doesn't install the header files, which means that the Fedora packagers will need to hack the build system to compile a shared .so library (and deal with versioning, etc). Then, the header files will need to be manually installed in the spec.

I think it would be far easier to use the included disasm library.
Comment 36 Xiao-Long Chen 2012-02-29 01:55:13 CET
(In reply to comment #34)
> As for CLrun, I'm having trouble finding the website or the hosting for the
> latest code. I found opencl-utils on Google code but it looks like it's no
> longer active and the bundled dolphin version is much later than the svn in the
> source tab.

I just ran a diff on the opencl-utils on Google Code and the dolphin-emu source code and it seems that a lot of the changes were just to make clrun work with Windows. Otherwise, the code is pretty much the same :-).

The dolphin-emu developers did do a few bug fixes though (git log: http://paste.kde.org/431000/ ). With git, it shouldn't be hard to include those patches in a package.
Comment 37 Jeremy Newton 2012-02-29 06:24:16 CET
(In reply to comment #35)
> Okay, I think we'll need to make an exception for disasm. The bochs build
> system compiles disasm statically and doesn't install the header files, which
> means that the Fedora packagers will need to hack the build system to compile a
> shared .so library (and deal with versioning, etc). Then, the header files will
> need to be manually installed in the spec.
> 
> I think it would be far easier to use the included disasm library.

I would need to take a look into it myself and perhaps talk with the bochs maintainer, but it does sound like an exception may have to be made in this case. Depending on the maintainer, he maybe up for manually packaging the headers and whatnot, who knows. Furthermore, he could have some tips/tricks, and it maybe worth it to talk to him.

(In reply to comment #36)
> (In reply to comment #34)
> > As for CLrun, I'm having trouble finding the website or the hosting for the
> > latest code. I found opencl-utils on Google code but it looks like it's no
> > longer active and the bundled dolphin version is much later than the svn in the
> > source tab.
> 
> I just ran a diff on the opencl-utils on Google Code and the dolphin-emu source
> code and it seems that a lot of the changes were just to make clrun work with
> Windows. Otherwise, the code is pretty much the same :-).
> 
> The dolphin-emu developers did do a few bug fixes though (git log:
> http://paste.kde.org/431000/ ). With git, it shouldn't be hard to include those
> patches in a package.

Splendid, thanks for your help! I'll have to look into it when I have a little more time on my hands to generate patches and a spec. I'm very busy at the moment, so I'm  not sure how long it will take me, but feel free to help out if you're interested :)
Comment 38 Alec Leamas 2012-02-29 08:10:59 CET
Maybe you could just handle this using the general "Conditionalized functionality" exception  defined in http://fedoraproject.org/wiki/Packaging:Treatment_Of_Bundled_Libraries. To me, it looks like it covers this case.
Comment 39 Jeremy Newton 2012-02-29 16:33:18 CET
(In reply to comment #38)
> Maybe you could just handle this using the general "Conditionalized
> functionality" exception  defined in
> http://fedoraproject.org/wiki/Packaging:Treatment_Of_Bundled_Libraries. To me,
> it looks like it covers this case.

In regards to bochs?
Comment 40 Alec Leamas 2012-02-29 17:09:12 CET
Yes.
Comment 41 Jeremy Newton 2012-03-10 21:49:34 CET
I talked with one of the maintainers for bochs and he said he's willing to looking into making a devel package for bochs. I managed to compile it with a handful of symlinks of the upstream bochs source code, but I think I'll take advantage of the bundling exception for the time being.

Off to look into packaging CLrun :)
Comment 42 Xiao-Long Chen 2012-03-12 18:06:50 CET
(In reply to comment #41)
> I talked with one of the maintainers for bochs and he said he's willing to
> looking into making a devel package for bochs. I managed to compile it with a
> handful of symlinks of the upstream bochs source code, but I think I'll take
> advantage of the bundling exception for the time being.
> 
> Off to look into packaging CLrun :)

That's great :)

I just wanted to let you know that I had a problem in the get-source-from-git.sh script that cause the latest git version to be downloaded, not version 3.0. I've fixed this in my git repository, so hopefully you can update it in your src rpm :D Sorry!
Comment 43 Jeremy Newton 2012-03-12 18:09:06 CET
(In reply to comment #42)
> (In reply to comment #41)
> > I talked with one of the maintainers for bochs and he said he's willing to
> > looking into making a devel package for bochs. I managed to compile it with a
> > handful of symlinks of the upstream bochs source code, but I think I'll take
> > advantage of the bundling exception for the time being.
> > 
> > Off to look into packaging CLrun :)
> 
> That's great :)
> 
> I just wanted to let you know that I had a problem in the
> get-source-from-git.sh script that cause the latest git version to be
> downloaded, not version 3.0. I've fixed this in my git repository, so hopefully
> you can update it in your src rpm :D Sorry!

It's quite alright, I should probably should have checked for that too ;)
Comment 44 Jeremy Newton 2012-04-05 05:01:30 CEST
Sorry for the delay, but I packaged clrun. I may have to contact someone who is more familiar with cmake on how I would patch it to not use the bundle clrun. Although I made a devel file for clrun, so I may be able to get away with symlinks, like bochs-disasm.

None the less that package review blocks this one:
https://bugzilla.redhat.com/show_bug.cgi?id=810059
Comment 45 Alec Leamas 2012-04-05 08:16:51 CEST
Richard is the cmake guru here,  it seems. In one of the rpmfusion bugs (lost which one) you can find:

I can help there. It looks like lib3ds-devel provides a pkgconf config file so
something like this should work:

INCLUDE(FindPkgConfig)
IF(USE_EXTERNAL_LIB3DS)
       # The last argument may need to be lib3ds depending on the pkgconf
config.
       PKG_CHECK_MODULES(LIB3DS REQUIRED 3ds)
       INCLUDE_DIRECTORIES(SYSTEM ${LIB3DS_INCLUDE_DIRS})
ENDIF()
---

And then the following:
TARGET_LINK_LIBRARIES(Exchange3DS TKernel TKBRep TKMath TKMesh TKV3d TKTopAlgo
3ds)
would become:
TARGET_LINK_LIBRARIES(Exchange3DS TKernel TKBRep TKMath TKMesh TKV3d TKTopAlgo
${LIB3DS_LIBRARIES})
---

Also, if you use the "option" as shown above you'll need to add that to you
cmake options:
%cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo \
       -DUSE_EXTERNAL_LIB3DS=TRUE \
Comment 46 Richard 2012-04-05 15:12:03 CEST
(In reply to comment #45)
> Richard is the cmake guru here,  it seems. In one of the rpmfusion bugs (lost
> which one) you can find:

Thanks for the title but there's still a few others on RPM Fusion that know it better than I :)

 
> I can help there. It looks like lib3ds-devel provides a pkgconf config file so
> something like this should work:
> 
> INCLUDE(FindPkgConfig)
> IF(USE_EXTERNAL_LIB3DS)
>        # The last argument may need to be lib3ds depending on the pkgconf
> config.
>        PKG_CHECK_MODULES(LIB3DS REQUIRED 3ds)
>        INCLUDE_DIRECTORIES(SYSTEM ${LIB3DS_INCLUDE_DIRS})
> ENDIF()

You really have two options here. If you want to send a patch upstream so you don't have to maintain a patch forever then the above will work but you'll also need to add the ELSE() condition to be acceptable upstream.

If you don't want to send it upstream (although I recommend you do, it will make you're life easier in the long run) you could go with a minimal patch and not mess with creating a new cmake option.


> Also, if you use the "option" as shown above you'll need to add that to you
> cmake options:
> %cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo \
>        -DUSE_EXTERNAL_LIB3DS=TRUE \

The CMAKE_BUILD_TYPE should only be used if you're not getting the right build flags. I'm not entirely sure what causes it but some packages try to build with -O3 instead of -O2 which this option often fixes. Try building without the option first and review your build log to see if the Fedora build flags are being overridden.
Comment 47 Alec Leamas 2012-04-05 15:29:31 CEST
i. e.,  s/the cmake guru/a cmake guru/ :)
Comment 48 Jeremy Newton 2012-04-05 18:35:58 CEST
(In reply to comment #45)
> I can help there. It looks like lib3ds-devel provides a pkgconf config file so
> something like this should work:
> 

Well opencl-utils-CLRun-devel does not have a pkgconf and to be honest, I'm frankly lost when it comes to cmake.

(In reply to comment #46)
> (In reply to comment #45)
> You really have two options here. If you want to send a patch upstream so you
> don't have to maintain a patch forever then the above will work but you'll also
> need to add the ELSE() condition to be acceptable upstream.

Hmmm, yeah that is what I aim for but I don't know how to detect if a shared *.so file exists in cmake.
 
> If you don't want to send it upstream (although I recommend you do, it will
> make you're life easier in the long run) you could go with a minimal patch and
> not mess with creating a new cmake option.

That was my backup solution, but not long after I realized I still don't know how to link a library anyway in cmake haha. I was going to play around with it to see if I could get something but I haven't had enough time yet. Do you know somewhere or someone to bug to get help make a patch?
Comment 49 Richard 2012-04-05 18:58:35 CEST
Ok, maybe I'm missing something but in the main CMakeLists.txt file I see:
--- start ---
if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
        find_library(CL OpenCL)
        set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,-weak_framework,OpenCL")
else()
        include_directories(Externals/CLRun/include)
        add_subdirectory(Externals/CLRun)
endif()
--- end ---

This leads me to believe CLRun is only needed on OSX/Darwin unless it's referenced  somewhere else but I did a recursive grep for it and didn't find any other instance.

Did I miss something? One good test would be to rm -rf Externals/CLRun in %prep and see if the build succeseeds since bundled libraries need to be removed anyway per the guidelines.
Comment 50 Richard 2012-04-05 19:00:36 CEST
Oops. I missed the "else()" ignore the previous comment.
Comment 51 Richard 2012-04-05 21:27:57 CEST
Ok, it took me a while to figure out the right cmake magic. The dolphin project is not making use of the full capability of cmake. Instead of using add_library(...) and target_link_library(...) they are using a more manual linking method that confused the heck out of me.

Here's the updated patch and spec file:
http://dl.dropbox.com/u/34775202/dolphin-emu.spec
http://dl.dropbox.com/u/34775202/dolphin-emu-3.0-clrun.patch

Since CLRun doesn't have any sort of automated config I added a cmake flag for the include directory. I also moved to an out-of-source build because the in-source builds just clutter things up when you're trying to work on build problems.

I didn't bump the rev in the spec file or add a BuildRequires for CLRun-devel so you'll need to do that.
Comment 52 Jeremy Newton 2012-04-06 06:04:44 CEST
(In reply to comment #51)
> Ok, it took me a while to figure out the right cmake magic. The dolphin project
> is not making use of the full capability of cmake. Instead of using
> add_library(...) and target_link_library(...) they are using a more manual
> linking method that confused the heck out of me.
> 
> Here's the updated patch and spec file:
> http://dl.dropbox.com/u/34775202/dolphin-emu.spec
> http://dl.dropbox.com/u/34775202/dolphin-emu-3.0-clrun.patch
> 
> Since CLRun doesn't have any sort of automated config I added a cmake flag for
> the include directory. I also moved to an out-of-source build because the
> in-source builds just clutter things up when you're trying to work on build
> problems.
> 
> I didn't bump the rev in the spec file or add a BuildRequires for CLRun-devel
> so you'll need to do that.

Wow thanks a lot, that really helped! I'll know what to do next time too.
None the less, I'll send the patch upstream right away!
Haha, you're a miracle worker Richard ;)

And with that, dolphin is officially bundled library free :)

Here's the new files:
SPEC:
http://dl.dropbox.com/u/42480493/dolphin-emu.spec
SRPM:
http://dl.dropbox.com/u/42480493/dolphin-emu-3.0-8.fc16.src.rpm

Note this requires bochs-devel but it is only available in Fedora 17, though it can be easily rebuilt (that's how I got my build to work on F16). Also of course opencl-utils is still in review.
Comment 53 Xiao-Long Chen 2012-04-06 07:21:27 CEST
Wow! Nice work! I just got back from vacation during my spring break and didn't expect to see so much done :D

One little mistake on my git script (again...):

SOURCEDIR=$(cd $(dirname ${0}) | pwd)

should be

SOURCEDIR=$(cd $(dirname ${0}) && pwd)

Sorry!
Comment 54 Jeremy Newton 2012-04-06 07:42:09 CEST
(In reply to comment #53)
> Wow! Nice work! I just got back from vacation during my spring break and didn't
> expect to see so much done :D
> 
> One little mistake on my git script (again...):
> 
> SOURCEDIR=$(cd $(dirname ${0}) | pwd)
> 
> should be
> 
> SOURCEDIR=$(cd $(dirname ${0}) && pwd)
> 
> Sorry!

No problem, not a bit issue. Since this doesn't effect the way I pulled my source0, there's no need to bump a rev. I reloaded the new SRPM to the same link :)
Comment 55 Xiao-Long Chen 2012-04-06 07:49:31 CEST
(In reply to comment #54)
> No problem, not a bit issue. Since this doesn't effect the way I pulled my
> source0, there's no need to bump a rev. I reloaded the new SRPM to the same
> link :)

Awesome! I just compiled bochs (from fedpkg), opencl-utils (from your SRPM), and dolphin-emu, and I'm happy to say that they all worked perfectly :)

I tested a game just to make sure: http://i.imgur.com/adlPE.jpg
Comment 56 Xiao-Long Chen 2012-06-03 02:50:20 CEST
Because of the new version of GCC in Fedora 17, dolphin-emu no longer builds. I've created a simple patch that fixes this issue (it doesn't affect previous Fedora releases). Here's the spec and SRPM:

http://ompldr.org/vZTJlMw/dolphin-emu.spec
http://ompldr.org/vZTJlNA/dolphin-emu-3.0-9.fc17.src.rpm
Comment 57 Jeremy Newton 2012-06-25 02:33:33 CEST
Thanks for your help :)

Have you submitted this patch upstream or is this fixed in the latest git?
Comment 58 Xiao-Long Chen 2012-06-25 08:44:50 CEST
No problem :)

The patch is already committed upstream (although I think the commit was to fix another problem).
Comment 59 Andrea Musuruane 2012-06-25 10:18:24 CEST
I had a very quick glance at the package and I notice that the spec doesn't require hicolor-icon-theme. Any application installing icons to hicolor MUST Require hicolor-icon-theme. The purpose of hicolor-icon-theme is to be required by any package which installs any icons. It contains no actual icons, but just an empty theme for applications to install their icons into.
Comment 60 Jeremy Newton 2012-06-25 16:36:46 CEST
(In reply to comment #58)
> No problem :)
> 
> The patch is already committed upstream (although I think the commit was to fix
> another problem).

Thanks, I'll make note of it in the spec

(In reply to comment #59)
> I had a very quick glance at the package and I notice that the spec doesn't
> require hicolor-icon-theme. Any application installing icons to hicolor MUST
> Require hicolor-icon-theme. The purpose of hicolor-icon-theme is to be required
> by any package which installs any icons. It contains no actual icons, but just
> an empty theme for applications to install their icons into.

Good eye, thanks!



Here's an updated spec; I'm just waiting on a SCM request for CLRun to unblock this. Note that I changed the package name of CLRun, so to install, one would have to uninstall open-utils-CLRun in favour for the new package to avoid a file conflict.

SPEC:
http://dl.dropbox.com/u/42480493/dolphin-emu.spec
SRPM:
http://dl.dropbox.com/u/42480493/dolphin-emu-3.0-10.fc17.src.rpm

Current RPMLINT Ouput:

dolphin-emu.src: W: spelling-error Summary(en_US) Gamecube -> Game cube, Game-cube, Gamecock
dolphin-emu.x86_64: W: spelling-error Summary(en_US) Gamecube -> Game cube, Game-cube, Gamecock
dolphin-emu.src: W: invalid-url Source1: dolphin-emu-extra.tar.xz
dolphin-emu.src: W: invalid-url Source0: dolphin-emu-3.0.tar.xz
dolphin-emu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPHWInterface.cpp
...
dolphin-emu-debuginfo.x86_64: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPIntExtOps.h
3 packages and 0 specfiles checked; 13 errors, 4 warnings.

The FSF Address issue has been fixed in the unstable version, the invalid-url can be safely ignored due to the nature of the source files, and gamecube is obviously not in the dictionary.

I'm just waiting to push this: https://bugzilla.redhat.com/show_bug.cgi?id=810059
Comment 61 Alec Leamas 2012-07-01 21:35:22 CEST
The comment on how to generate Source0 is still incomplete, I cant check the upstream source
Comment 62 Alec Leamas 2012-07-01 22:24:27 CEST
I. e., the comment for Source0 is there, adding a dependency on Source1.  But the comment for Source1 doesn't  really work.
Comment 63 Alec Leamas 2012-07-01 22:37:35 CEST
The tarball generated by the script get-source-from-git.sh doesn't match the tarball in the srpm: different size, large diff.
Comment 64 Jeremy Newton 2012-07-03 11:38:55 CEST
Hmm... odd, there must have been an glitch when I last pulled the source.
I'll do a repull and reupload the SRPM when I get home later today.
Comment 65 Jeremy Newton 2012-07-04 01:52:42 CEST
(In reply to comment #64)
> Hmm... odd, there must have been an glitch when I last pulled the source.
> I'll do a repull and reupload the SRPM when I get home later today.

Very odd indeed, I'm not sure what went wrong there.

None the less, I ran the script in SOURCE1 and rebuilt the rev:
http://dl.dropbox.com/u/42480493/dolphin-emu-3.0-10.fc17.src.rpm

If there is any more issues, it's likely an issue with the git I'm pulling from. Also, I pulled it twice just to make sure, diff -r seems to give consistent results now, though md5sums of the tar.xz do not match.
Comment 66 Alec Leamas 2012-07-04 22:32:00 CEST
(In reply to comment #65)
> (In reply to comment #64)
> > Hmm... odd, there must have been an glitch when I last pulled the source.
> > I'll do a repull and reupload the SRPM when I get home later today.
> 
> Very odd indeed, I'm not sure what went wrong there.
> 
> None the less, I ran the script in SOURCE1 and rebuilt the rev:
> http://dl.dropbox.com/u/42480493/dolphin-emu-3.0-10.fc17.src.rpm
> 
> If there is any more issues, it's likely an issue with the git I'm pulling
> from. Also, I pulled it twice just to make sure, diff -r seems to give
> consistent results now, though md5sums of the tar.xz do not match.

Out of the top of my head, standard git retains modification times when checking out a commit, but resets them when checking out a tag. So this is as expected.



Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated


===== MUST items =====
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Rpath absent or only used for internal libs.
[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]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Buildroot is not present
[x]: Package contains no bundled libraries.
[x]: Changelog in prescribed format.
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Sources contain only permissible code or content.
[x]: Each %files section contains %defattr if rpm < 4.4
     Note: Note: defattr macros not found. They would be needed for EPEL5
[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 if there is
     such a file.
[-]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Permissions on files are set properly.
[x]: Package does not contain duplicates in %files.
[x]: Package complies to the Packaging Guidelines
[x]: Spec file lacks Packager, Vendor, PreReq tags.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[-]: Large documentation files are in a -doc subpackage, if required.
[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 %doc.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "*No copyright* UNKNOWN", "zlib/libpng GPL", "UNKNOWN", "BSD (3 clause)
     GPL", "GPL", "GPL (v2 or later)", "BSD (3 clause)", "*No copyright* BSD",
     "GPL (v2 or later) (with incorrect FSF address)", "*No copyright*
     GENERATED FILE", "GPL (unversioned/unknown version) GPL", "GPL (v2) (with
     incorrect FSF address)" For detailed output of licensecheck see file:
     /home/mk/FedoraReview/src/dolphin-emu/licensecheck.txt
[x]: The spec file handles locales properly.
[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named using only allowed ascii characters.
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
     Note: Package contains no Conflicts: tag(s)
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package installs properly.
[x]: Package is not relocatable.
[x]: Requires correct, justified where necessary.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file is legible and written in American English.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: Package contains systemd file(s) if in need.
[x]: File names are valid UTF-8.
[x]: Useful -debuginfo package or justification otherwise.

===== SHOULD items =====
[x]: Reviewer should test that the package builds in mock.
[-]: 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]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[ ]: Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[?]: 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]: Scriptlets must be sane, if used.
[x]: SourceX / PatchY prefixed with %{name}.
[x]: SourceX is a working URL.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[?]: 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]: Spec use %global instead of %define.

===== EXTRA items =====
[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: dolphin-emu-3.0-10.fc17.src.rpm
          dolphin-emu-debuginfo-3.0-10.fc17.i686.rpm
          dolphin-emu-3.0-10.fc17.i686.rpm
dolphin-emu.src: W: spelling-error Summary(en_US) Gamecube -> Game cube, Game-cube, Gamecock
dolphin-emu.src: W: invalid-url Source1: dolphin-emu-extra.tar.xz
dolphin-emu.src: W: invalid-url Source0: dolphin-emu-3.0.tar.xz
dolphin-emu-debuginfo.i686: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPHWInterface.cpp
[ another 12 incorrect-fsf-address removed ]
dolphin-emu.i686: W: spelling-error Summary(en_US) Gamecube -> Game cube, Game-cube, Gamecock
3 packages and 0 specfiles checked; 13 errors, 4 warnings.


Rpmlint (installed packages)
----------------------------
# rpmlint dolphin-emu dolphin-emu-debuginfo
dolphin-emu.i686: I: enchant-dictionary-not-found en_US
dolphin-emu-debuginfo.i686: E: incorrect-fsf-address /usr/src/debug/dolphin-emu-3.0/Source/Core/Core/Src/DSP/DSPHWInterface.cpp
[ another 12 incorrect-fsf-address removed ]
2 packages and 0 specfiles checked; 13 errors, 0 warnings.
# echo 'rpmlint-done:'

Requires
--------
dolphin-emu-debuginfo-3.0-10.fc17.i686.rpm (rpmlib, GLIBC filtered):
    

dolphin-emu-3.0-10.fc17.i686.rpm (rpmlib, GLIBC filtered):
    
    /bin/sh  
    hicolor-icon-theme  
    libCg.so  
    libCgGL.so  
    libGL.so.1  
    libGLEW.so.1.6  
    libGLU.so.1  
    libSDL-1.2.so.0  
    libSOIL.so.1  
    libX11.so.6  
    libXext.so.6  
    libXrandr.so.2  
    libao.so.4  
    libao.so.4(LIBAO4_1.1.0)  
    libasound.so.2  
    libasound.so.2(ALSA_0.9)  
    libasound.so.2(ALSA_0.9.0rc4)  
    libatk-1.0.so.0  
    libavcodec.so.53  
    libavcodec.so.53(LIBAVCODEC_53)  
    libavformat.so.53  
    libavformat.so.53(LIBAVFORMAT_53)  
    libavutil.so.51  
    libavutil.so.51(LIBAVUTIL_51)  
    libbluetooth.so.3  
    libc.so.6  
    libcairo.so.2  
    libclrun.so.0.16  
    libfreetype.so.6  
    libgcc_s.so.1  
    libgdk-x11-2.0.so.0  
    libgdk_pixbuf-2.0.so.0  
    libglib-2.0.so.0  
    libgobject-2.0.so.0  
    libgomp.so.1  
    libgomp.so.1(GOMP_1.0)  
    libgomp.so.1(OMP_1.0)  
    libgtk-x11-2.0.so.0  
    liblzo2.so.2  
    libm.so.6  
    libopenal.so.1  
    libpango-1.0.so.0  
    libportaudio.so.2  
    libpthread.so.0  
    libpulse.so.0  
    libpulse.so.0(PULSE_0)  
    libsfml-network.so.1.6  
    libstdc++.so.6  
    libstdc++.so.6(CXXABI_1.3)  
    libswscale.so.2  
    libswscale.so.2(LIBSWSCALE_2)  
    libwx_baseu-2.8.so.0  
    libwx_baseu-2.8.so.0(WXU_2.8)  
    libwx_gtk2u_adv-2.8.so.0  
    libwx_gtk2u_adv-2.8.so.0(WXU_2.8)  
    libwx_gtk2u_aui-2.8.so.0  
    libwx_gtk2u_aui-2.8.so.0(WXU_2.8)  
    libwx_gtk2u_aui-2.8.so.0(WXU_2.8.9)  
    libwx_gtk2u_core-2.8.so.0  
    libwx_gtk2u_core-2.8.so.0(WXU_2.8)  
    libz.so.1  
    rtld(GNU_HASH)  

Provides
--------
dolphin-emu-debuginfo-3.0-10.fc17.i686.rpm:
    
    dolphin-emu-debuginfo = 3.0-10.fc17
    dolphin-emu-debuginfo(x86-32) = 3.0-10.fc17

dolphin-emu-3.0-10.fc17.i686.rpm:
    
    dolphin-emu = 3.0-10.fc17
    dolphin-emu(x86-32) = 3.0-10.fc17

MD5-sum check
-------------
Using local file /home/mk/FedoraReview/src/dolphin-emu-3.0.tar.xz as upstream
/home/mk/FedoraReview/src/dolphin-emu-3.0.tar.xz :
  MD5SUM this package     : 6fb4c50ddb22cd6413b24ac76bd2735f
  MD5SUM upstream package : 7cda1c13c6f80d3b4b7f44c17df27a6c
However, diff -r shows no differences

Generated by fedora-review 0.2.0git (0be4d03) last change: 2012-07-03
Applied tests: Generic C/C++
Command line :./fedora-review -u https://bugzilla.rpmfusion.org/show_bug.cgi?id=2098 -m fedora-17-i386-rpmfusion_nonfree
External plugins:
Comment 67 Alec Leamas 2012-07-04 22:34:11 CEST
No blocking issues...

*** Approved
Comment 68 Alec Leamas 2012-07-04 22:35:30 CEST
A little late, but I should assign it as well ;)
Comment 69 Jeremy Newton 2012-07-05 08:34:35 CEST
(In reply to comment #68)
> A little late, but I should assign it as well ;)

Thanks! :)
Comment 70 Jeremy Newton 2012-07-05 08:34:50 CEST
Package CVS request
======================
Package Name: dolphin-emu
Short Description: Gamecube / Wii / Triforce Emulator
Owners: jem256
Branches: f17 devel
InitialCC:
----------------------
License tag: nonfree
Comment 71 Jeremy Newton 2012-07-05 18:32:10 CEST
Fixed in Fedora 17 Updates Testing.

Note this has not been successfully built in Fedora 18 (aka devel), but I wouldn't declare it a breaking issue until F18 is released.
Comment 72 Xiao-Long Chen 2012-12-11 22:05:29 CET
Git commit e5d051a4e977443785f4e78cb6464eca0408e578 fixes the ffmpeg build issue in Fedora 18. The patch for the commit can be generated with:

git format-patch 29f52ce6dd8b16773b48936cda80d434143966f1..e5d051a4e977443785f4e78cb6464eca0408e578

I'll attach it to this bug report.
Comment 73 Xiao-Long Chen 2012-12-11 22:05:54 CET
Created attachment 1010 [details]
Fedora 18 build fix
Comment 74 Jeremy Newton 2012-12-14 17:23:34 CET
Thanks! I've been racking my brain over fixing this for a while, I've just been so busy unfortunately.

Much appreciated!

As well, I'm curious if you interested in co-maintaining this package with me?