Bug 485418

Summary: Review Request: vgabios - vga option rom for bochs/qemu
Product: [Fedora] Fedora Reporter: Glauber Costa <gcosta>
Component: Package ReviewAssignee: Peter Lemenkov <lemenkov>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: a.badger, ehabkost, fedora-package-review, lemenkov, markmc, notting, rjones, virt-maint
Target Milestone: ---Flags: lemenkov: fedora‑review+
kevin: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-03-07 10:31:46 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:

Description Glauber Costa 2009-02-13 09:33:16 EST
Spec URL: http://glommer.fedorapeople.org/vgabios.spec
SRPM URL: http://glommer.fedorapeople.org/vgabios-0.6-0.1beta.fc11.src.rpm
Description:
vgabios is an LPGL implementation of a bios for a video card.
It is tied to plex86/bochs, althoug it will likely work on other
emulators. It is not intended for use in real cards.
Comment 1 Eduardo Habkost 2009-02-13 10:13:44 EST
> %files
> %defattr(-,root,root,-)
> %{_datadir}/vgabios/VGABIOS-lgpl-latest.bin
> %{_datadir}/vgabios/VGABIOS-lgpl-latest.cirrus.bin
> %{_datadir}/vgabios/VGABIOS-lgpl-latest.cirrus.debug.bin
> %{_datadir}/vgabios/VGABIOS-lgpl-latest.debug.bin

Your package also owns %{_datadir}/vgabios, doesn't it?
Comment 2 Eduardo Habkost 2009-02-13 10:17:09 EST
> %install

rm -rf $RPM_BUILD_ROOT

http://fedoraproject.org/wiki/Packaging/Guidelines#Prepping_BuildRoot_For_.25install
Comment 3 Eduardo Habkost 2009-02-13 10:18:44 EST
> %doc

Uh?
Comment 4 Glauber Costa 2009-02-13 10:21:47 EST
1) yeah, we do own vgabios directory. I now remember having this problem with etherboot too, but forgot about it. 

2) Forgot again

3) Got from a template, slipped through my fingers.

I'll update the packages.
Comment 5 Mark McLoughlin 2009-02-13 10:25:10 EST
Reviewers: see bug #464621 for a review of similar package (etherboot)
Comment 7 Eduardo Habkost 2009-02-13 11:21:33 EST
%changelog seems to be in reverse order.  :)
Comment 8 Peter Lemenkov 2009-02-14 06:39:52 EST
I'll review it.
Comment 9 Peter Lemenkov 2009-02-14 14:09:33 EST
Blocker - it's a bad, bad idea, to use %{release} in source's naming scheme:

[petro@Sulaco SPECS]$ LANG=C rpmbuild -ba vgabios.spec 
Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.6Ptmxi
+ umask 022
+ cd /home/petro/rpmbuild/BUILD
+ cd /home/petro/rpmbuild/BUILD
+ rm -rf vgabios-0.6
+ /usr/bin/gzip -dc /home/petro/rpmbuild/SOURCES/vgabios-0.6.tgz
+ /bin/tar -xf -
+ STATUS=0
+ '[' 0 -ne 0 ']'
+ cd vgabios-0.6
+ /bin/chmod -Rf a+rX,u+w,g-w,o-w .
+ mkdir prebuilt
+ cd prebuilt
+ tar -zxf /home/petro/rpmbuild/SOURCES/vgabios-binaries-0.6-0.2beta.fc10.tar.gz
tar: /home/petro/rpmbuild/SOURCES/vgabios-binaries-0.6-0.2beta.fc10.tar.gz: Cannot open: No such file or directory
tar: Error is not recoverable: exiting now
tar: Child returned status 2
tar: Error exit delayed from previous errors
error: Bad exit status from /var/tmp/rpm-tmp.6Ptmxi (%prep)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.6Ptmxi (%prep)
[petro@Sulaco SPECS]$
Comment 10 Peter Lemenkov 2009-02-14 14:28:13 EST
I don't like the idea with shipping pre-built blobs. Please, explain, why we can't compile dev86 for non-x86 arches?

