This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 485417 - Review Request: bochs-bios - bios implementation from the bochs project
Review Request: bochs-bios - bios implementation from the bochs project
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Richard W.M. Jones
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 488421
  Show dependency treegraph
 
Reported: 2009-02-13 09:30 EST by Glauber Costa
Modified: 2009-03-25 10:32 EDT (History)
7 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-25 10:32:48 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
rjones: fedora‑review+


Attachments (Terms of Use)
Updated bochs-bios.spec (3.59 KB, text/plain)
2009-02-28 09:37 EST, Richard W.M. Jones
no flags Details
Patch to bochs-bios.spec from release 0.3 to release 0.6. (4.27 KB, patch)
2009-02-28 09:39 EST, Richard W.M. Jones
no flags Details | Diff

  None (edit)
Description Glauber Costa 2009-02-13 09:30:02 EST
Spec URL: http://glommer.fedorapeople.org/bochs-bios.spec
SRPM URL:
http://glommer.fedorapeople.org/bochs-bios-2.3.8-0.1.git36989b0d2.fc11.src.rpm
Description:
This is the BIOS to be used in the Bochs Project
Comment 1 Eduardo Habkost 2009-02-13 10:05:46 EST
> Summary:        Portable x86 PC emulator

Is bochs-bios a "portable x86 PC emulator"?

> Patch0:	0001_bx-qemu.patch

I hope this doesn't break any guidelines on naming patches. I think using the package name as a prefix for patch file names is good practice, even if not explicitly required by Fedora.
Comment 2 Eduardo Habkost 2009-02-13 10:09:37 EST
> mkdir -p prebuilt
> cd prebuilt
> tar -xzf %{SOURCE1}

You can probably use %setup here, with magic parameters to make it create prebuilt/ and unpack only %{SOURCE1} inside it.
Comment 3 Eduardo Habkost 2009-02-13 10:16:45 EST
> %install

rm -rf $RPM_BUILD_ROOT

http://fedoraproject.org/wiki/Packaging/Guidelines#Prepping_BuildRoot_For_.25install
Comment 4 Mark McLoughlin 2009-02-13 10:24:55 EST
Reviews: see bug #464621 for a review of similar package (etherboot)
Comment 5 Glauber Costa 2009-02-13 11:14:57 EST
wrt setup: Maybe we could use it, but what would it buy us?
the rpm directives seem to be quite arcane, I believe this is much cleaner.
Comment 7 Peter Lemenkov 2009-02-14 06:40:21 EST
I'll review it
Comment 8 Peter Lemenkov 2009-02-15 09:56:23 EST
404 while trying to download Source0 from SF. Please provide spec file with such obvious issues fixed.
Comment 9 Glauber Costa 2009-02-17 08:30:04 EST
Ok, I have uploaded new spec and SRPM
Please note that in this package, we don't see a way out of the binaries,
as it does not depend only on dev86. Part of it is compiled with gcc, aiming at the target platform. As we don't have a cross compilation infrastructure, there's not too much we can do.

spec: http://glommer.fedorapeople.org/bochs-bios.spec
srpm: 
http://glommer.fedorapeople.org/bochs-bios-2.3.8-0.3.git36989b0d2.fc11.src.rpm
Comment 10 Glauber Costa 2009-02-18 13:14:26 EST
Peter, comments on this one?
Comment 11 Richard W.M. Jones 2009-02-28 08:45:43 EST
Taking for review ...
Comment 12 Richard W.M. Jones 2009-02-28 09:35:28 EST
I patched the spec file (see comment 13), so this review
refers to the patched package.

Koji scratch build:

  http://koji.fedoraproject.org/koji/taskinfo?taskID=1208731

+ rpmlint output

bochs-bios.x86_64: E: no-binary

We can ignore this error - rpmlint doesn't recognise the binary.

+ package name satisfies the packaging naming guidelines
+ specfile name matches the package base name
+ package should satisfy packaging guidelines
+ license meets guidelines and is acceptable to Fedora
+ license matches the actual package license
  LGPLv2+
+ %doc includes license file
+ spec file written in American English
+ spec file is legible
+ upstream sources match sources in the srpm

  Verified by checking out Bochs sources from upstream git.

