Bug 485418 - Review Request: vgabios - vga option rom for bochs/qemu
Summary: Review Request: vgabios - vga option rom for bochs/qemu
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Peter Lemenkov
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-02-13 14:33 UTC by Glauber Costa
Modified: 2009-03-07 15:31 UTC (History)
8 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-03-07 15:31:46 UTC
Type: ---
Embargoed:
lemenkov: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Glauber Costa 2009-02-13 14:33:16 UTC
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 15:13:44 UTC
> %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 15:17:09 UTC
> %install

rm -rf $RPM_BUILD_ROOT

http://fedoraproject.org/wiki/Packaging/Guidelines#Prepping_BuildRoot_For_.25install

Comment 3 Eduardo Habkost 2009-02-13 15:18:44 UTC
> %doc

Uh?

Comment 4 Glauber Costa 2009-02-13 15:21:47 UTC
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 15:25:10 UTC
Reviewers: see bug #464621 for a review of similar package (etherboot)

Comment 7 Eduardo Habkost 2009-02-13 16:21:33 UTC
%changelog seems to be in reverse order.  :)

Comment 8 Peter Lemenkov 2009-02-14 11:39:52 UTC
I'll review it.

Comment 9 Peter Lemenkov 2009-02-14 19:09:33 UTC
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 19:28:13 UTC
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 19:37:53 UTC
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 19:53:29 UTC
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 20:31:23 UTC
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 07:38:18 UTC
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> - 0.6.0.3

with

* Mon Feb 16 2009 Glauber Costa <glommer> - 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 19:18:00 UTC
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 20:50:09 UTC
Glauber: Any reply to comment #16?

Comment 19 Glauber Costa 2009-03-02 16:20:55 UTC
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-03 00:25:49 UTC
I don't see virt-maint in the list of cc lists we can add. 

cvs done otherwise.

Comment 21 Eduardo Habkost 2009-03-03 00:31:55 UTC
(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. Can it be added?

Comment 22 Toshio Ernie Kuratomi 2009-03-03 20:01:29 UTC
(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. Can it be added?

I can do that.  How is this different from xen-maint?  Should I just replace that with this one or do they exist for separate purposes?

Comment 23 Toshio Ernie Kuratomi 2009-03-03 20:06:19 UTC
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 20:11:11 UTC
(In reply to comment #22)
> > 
> > It's fedora-virt-maint. Can it be added?
> 
> I can do that.  How is this different from xen-maint?  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 20:35:31 UTC
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 16:03:48 UTC
(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 mailing list keep working, I think you can go ahead change it to the safer setup.

Comment 27 Kevin Fenzi 2009-03-05 20:11:31 UTC
Clearning cvs flag here.


Note You need to log in before you can comment on or make changes to this bug.