http://cvs.fedoraproject.org/viewvc/rpms/dev86/

I just commented out string with ExclusiveArch and it at least builds on my PowerPC. Will try to assembly this firmware with it.
Comment 11 Peter Lemenkov 2009-02-14 14:37:53 EST
Ok, I successed in building it on my Mac Mini G4. However, I don't know how to test these blobs :).

Anyway, here are my suggestions:

* Provide dev86 for all arches (not only for x86/x86_64)
* Mark this package as noarch
Comment 12 Peter Lemenkov 2009-02-14 14:53:29 EST
Few more notes:

* No need to install %docs. It's  enough to include them in %files, e.g. instead of

mkdir -p $RPM_BUILD_ROOT%{_docdir}/vgabios
install -m 0644 README COPYING $RPM_BUILD_ROOT%{_docdir}/vgabios 
...
%files
...
%docdir %{_docdir}/vgabios/
%doc %{_docdir}/vgabios/README
%doc %{_docdir}/vgabios/COPYING


you should write only 

%files
...
%doc README COPYING


* Source0 file doesn't match one, shipped within srpm:

[petro@Sulaco SOURCES]$ md5sum vgabios-0.6.tgz ~/Desktop/vgabios-0.6b.tgz
35ddf3eaf16abe546797ae9cc18cbd6e  vgabios-0.6.tgz
36399621c4d6753e83a3cec3009c7183  /home/petro/Desktop/vgabios-0.6b.tgz
[petro@Sulaco SOURCES]$

Generally, it's not a problem if you need to ship different (from upstream) source, but in case then you *really* need to modify upstream sources (or even create tarball from VCS), you *must* provide instructions on how to create exact the same source file in comments to your spec-file. See this spec-file as an example:

http://cvs.fedoraproject.org/viewvc/rpms/flashrom/devel/flashrom.spec?view=markup
Comment 13 Glauber Costa 2009-02-16 15:31:23 EST
Okay, I've updated the package to address your concerns. Hopefully I'm covering all of them

grab them at 

http://glommer.fedorapeople.org/vgabios-0.6-0.3beta.fc11.src.rpm
http://glommer.fedorapeople.org/vgabios.spec

Note that koji builds will fail on ppc64. This is due to a dev86 bug, to which I've already submitted the patch.
Comment 14 Peter Lemenkov 2009-02-17 02:38:18 EST
REVIEW:

- rpmlint is not silent:

[petro@Sulaco rpmbuild]$ rpmlint RPMS/noarch/vgabios-0.6-0.3beta.fc10.noarch.rpm 
vgabios.noarch: W: non-standard-group Application/Emulators
vgabios.noarch: W: incoherent-version-in-changelog 0.6.0.3 ['0.6-0.3beta.fc10', '0.6-0.3beta']
1 packages and 0 specfiles checked; 0 errors, 2 warnings.
[petro@Sulaco rpmbuild]$

These two warnings are easy-to-fix. First, you accidentally made a typo - non Application/Emulators, but Applications/Emulators (see /usr/share/doc/rpm-4.6.0/GROUPS ). Second, just use correct EVR in %changelog, e.g. replace 

* Mon Feb 16 2009 Glauber Costa <glommer@redhat.com> - 0.6.0.3

with

* Mon Feb 16 2009 Glauber Costa <glommer@redhat.com> - 0.6-0.3beta

+ The package is named according to the Package Naming Guidelines .
+ The spec file name matches the base package %{name}, in the format %{name}.spec.

- The package meets the Packaging Guidelines, and I have only one small advice. This string (looks like leftover) should be removed:

mkdir -p $RPM_BUILD_ROOT%{_docdir}/vgabios

+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.
+ The License field in the package spec file matches the actual license.
+ The file with text of the license is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package matches the upstream source, as provided in the spec URL. 

[petro@Sulaco SOURCES]$ md5sum vgabios-0.6b.tgz*
36399621c4d6753e83a3cec3009c7183  vgabios-0.6b.tgz
36399621c4d6753e83a3cec3009c7183  vgabios-0.6b.tgz.from_srpm
[petro@Sulaco SOURCES]$

