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.