[Bug 456190] Review Request: dosemu - dos emulator

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


https://bugzilla.redhat.com/show_bug.cgi?id=456190





--- Comment #46 from Justin Zygmont <solarflow99 at gmail.com>  2009-03-22 19:29:45 EDT ---
(In reply to comment #44)
> Guys, please don't discuss the functional issues here, until the package is in
> Fedora. Once there will be a single official build of the package you'll agree
> on, then it will start to make sense to solve issues, not before then. It makes
> this review report a lot less legible for the reviewer, and it's increasingly
> hard to keep track of the packaging issues here. Derek, thank you for the
> participation though.


ok, thanks for letting us know.


> When it comes to the compiler flags, I see them being properly used, since
> %configure macro sets them correctly. Probably past use of ./configure instead
> of %configure resulted into build w/o proper flags, but it all seems OK to me
> now. (Please be sure you understand it though).


Thats good to know because I didnt see any place where opt_flags was used.


> Justin, are there any examples of your work that could prove that you
> understand packaging well? Some informal reviews? I'll need to see it so that 
> I can sponsor you.


ok, I am about to do this, i'll let you know.


> So here's first crack at the official review (sorry that it took so long):
> 
> 1.) Source files
> 
> Please comment on how did you get these:


You mean comment where I got it in the spec file?


> VCS checkout? Which revision?
> Source:         %{name}-%{version}.tgz
> 
> Full URL?
> Source1:        %{name}-freedos-bin.tgz
> 
> Original work?
> Source2:        %{name}.desktop
> 
> Isn't etc/dosemu.xpm from the source tarball sufficient?
> Source3:        %{name}.xpm


I think I got that idea from the dosbox spec file, I can change it if
necessary.


> 2.) FreeDOS image
> 
> I don't believe this is formally allowed (shipping binaries), though other
> packages do this (say, qemu includes bochs bios image).  I'll ask on packaging
> list how to deal with this and let you know.


ok, I guess I overlooked it as shipping binaries because it was just a freedos
image.  I hope it will be ok.


> It is also probably illegal (if it's GPL) to distribute this without source
> code, so at the very least, please add also the source code package and, as
> always, please comment as heavily as you can :)


ok, I guess the source URL in the comments.  


> In case we won't be able to this -- do you know if it's very hard to build
> FreeDOS kernel image and cross-build the basic utilities (at least COMMAND.COM)
> with the tooling currently in Fedora?
> 
> Source1:        %{name}-freedos-bin.tgz


hmm, I guess that would be the ideal way, but the freedos kernel image is
trimmed and customized a bit, i'd hate to have to go down that route just for a
replaceable image.  At this point I dont know what it takes to build that.
This .tgz file is not something thats available on the freedos site, for the
source code, all they have is a large ISO image.


> 3.)  Please use make flags.
> 
> Please add %{_smp_mflags} after make, unless it breaks built.  If it does,
> please add a comment instead.
> 
> See: http://fedoraproject.org/wiki/Packaging/Guidelines#Parallel_make


ok thanks.  Done.


> 4.) Legibility
> 
> Please try not to cross 75 or 80 column boundary:
> 
> chmod 755 $RPM_BUILD_ROOT%{_datadir}/dosemu
> $RPM_BUILD_ROOT%{_datadir}/dosemu/drive_z
> $RPM_BUILD_ROOT%{_datadir}/dosemu/drive_z/doc/exe2bin
> 
> That is better written as:
> 
> chmod 755 $RPM_BUILD_ROOT%{_datadir}/dosemu \
>   $RPM_BUILD_ROOT%{_datadir}/dosemu/drive_z \
>   $RPM_BUILD_ROOT%{_datadir}/dosemu/drive_z/doc/exe2bin


All done.  I just have a URL that is long, should I break that up too?


> 5.) Group
> 
> RPMLint complains:
> 
> dosemu.i586: W: non-standard-group Game/Emulator
> 
> Indeed, this is not a standard group. Pick one from /usr/share/doc/rpm*/GROUPS.
> I believe the right one is "Applications/System". Hint: This is not used for
> anything, so does not matter at all. It can even be omitted since Fedora 10.
> 
> 6.) .desktop entry
> 
> Please set the back to Category=System;Emulator from Category=Game;Emulator,
> which is the only right value in this case. I believe we've have communicated
> this with Andrea via private mail back then.  

Yes, thats done.  I have also changed "linux" to Linux" :) The summary is
accurate though, dosemu doesn't build on other platforms, there used to be a
port to netbsd but that was a long time ago.  

I have uploaded the latest RPM packages and spec file here:
http://jzygmont.fedorapeople.org/dosemu.spec

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