Bug 198928 - Review Request: lsscsi
Review Request: lsscsi
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT 194453
  Show dependency treegraph
 
Reported: 2006-07-14 15:24 EDT by Chip Coldwell
Modified: 2009-11-27 00:57 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-07-26 10:53:21 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Chip Coldwell 2006-07-14 15:24:55 EDT
Spec URL: http://people.redhat.com/coldwell/lsscsi.spec
SRPM URL: http://people.redhat.com/coldwell/lsscsi-0.17-2.src.rpm
Description: Uses information provided by the sysfs pseudo file system in Linux kernel 2.6 series to list SCSI devices or all SCSI hosts. Includes a "classic" option to mimic the output of "cat /proc/scsi/scsi" that has been widely used prior to the lk 2.6 series.
Comment 1 Chris Weyl 2006-07-14 21:53:38 EDT
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.
Comment 2 Paul Howarth 2006-07-15 09:41:45 EDT
(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.
Comment 3 Chip Coldwell 2006-07-17 09:46:06 EDT
(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
Comment 4 Chip Coldwell 2006-07-17 10:33:19 EDT
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
Comment 5 Chris Weyl 2006-07-20 02:30:42 EDT
Spec file looks much cleaner, and builds in mock (5&devel/x86_64). 

And yes, always bump the release number :)
Comment 6 Jason Tibbitts 2006-07-20 22:13:53 EDT
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.
Comment 7 Chip Coldwell 2006-07-24 10:42:56 EDT
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
Comment 8 Jason Tibbitts 2006-07-24 11:27:19 EDT
You should be all set up now; let me know if you have any trouble getting things
imported and built.
Comment 9 Chip Coldwell 2006-07-24 11:50:45 EDT
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
Comment 10 Ville Skyttä 2006-07-24 13:02:04 EDT
That CVSROOT is wrong for FE.  See http://fedoraproject.org/wiki/Extras/UsingCvsFaq
Comment 11 Chip Coldwell 2006-07-25 11:05:15 EDT
I was using CVS with the Fedora CVSROOT yesterday, but today I'm locked out:

$ export CVSROOT=:ext:USERNAME@cvs.fedora.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)
Comment 12 Jason Tibbitts 2006-07-25 11:12:10 EDT
Dumb question, I know, but can I assume that you're not actually using
"USERNAME" there?
Comment 13 Chip Coldwell 2006-07-25 11:14:32 EDT
(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
Comment 14 Chip Coldwell 2006-07-25 11:16:11 EDT
(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@cvs.fedora.redhat.com:/cvs/extras
CVS_RSH=ssh
Comment 15 Jason Tibbitts 2006-07-25 13:14:39 EDT
I don't know, honestly.  Perhaps the public key on that machine doesn't match
what you sent?
Comment 16 Chip Coldwell 2006-07-26 10:53:21 EDT
Date: Wed, 26 Jul 2006 10:55:06 -0400 (EDT)
From: buildsys@fedoraproject.org
To: coldwell@redhat.com
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/
Comment 17 Chip Coldwell 2006-07-26 13:48:19 EDT
http://brewweb.devel.redhat.com/brew/taskinfo?taskID=135258

emacs-21.4-15 built.
Comment 18 Chip Coldwell 2006-07-26 13:49:24 EDT
Ooops, wrong bug.
Comment 19 Dan Horák 2009-11-25 14:55:19 EST
Package Change Request
======================
Package Name: lsscsi
New Branches: EL-4 EL-5
Owners: sharkcz
Comment 20 Kevin Fenzi 2009-11-27 00:57:34 EST
cvs done.

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