Bug 485418
Summary: | Review Request: vgabios - vga option rom for bochs/qemu | ||
---|---|---|---|
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: | a.badger, ehabkost, fedora-package-review, lemenkov, markmc, notting, rjones, 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:46 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:33:16 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?
> %install rm -rf $RPM_BUILD_ROOT http://fedoraproject.org/wiki/Packaging/Guidelines#Prepping_BuildRoot_For_.25install > %doc
Uh?
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. Reviewers: see bug #464621 for a review of similar package (etherboot) Ok, Updated it. You can grab: http://glommer.fedorapeople.org/vgabios-0.6-0.2beta.fc11.src.rpm http://glommer.fedorapeople.org/vgabios.spec %changelog seems to be in reverse order. :) I'll review it. 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]$ 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. 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 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 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. 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 Ok, I addressed your issues: http://glommer.fedorapeople.org/vgabios.spec http://glommer.fedorapeople.org/vgabios-0.6-0.4beta.fc11.src.rpm 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. Glauber: Any reply to comment #16? hope it is okay now. you can grab: http://glommer.fedorapeople.org/vgabios.spec http://glommer.fedorapeople.org/vgabios-0.6-0.5.b.fc11.src.rpm 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 I don't see virt-maint in the list of cc lists we can add. cvs done otherwise. (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? (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? 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.... (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. 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. (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. Clearning cvs flag here. |