Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 553706
Summary: | Review Request: seabios - Open-source legacy BIOS implementation | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Justin M. Forbes <jforbes> | ||||
Component: | Package Review | Assignee: | Bill Nottingham <notting> | ||||
Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | medium | ||||||
Version: | rawhide | CC: | dan, fedora-package-review, notting, rvokal, ville.skytta | ||||
Target Milestone: | --- | Flags: | notting:
fedora-review+
j: fedora-cvs+ |
||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | |||||||
: | 566014 (view as bug list) | Environment: | |||||
Last Closed: | 2010-02-16 21:58:47 UTC | Type: | --- | ||||
Regression: | --- | Mount Type: | --- | ||||
Documentation: | --- | CRM: | |||||
Verified Versions: | Category: | --- | |||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
Cloudforms Team: | --- | Target Upstream Version: | |||||
Embargoed: | |||||||
Bug Depends On: | |||||||
Bug Blocks: | 496968, 566014 | ||||||
Attachments: |
|
Description
Justin M. Forbes
2010-01-08 17:49:42 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 ... 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. (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. Created attachment 382548 [details]
demo spec
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 (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. 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. %dir %{_datadir}/seabios/ .. %{_datadir}/seabios Need one or the other, not both. (Did I miss the %dir before.) Aside from that, looks fine. APPROVED. Yes, missed the other from before, I thought they had the same result until you mentioned it. No problem, will remove the %{_datadir}/seabios New Package CVS Request ======================= Package Name: seabios Short Description: Open-source legacy BIOS implementation Owners: jforbes Branches: F-12 InitialCC: virtmaint CVS done (by process-cvs-requests.py) 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. Bug cloned for packaging issues; initial review bug closed. |