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 198928
Summary: | Review Request: lsscsi | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Chip Coldwell <coldwell> |
Component: | Package Review | Assignee: | Jason Tibbitts <j> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | coughlan, cweyl, dan |
Target Milestone: | --- | Flags: | 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: | 2006-07-26 14:53:21 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: | 163779 |
Description
Chip Coldwell
2006-07-14 19:24:55 UTC
Are you sponsored into extras yet (as I'm not seeing you on the cvsextras page). If not, you should mark this bug as blocking FE-NEEDSPONSOR. A couple thoughts on the spec, off the top of my head: * The macros you have defined at the top are rather redundant, as you use them to then populate the Name:, Version:, Release: tags, all of which (when populated) define macros of the same names you're using containing the exact same information. * You're using both, e.g., $RPM_BUILD_ROOT and %{buildroot}. Either or is fine, but the packaging guidelines require us to be consistent within a specfile (this is a "MUST"). * In build, why not use "%configure"? Similarly, in install, why not "%makeinstall"? Both of those macros should take care of passing the information you're doing manually. (Also, it would quiet rpmlint about "E: lsscsi configure-without-libdir-spec".) * There may be comments on the test before "rm -rf ..." in %clean. I think it's ok, as it will always evaluate to true in the buildsys, but it could be nixed for brevity's sake. That being said, I've built and installed this package on my system. Works nicely for me, even against my SATA drives. (In reply to comment #1) > * In build, why not use "%configure"? Similarly, in install, why not > "%makeinstall"? %makeinstall is deprecated. See the "Macros" section of the Packaging Guidelines. http://fedoraproject.org/wiki/Packaging/Guidelines > * There may be comments on the test before "rm -rf ..." in %clean. I think it's > ok, as it will always evaluate to true in the buildsys, but it could be nixed > for brevity's sake. Not only that but the tests are completely redundant since a buildroot is explicitly specified in the spec file. (In reply to comment #1) > Are you sponsored into extras yet (as I'm not seeing you on the cvsextras page). > If not, you should mark this bug as blocking FE-NEEDSPONSOR. > > A couple thoughts on the spec, off the top of my head: > > * The macros you have defined at the top are rather redundant, as you use them > to then populate the Name:, Version:, Release: tags, all of which (when > populated) define macros of the same names you're using containing the exact > same information. > > * You're using both, e.g., $RPM_BUILD_ROOT and %{buildroot}. Either or is fine, > but the packaging guidelines require us to be consistent within a specfile (this > is a "MUST"). > > * In build, why not use "%configure"? Similarly, in install, why not > "%makeinstall"? Both of those macros should take care of passing the > information you're doing manually. (Also, it would quiet rpmlint about "E: > lsscsi configure-without-libdir-spec".) > > * There may be comments on the test before "rm -rf ..." in %clean. I think it's > ok, as it will always evaluate to true in the buildsys, but it could be nixed > for brevity's sake. > > That being said, I've built and installed this package on my system. Works > nicely for me, even against my SATA drives. Good points, thanks. Just to clarify, the spec file comes from the project homepage; I just touched it up to silence some rpmlint complaints. However, it looks like it wasn't enough, so I'll incorporate your recommendations. lsscsi is a very desirable addition since /proc/scsi/scsi runs out of memory if there are a large number of LUNs (>512, I think), and any kernel patch submitted to fix this problem is rejected (/proc/scsi/scsi is deprecated) and the submitter is referred to lsscsi: http://marc.theaimsgroup.com/?l=linux-kernel&m=111287017419578&w=2 Chip I updated the spec file and src.rpm to incorporate your recommendations; they're still in the same place: http://people.redhat.com/coldwell/lsscsi.spec http://people.redhat.com/coldwell/lsscsi-0.17-2.src.rpm (don't know if I should have bumped the release number or not). Chip Spec file looks much cleaner, and builds in mock (5&devel/x86_64). And yes, always bump the release number :) Chip, I'll sponsor you, now that Chris has done all of the work. rpmnlint has one complaint about the spec: W: lsscsi mixed-use-of-spaces-and-tabs cat -T shows this coming from spaces after the colons in Summary:, Name:, Version:, and Release:. Terribly minor, I know, but there's no reason not to fix it. (You only see this if you run rpmlint against the src.rpm.) I guess it's pretty pointless for a package with one source file, but generally folks with multiprocessor machines appreciate it when you add %{?_smp_mflags} to your make line to build things in parallel. As long as it doesn't break the build, of course. (And if it does break the build, add a comment to indicate that fact.) The only other thing is the oddity of including the author information at the end of %description. Then again, it would be somewhat uncouth to grab his specfile and then cut his name out of it, so I'm not sure what to do. I'll leave it up to you. With only the space/tab thing to fix, I'll go ahead and approve this and you can fix it when you check in. Review: * source files match upstream: c05c1cc6e6c425d86bf41e2ab0f09172 lsscsi-0.17.tgz * package meets naming and packaging guidelines. * specfile is properly named, is cleanly written and uses macros consistently. * dist tag is present. * build root is correct. * license field matches the actual license. * license is open source-compatible. License text included in package. * latest version is being packaged. * BuildRequires are proper. * Compiler flags are appropriate. * %clean is present. * package builds in mock (development, x86_64). X rpmlint is silent (minor tabs/spaces thing) * debuginfo package looks complete. * final provides and requires are sane: lsscsi = 0.17-2 = libc.so.6()(64bit) * %check is not present; no test suite upstream. * no shared libraries are present. * package is not relocatable. * owns the directories it creates. * doesn't own any directories it shouldn't. * no duplicates in %files. * file permissions are appropriate. * no scriptlets present. * code, not content. * documentation is small, so no -docs subpackage is necessary. (Technically the documentation is larger then the executable, but we're talking about 25K total here.) * %docs are not necessary for the proper functioning of the package. * no headers. * no pkgconfig files. * no libtool .la droppings. * not a GUI app. APPROVED Go ahead and request cvsextras and fedorabugs access and I'll approve you. See http://fedoraproject.org/wiki/Extras/Contributors#GetAFedoraAccount for details. OK, I replaced the tabs with spaces and added the SMP flags to the %make macro. The new spec/srpm are in the same place: http://people.redhat.com/coldwell/lsscsi.spec http://people.redhat.com/coldwell/lsscsi-0.17-2.src.rpm I also did the Fedora rigamarole -- username 'coldwell'. Thanks, Chip You should be all set up now; let me know if you have any trouble getting things imported and built. http://fedoraproject.org/wiki/Extras/NewPackageProcess step 9 is failing; the owners CVS module doesn't seem to exist CVSROOT is :gserver:cvs.devel.redhat.com:/cvs/dist Chip That CVSROOT is wrong for FE. See http://fedoraproject.org/wiki/Extras/UsingCvsFaq I was using CVS with the Fedora CVSROOT yesterday, but today I'm locked out: $ export CVSROOT=:ext:USERNAME.redhat.com:/cvs/extras $ export CVS_RSH=ssh $ cvs co lsscsi For more information on using the Fedora source code repositories, please visit http://fedoraproject.org/wiki/UsingCvs Permission denied (publickey,keyboard-interactive). cvs [checkout aborted]: end of file from server (consult above messages if any) Dumb question, I know, but can I assume that you're not actually using "USERNAME" there? (In reply to comment #12) > Dumb question, I know, but can I assume that you're not actually using > "USERNAME" there? Ooops. The hazards of copy/paste from the FAQ. Thanks. Chip (In reply to comment #13) > (In reply to comment #12) > > Dumb question, I know, but can I assume that you're not actually using > > "USERNAME" there? > > Ooops. The hazards of copy/paste from the FAQ. Thanks. I take it back: $ cvs co lsscsi For more information on using the Fedora source code repositories, please visit http://fedoraproject.org/wiki/UsingCvs Permission denied (publickey,keyboard-interactive). cvs [checkout aborted]: end of file from server (consult above messages if any) $ set | grep CVS CVSROOT=:ext:coldwell.redhat.com:/cvs/extras CVS_RSH=ssh I don't know, honestly. Perhaps the public key on that machine doesn't match what you sent? Date: Wed, 26 Jul 2006 10:55:06 -0400 (EDT) From: buildsys To: coldwell Subject: Build Result: 13160 - lsscsi on fedora-development-extras 13160 (lsscsi): Build on target fedora-development-extras succeeded. Build logs may be found at http://buildsys.fedoraproject.org/logs/fedora-development-extras/13160-lsscs i-0.17-2/ http://brewweb.devel.redhat.com/brew/taskinfo?taskID=135258 emacs-21.4-15 built. Ooops, wrong bug. Package Change Request ====================== Package Name: lsscsi New Branches: EL-4 EL-5 Owners: sharkcz cvs done. |