Bug 485420
| Summary: | Review Request: openbios - Open Source implementation of IEEE 1275-1994 | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Glauber Costa <gcosta> |
| Component: | Package Review | Assignee: | Peter Lemenkov <lemenkov> |
| Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | ehabkost, fedora-package-review, lemenkov, markmc, notting, 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 15:31:12 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: | |||
|
Description
Glauber Costa
2009-02-13 14:35:35 UTC
> %files > %defattr(-,root,root,-) > %dir %{_datadir}/openbios/openbios-sparc32 > %dir %{_datadir}/openbios/openbios-sparc64 > #%dir %{_datadir}/openbios/openbios-ppc Shouldn't the package own the %{_datadir}/openbios directory? http://fedoraproject.org/wiki/Packaging/UnownedDirectories Reviewers: see bug #464621 for a review of similar package (etherboot) Updated: http://glommer.fedorapeople.org/openbios-1.0-0.2.svn450.fc11.src.rpm http://glommer.fedorapeople.org/openbios.spec I'll review it. Blocker - missing fcode utils as BuildReqires. You should consider packaging them also. http://www.openfirmware.info/FCODE_suite Ok, let's assume that you (or something else) already packaged fcode and you added proper BuildRequires into spec-file. Notes: * %prep section contains leftover (ls command) * This package must be marked as noarch * The same issue with %docs as in bz#485418 * I still think that shipping pre-build firmware is a bad idea. What prevents us to build them all from source on every arch? Peter, Fcode was not required to build the pieces I'm building so far. This is the reason I have not included it. But I can happily shoot a FCode package if you really feel necessary. As for the binaries, nobody likes them. Your suggestion in vgabios was totally welcome and accepted. But in that case, dev86 was compiling everything. In this package, (as well as bochs-bios and etherboot), gcc is doing a big part of the job. By purely gcc suckiness, we can't easily generate code for alternate platforms other than native. So I'm afraid we do have to build once, and then pack binaries in the way we did for etherboot. Please take a look at that package review for more information. Peter, please take a look at http://glommer.fedorapeople.org/openbios-1.0-0.3.svn463.fc11.src.rpm http://glommer.fedorapeople.org/openbios.spec I had a nice idea of how to explore koji's new capabilities, and this is the result. Because of the conditional architecture, openbios-xxx will only be built in the architecture it was targeted for, and will then span to all other targets. If you want to have a better picture of what I'm talking about, there are koji builds at: http://koji.fedoraproject.org/koji/taskinfo?taskID=1215501 (primary arches) http://sparc.koji.fedoraproject.org/koji//taskinfo?taskID=160708 (sparc) The only remaining issue, is the fact that sparc is a secondary architecture. And as of now, builds in secondary architectures don't reach primary ones. But this seems the best we can do now. From my PoV it;s still look ugly - I don't think that it's easy to understand that all these tricks with exactarch/noarch for w/o comments. In other words, I think that this package is not legible enough. However, keeping in mind that we really need this package in order to clean up mess with pre-built blobs in qemu here is my
REVIEW:
- rpmlint is not silent (checked on ppc):
openbios.src:137: E: files-attr-not-set
openbios.src:138: E: files-attr-not-set
openbios.src:139: E: files-attr-not-set
openbios.src: W: mixed-use-of-spaces-and-tabs (spaces: line 2, tab: line 10)
openbios.src: W: summary-ended-with-dot OpenBios implementation of IEEE 1275-1994.
openbios.src: E: description-line-too-long features, Open Firmware provides an instruction set independent device interface.
openbios-debuginfo.ppc: E: empty-debuginfo-package
openbios-ppc.noarch: W: no-documentation
openbios-ppc.noarch: E: arch-independent-package-contains-binary-or-object /usr/share/openbios/openbios-ppc
4 packages and 0 specfiles checked; 6 errors, 3 warnings.
+ 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. I've got only one complain about explicit installation of docs in %install section. I't enogh just to list them as %doc
+ 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, containing the text of the license(s) for the package, is included in %doc.
+ The spec file is written in American English.
- The spec file for the package is not legible enough (see messages above). unfortunately, I don't think that this can be easily fixed, because this situation is relatively complex (different parts of this package should be built on different arches).
- The sources used to build the package must match the upstream source, as provided in the spec URL. Unfortunately, I failed to download sources using the following address (taken from spec-file):
http://downloads.sourceforge.net/openbios/openbios-1.0.tar.gz
+ The package successfully compiles and builds into binary rpms on at least one primary architecture (see koji ogs above)
+ All build dependencies are listed in BuildRequires.
- The package must own all directories that it creates. Unfortunately, I can't find that sub-package owns %{_datadir}/openbios
+ A package does not contain any duplicate files in the %files listing.
- Permissions on files must be set properly. See rpmlint warnings above.
+ The package has a %clean section.
+ The package consistently uses macros.
+ The package contains code, or permissable content.
+ Everything, the package includes as %doc, does not affect the runtime of the application.
+ 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 rpm packages are valid UTF-8.
Please, fix issues, mentioned above.
Ok, uploaded a new version. http://glommer.fedorapeople.org/openbios-1.0-0.4.svn463.fc11.src.rpm http://glommer.fedorapeople.org/openbios.spec As for rpmlint complaining about binaries in noarch subpackages... There's nothing we can do here ;-) I also added a good amount of comments that might help people looking at it. Uploaded: http://glommer.fedorapeople.org/openbios-1.0-0.5.svn463.fc11.src.rpm http://glommer.fedorapeople.org/openbios.spec changing openbios-doc to openbios-common OK, all issues are fixed, so this package is APPROVED New Package CVS Request ======================= Package Name: openbios Short Description: OpenBios implementation of IEEE 1275-1994 Owners: glommer Branches: InitialCC: fedora-virt-maint cvs done. |