[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 #13 from Orcan Ogetbil <oget.fedora at gmail.com>  2009-04-03 23:16:56 ---
Thanks for the update!

(In reply to comment #12)
> (In reply to comment #11)
> 
> > 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).
> 

Yes, if it is not used, it isn't relevant for the license tag. It's up to your
judgement to use the asm files or not.

> 
> > * 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}")?
> 

There are occurrences of vbam in %install and in %files that you can replace
with %{name}. It is OK to leave the comments as is, but I would use vbam in the
URL for the same purpose you use vbam for the comments to fetch the sources. 

By the way, shouldn't this be the actual URL?:
   http://vba-m.ngemu.com/

And also g%{name} is OK.


> > * 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?
> 

Either patch them, or use this in %prep:
   sed -i 's|\(CMAKE_C.*_FLAGS\).*|\1 "%{optflags}")|' CMakeLists.txt


> > * 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?
> 

I know, but dos2unix is non-standard. Besides it pulls an extra BR. I guess it
is a matter of convention. There are some people who prefer using it, but I'm
not one of them.

> > 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.
> 

You're welcome. I'll wait for the next release.


-- 
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.