+ package successfully builds on at least one architecture
n/a ExcludeArch bugs filed
+ BuildRequires list all build dependencies
n/a %find_lang instead of %{_datadir}/locale/*
n/a binary RPM with shared library files must call ldconfig in %post and %postun
n/a does not use Prefix: /usr
+ package owns all directories it creates
+ no duplicate files in %files
+ %defattr line
+ %clean contains rm -rf $RPM_BUILD_ROOT
+ consistent use of macros
+ package must contain code or permissible content
n/a large documentation files should go in -doc subpackage
+ files marked %doc should not affect package
n/a header files should be in -devel
n/a static libraries should be in -static
n/a packages containing pkgconfig (.pc) files need 'Requires: pkgconfig'
n/a libfoo.so must go in -devel
n/a -devel must require the fully versioned base
n/a packages should not contain libtool .la files
n/a packages containing GUI apps must include %{name}.desktop file
n/a packages must not own files or directories owned by other packages
+ %install must start with rm -rf %{buildroot} etc.
+ filenames must be valid UTF-8

Optional:

n/a if there is no license file, packager should query upstream
n/a translations of description and summary for non-English languages, if available
+ reviewer should build the package in mock
+ the package should build into binary RPMs on all supported architectures
+ review should test the package functions as described
n/a scriptlets should be sane
n/a pkgconfig files should go in -devel
+ shouldn't have file dependencies outside /etc /bin /sbin /usr/bin or /usr/sbin
Comment 13 Richard W.M. Jones 2009-02-28 09:37:27 EST
Created attachment 333604 [details]
Updated bochs-bios.spec
Comment 14 Richard W.M. Jones 2009-02-28 09:39:00 EST
Created attachment 333605 [details]
Patch to bochs-bios.spec from release 0.3 to release 0.6.

This patch shows the differences between release 0.3
and release 0.6.

Note that you will have to rename %{SOURCE1} in your
SOURCES directory, because I removed the release part
from the name of this file.
Comment 15 Richard W.M. Jones 2009-02-28 09:39:47 EST
---------------------------
This package (release 0.6) is APPROVED by rjones
---------------------------
Comment 16 Peter Lemenkov 2009-02-28 10:10:46 EST
Unfortunately, the package in its current state doesn't change much in the current situation with prebuilt files in qemu rpm.. It just drops blobs into the main repository as qemu did before.

I tried to rebuild this package with different compilers (pcc, tinycc), but with no success so far. I still think that we should add i386-gcc on every non-x86 arch before submitting this title.
Comment 17 Richard W.M. Jones 2009-02-28 11:12:13 EST
I don't understand comment 16.  This package builds from source
on %{ix86} and x86_64.  It doesn't build from source on non-x86
builders, simply because they lack the required cross-compiler.

If Koji did something sensible with:
  ExclusiveArch: %{ix86} x86_64
  BuildArch: noarch
then we could even build properly on Koji and create a noarch
package.  The fact that it doesn't is just a bug in Koji.
Comment 18 Peter Lemenkov 2009-02-28 11:43:51 EST
If so, then you should open (if still not opened by someone) bug against koji and mark it as blocker for this ticket.
Comment 19 Glauber Costa 2009-03-02 07:57:53 EST
As far as I know, there is work in progress in koji to make the noarch trick possible.

But I don't think it is a blocker for this package. But be sure we'll switch to it as soon as koji supports it.

Please understand that currently, we ship binaries in qemu that we don't build _at all_. This is a big liability, so this package, while not perfect, is a _huge_ step forward.
Comment 20 Glauber Costa 2009-03-02 09:32:20 EST
New Package CVS Request
=======================
Package Name: bochs-bios
Short Description: bios for bochs/qemu projects
Owners: glommer
Branches: f-11
InitialCC: virt-maint
Comment 21 Glauber Costa 2009-03-02 14:32:46 EST
To totally settle down in the binaries issue, koji now finally supports
noarch subpackages.

I've uploaded a new:

http://glommer.fedorapeople.org/bochs-bios-2.3.8-0.4.git36989b0d2.fc11.src.rpm
http://glommer.fedorapeople.org/bochs-bios.spec

that uses no binaries at all.
Comment 22 Kevin Fenzi 2009-03-02 19:37:57 EST
cvs done (except for cc to virt-maint).
Comment 23 Glauber Costa 2009-03-03 10:00:26 EST
Can CC to fedora-virt-maint@redhat.com be late added? (since I got it wrong in the initial review)

I don't see a way to do it myself in packagedb.
Comment 24 Glauber Costa 2009-03-04 14:44:29 EST
Kevin,

We requested this package because it included pre-built binaries, and to comply with Fedora policies, we'd have to build it twice.

With koji recently added features, we can actually build bochs-bios from within bochs package as a noarch subpackage(done a scratch build of it today).

So given this, what is the preferred path? Get rid of the newly created rpm and use a bochs-bios package created from bochs? Or keep it, and have bochs to depend on it?

Appreciated.
Comment 25 Kevin Fenzi 2009-03-04 15:04:15 EST
Yeah, I would say if you can do it with an existing package we should just drop this one. 

It's up to you however, if you think it's good to seperate the two.
Comment 26 Glauber Costa 2009-03-05 14:29:57 EST
Should I go through the normal package EOL here, or can we do it an easier way since it was not in any release yet?
Comment 27 Toshio Ernie Kuratomi 2009-03-05 18:36:01 EST
Since packages have been built in koji (even though they haven't been released), I'd rather we go through the package EOL procedure.  Mark as dead.package, etc.
Comment 28 Mark McLoughlin 2009-03-25 08:17:12 EDT
glauber: any progress on EOL-ing this?
Comment 29 Glauber Costa 2009-03-25 10:11:00 EDT
To the best of my knowledge, this is EOLd already.
Comment 30 Mark McLoughlin 2009-03-25 10:32:48 EDT
Yeah, you're right:

http://cvs.fedoraproject.org/viewvc/devel/bochs-bios/

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