Bug 553706 - Review Request: seabios - Open-source legacy BIOS implementation
Summary: Review Request: seabios - Open-source legacy BIOS implementation
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Bill Nottingham
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: DebugInfo 566014
TreeView+ depends on / blocked
 
Reported: 2010-01-08 17:49 UTC by Justin M. Forbes
Modified: 2014-03-17 03:21 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
: 566014 (view as bug list)
Environment:
Last Closed: 2010-02-16 21:58:47 UTC
Type: ---
Embargoed:
notting: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)
demo spec (783 bytes, text/plain)
2010-01-08 20:46 UTC, Dan Horák
no flags Details

Description Justin M. Forbes 2010-01-08 17:49:42 UTC
Spec URL: http://www.linuxtx.org/rpms/seabios/seabios.spec
SRPM URL: http://www.linuxtx.org/rpms/seabios/seabios-0.5.1-0.1.20100108git669c991.fc12.src.rpm
Description: 

SeaBIOS is an open-source legacy BIOS implementation which can be used as
a coreboot payload. It implements the standard BIOS calling interfaces
that a typical x86 proprietary BIOS implements.

Note that seabios is required for kvm 0.12 and later to function.  It has replaced bochs for kvm.

Comment 1 Dan Horák 2010-01-08 19:28:30 UTC
If I am not missing something then the package is buildable only on x86/x86_64 host. So it can't have BuildArch: noarch and some tricks will be required to produce the noarch binary rpm. Maybe a noarch subpackage containing the BIOS image and main package without the files section ...

Comment 2 Bill Nottingham 2010-01-08 20:11:41 UTC
MUST items:
- Package meets naming and packaging guidelines - OK
- Spec file matches base package name. - OK
- Spec has consistant macro usage. - OK
- Meets Packaging Guidelines. - OK
- License - OK (LGPLv3)
- License field in spec matches - OK
- License file included in package - OK. Also includes GPL just for the heck of it.
- Spec in American English - OK
- Spec is legible. - OK
- Sources match upstream md5sum: - ***

This is a git repo, so hard to test. Spec should follow the convention on
http://fedoraproject.org/wiki/Packaging:SourceURL

- Package needs ExcludeArch - ***

Even if it's a noarch package, it should have ExclusiveArch set so we don't bother including it in ppc (or other) trees.

Moreover, Dan's comment stands on how it builds. Might just be simplest to add the ExclusiveArch and remove the BuildArch.

- BuildRequires correct - ***

Doesn't appear to actually need python-devel.

- Spec handles locales/find_lang - N/A
- Package is relocatable and has a reason to be. - N/A
- Package has %defattr and permissions on files is good.  - OK
- Package has a correct %clean section. - OK
- Package has correct buildroot - OK
%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
- Package is code or permissible content. - OK
- Doc subpackage needed/used. - N/A
- Packages %doc files don't affect runtime. - N/A

- Package compiles and builds on at least one arch. - OK (tested x86_64)
- Package has no duplicate files in %files. - OK
- Package doesn't own any directories other packages own. - OK
- Package owns all the directories it creates. - ***

Needs to own %{_datadir}/seabios

- No rpmlint output. - OK
- final provides and requires are sane: ***

Does this need to obsolete/provide bochs-bios?

SHOULD Items:

- Should build in mock. - did not test
- Should build on all supported archs - did not test
- Should function as described. - did not test
- Should have sane scriptlets. - N/A
- Should have dist tag - OK
- Should package latest version - I suppose git from today is latest enough.