+ The package successfully compiles and builds into binary rpms on at least one primary architecture (my ppc)
+ All build dependencies are listed in BuildRequires.
+ No need to handle locales.
+ No shared libraries.
+ The package owns all directories that it creates.
+ The package does not contain any duplicate files in the %files listing.
+ Permissions on files are set properly.
+ The package has a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ The package consistently use macros.
+ The package contains code, or permissible content.
+ No large documentation files.
+ The stuff, that the package includes as %doc, does not affect the runtime of the application.
+ No header files.
+ No static libraries.
+ No pkgconfig(.pc) files.
+ No library files with a suffix
+ No .la libtool archives
+ Not a GUI app
+ The package does not own files or directories already owned by other packages.
+ At the beginning of %install, the package runs rm -rf %{buildroot} (or $RPM_BUILD_ROOT).
+ All filenames in the package must be valid UTF-8.

Assuming, that you fixed three issues, described above (two from rpmlint and one leftover), and that buildroot already contains latest dev86 rpm, this package is 


APPROVED
Comment 16 Kevin Fenzi 2009-02-18 14:18:00 EST
Can you take a look at: 
http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages

I think you want 0.6-0.1.b or 0.6-0.1.beta as your verson? Or does upstream have different beta versions? I only see the b tag.
Comment 17 Kevin Fenzi 2009-02-24 15:50:09 EST
Glauber: Any reply to comment #16?
Comment 19 Glauber Costa 2009-03-02 11:20:55 EST
New Package CVS Request
=======================
Package Name: vgabios
Short Description: an LPGL implementation of a bios for a video card.
Owners: glommer
Branches: F-11
InitialCC: virt-maint
Comment 20 Kevin Fenzi 2009-03-02 19:25:49 EST
I don't see virt-maint in the list of cc lists we can add. 

cvs done otherwise.
Comment 21 Eduardo Habkost 2009-03-02 19:31:55 EST
(In reply to comment #20)
> I don't see virt-maint in the list of cc lists we can add. 

It's fedora-virt-maint@redhat.com. Can it be added?
Comment 22 Toshio Ernie Kuratomi 2009-03-03 15:01:29 EST
(In reply to comment #21)
> (In reply to comment #20)
> > I don't see virt-maint in the list of cc lists we can add. 
> 
> It's fedora-virt-maint@redhat.com. Can it be added?

I can do that.  How is this different from xen-maint@redhat.com?  Should I just replace that with this one or do they exist for separate purposes?
Comment 23 Toshio Ernie Kuratomi 2009-03-03 15:06:19 EST
hmm... and looking at the mailing list, it looks like there is an account for this already -- Someone seems to have set up "virtmaint" to go there.

This is a tiny bit dangerous as it's a real account, not one of our pseudo accounts.  So it will be subject to account expiration, someone could compromise its password, etc....
Comment 24 Eduardo Habkost 2009-03-03 15:11:11 EST
(In reply to comment #22)
> > 
> > It's fedora-virt-maint@redhat.com. Can it be added?
> 
> I can do that.  How is this different from xen-maint@redhat.com?  Should I just
> replace that with this one or do they exist for separate purposes?

xen-maint is used for different purposes. Please keep xen-maint as is.
Comment 25 Toshio Ernie Kuratomi 2009-03-03 15:35:31 EST
Okay.  I've added virtmaint on the CC.  Do you know who created/is responsible for virtmaint?  I need to get it straightened out if it should be a pseudo account or we're likely to have problems down the line.
Comment 26 Eduardo Habkost 2009-03-04 11:03:48 EST
(In reply to comment #25)
> Okay.  I've added virtmaint on the CC.  Do you know who created/is responsible
> for virtmaint?  I need to get it straightened out if it should be a pseudo
> account or we're likely to have problems down the line.

Mark McLoughlin probably created it, but he is away for a few weeks. But if the existing notifications and the fedora-virt-maint@redhat.com mailing list keep working, I think you can go ahead change it to the safer setup.
Comment 27 Kevin Fenzi 2009-03-05 15:11:31 EST
Clearning cvs flag here.