[Bug 440] Review request: vbam - Nintendo Gameboy, Gameboy Color, and Gameboy Advance Emulator

[RPM Fusion Bugzilla]

http://bugzilla.rpmfusion.org/show_bug.cgi?id=440





--- Comment #12 from Dave Straub <dave at is1337.net>  2009-04-03 21:57:58 ---
(In reply to comment #11)

Thanks for the response. I updated the spec at the same URL, but I will have a
proper SRPM after I patch it. Responses below:

> * There are many files in the source which are GPLv2 (no later). But there are
> also GPLv2+ and LGPLv2+ files.
> So it looks like the License field should be
> License: GPLv2 and GPLv2+ and LGPLv2+

Thanks for pointing this out, I will use GPLv2 and LGPLv2+ since both binary
files are built from GPLv2 solo and GPLv2 and above source files. (Unless this
does not apply for some reason:
https://fedoraproject.org/wiki/Licensing/FAQ#Multiple_licensing_situations )

> But apart from these, this one file:
>    src/filters/2xSaImmx.asm
> has a different license. I don't know what it should be called. Let's research.

As far as I can see, assembly files are not touched with the default settings,
and I am not specifying to include them. This means that file's license is not
important to the binary package, correct?

Performance seems good enough using the C++ alternatives, and they should keep
it portable. However if you think it would be better, I will could pass the
needed flags on the respective architectures to use the assembly files (and ask
the author or some Fedora list about that license).

> ! Not a blocker, but this line
>    gzip -9 < debian/vbam.1 > %{buildroot}%{_mandir}/man1/vbam.1.gz
> is not really necessary. You can install the file via "install -pm 644 ..." and
> rpmbuild will take care of the rest.

Okay, I didn't know it will compress man pages on its own. Nifty.

> * Package does not build on rawhide! gcc-4.4 issue:
>    /builddir/build/BUILD/vbam-1.8.0/src/sdl/debugger.cpp:948: error: invalid
> conversion from 'const char*' to 'char*'
> 
> Please patch this up and inform upstream.

I will do this over the weekend.

> * The package must be named according to the Package Naming Guidelines. Is the
> project named vbam or vba-m?

It seems the project is called "VBA-M" in discussions, but the SourceForge
project name and all built files are "vbam". I think this makes more sense for
the RPM name.

> * You need to pass VERBOSE=1 to make

Using %cmake generates a verbose Makefile.

> * Requires: hicolor-icon-theme is not necessary since it will be in the
> dependency chain. (rpmbuild will automatically create library dependencies,
> gtk2, libglade etc, and they will pull hicolor-icon-theme)

Removed.

> * We prefer %defattr(-,root,root,-). Also the two %attr(755,root,root) can be
> removed.

Done.

> * You only use %{name} in URL and everywhere else you use vbam. Actually, it is
> the other way around. Please make use of the %{name} and %{version} macros
> extensively, except possibly in URLs.

I don't see very many occurrences of "vbam" other than the file list. I'd
rather not use macros in comment commands as I think they should be
copy-and-paste-able.

I have used the name macro where applicable in the file list. Is it appropriate
to write out the GTK binary's file name "gvbam", or should it have the macro as
well ("g%{name}")?

> * Package does not honor Fedora specific flags. Please see:
>    http://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags
> Replacing "cmake" with the "%cmake" macro may or may not solve the issue. I
> didn't check. But using %cmake is a good practice anyway.

I attempted to use %cmake at first, but it wouldn't link properly with
BUILD_SHARED_LIBS enabled. This flag is switched on in /etc/rpm/macros.cmake
without an option to disable it.

I'm no expert in RPM macros; is there a clean syntax for replacing a substring
like for shell variables? (I have no luck in searching for this kind of thing.)
For the uploaded spec, I used an ugly hack to remove that flag so it can at
least make use of the other options in the macro.

Is the proper action here to patch the source so that flag works correctly?

> * Please preserve the time-stamps of the non compiled files. For instance, you
> can use:
>    sed 's/\r//' doc/License.txt > tmpfile
>    touch -r doc/License.txt tmpfile
>    mv -f tmpfile doc/License.txt

I used what you suggested, but from Andrea's link, the only given reason for
not using dos2unix is that it might break FC3. There is a single flag for
dos2unix to do essentially those three commands. Is anyone actually still
targeting FC3, or is there another reason for not using dos2unix?

> Please let me know if you have any questions/comments/objections.

I think what I ask above covers it. I will post the new SRPM after I add the
patch(es), but the same spec URL currently has the noted changes. Thanks for
the detailed suggestions.


-- 
Configure bugmail: http://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.