Comment 3 Dan Horák 2010-01-08 20:46:22 UTC
(In reply to comment #2)
> - Package needs ExcludeArch - ***
> 
> Even if it's a noarch package, it should have ExclusiveArch set so we don't
> bother including it in ppc (or other) trees.
> 
> Moreover, Dan's comment stands on how it builds. Might just be simplest to add
> the ExclusiveArch and remove the BuildArch.

Having the binary rpm as noarch would allow secondary architectures to import the resulting rpms into their kojis and have installable qemu in their repositories. The ExclusiveArch makes sense IMHO for RHEL not in Fedora (no arch besides x86/x86_64). See attachment for a demo spec.

Comment 4 Dan Horák 2010-01-08 20:46:53 UTC
Created attachment 382548 [details]
demo spec

Comment 5 Justin M. Forbes 2010-01-08 21:12:10 UTC
I can go either way.  There is benefit in having the package noarch because things besides kvm can actually use the bios image.  That said, having an empty package to facilitate this seems really hackish too.

For the python-devel buildrequires, it is actually needed.  Testing in mock showed that it would not build without python-devel.  The compiler test script uses it.

It does not make sense to obsolete bochs-bios or provide it.  While kvm 0.12 and newer will only work with seabios, there are other uses for bochs-bios in Fedora {probably not in RHEL}.  Both rpms can be installed side by side, and once this package is in the repository kvm will be updated to use and require seabios.

Builds were tested in mock for x86_64 and x86.

I have a spec update to include the source conventions and directory ownership, just want consensus on the arch issue before it is pushed.  FWIW, vgabios is built in a similar fashion, with buildarch: noarch

Comment 6 Bill Nottingham 2010-01-08 21:20:00 UTC
(In reply to comment #5)
> For the python-devel buildrequires, it is actually needed.  Testing in mock
> showed that it would not build without python-devel.  The compiler test script
> uses it.

Fairly sure that just requires python, not python-devel?

> It does not make sense to obsolete bochs-bios or provide it.  While kvm 0.12
> and newer will only work with seabios, there are other uses for bochs-bios in
> Fedora {probably not in RHEL}.  Both rpms can be installed side by side, and
> once this package is in the repository kvm will be updated to use and require
> seabios.

OK.

> I have a spec update to include the source conventions and directory ownership,
> just want consensus on the arch issue before it is pushed.  FWIW, vgabios is
> built in a similar fashion, with buildarch: noarch  

The problem is, as it is now, you're going to run into problems building it on a koji-type system that has non-x86 architectures; it will get assigned to non-x86 builders and fail.

Comment 7 Justin M. Forbes 2010-01-08 23:16:51 UTC
Spec and srpm have been updated in same location:

- Source has been commented to include command to regenerate the tarball from the git tree

- Changed BuildArch: noarch to ExclusiveArch: %{ix86} x86_64
It made more sense to limit the arch to the actual users of kvm for the moment than to create a dead and worthless package to track just to work around current koji limitations

- Changed BuildRequires: python-devel to BuildRequires: python
(thanks for the catch there)

- Added %{_datadir}/seabio to %files

- Clean up on the Makefile sed, add comment explaining why it is done, and remove the unnecessary cp.

Comment 8 Bill Nottingham 2010-01-11 17:52:33 UTC
%dir %{_datadir}/seabios/
..
%{_datadir}/seabios

Need one or the other, not both. (Did I miss the %dir before.)

Aside from that, looks fine. APPROVED.

Comment 9 Justin M. Forbes 2010-01-11 17:56:51 UTC
Yes, missed the other from before, I thought they had the same result until you mentioned it.  No problem, will remove the %{_datadir}/seabios

Comment 10 Justin M. Forbes 2010-01-11 18:04:21 UTC
New Package CVS Request
=======================
Package Name: seabios
Short Description: Open-source legacy BIOS implementation
Owners: jforbes
Branches: F-12 
InitialCC: virtmaint

Comment 11 Jason Tibbitts 2010-01-12 06:27:11 UTC
CVS done (by process-cvs-requests.py)

Comment 12 Ville Skyttä 2010-01-13 18:34:54 UTC
seabios-debuginfo is empty, $RPM_OPT_FLAGS are not used, and things appear to be explicitly stripped during build before find-debuginfo.sh has a chance of doing its job.  At least V=1 should be added to the make line in %build to make these apparent from build logs.

Maybe this package is special enough so usual -debuginfo stuff is not applicable (and if not, -debuginfo should be explicitly disabled).  But not honoring $RPM_OPT_FLAGS needs a comment in the specfile in case it's intentional, See bug 496968 for more info.

Comment 13 Bill Nottingham 2010-02-16 21:58:47 UTC
Bug cloned for packaging issues; initial review bug closed.


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