rpmlint is not silent
W: xfsdump summary-ended-with-dot Administrative utilities for the XFS filesystem.
Summary ends with a dot.

==> Remove dot at end of Summary description

E: xfsdump no-changelogname-tag
There is no %changelog tag in your spec file. To insert it, just insert a
'%changelog' in your spec file and rebuild it.

==> you have not added Changelog. Add it.

W: xfsdump mixed-use-of-spaces-and-tabs
The specfile mixes use of spaces and tabs for indentation, which is a
cosmetic annoyance.  Use either spaces or tabs for indentation, not both.

When i look at SPEC file i found :-
  - dist tag is missing
  - I dont think Obsoletes and Conflicts is needed as package name is not
changed from previous version.
   - You have added twice make %{?_smp_mflags}. Remove one line
   - No Document files added. try to add under %files
     %doc README doc/COPYING doc/CHANGES doc/README.xfsdump doc/xfsdump_ts.txt
   

Thanks for the comments, I think I've addressed the issues except for
the dist tag. (not sure how that is suppose to be handled).

The Buildrequires has also been fixed to include the needed devel libs.

Ohh one other note:
The patch to remove libdm has been checked in upstream, 
it is not the same patch as in the original post but meets
the same goal of not needing libdm-devel.

Nuts.  I meant to assign this to myself last night to review -- I figured I have
a vested interest in it as a XFS fanboy since the first XFS-enabled RH 7.x
"installer CD's" were out -- but Russell, are you sponsored?  If not, then this
bug needs to also block "FE-NEEDSPONSOR", and I can make comments but can't do
the first binding review.

It's also convention when making updates during a review to bump the release and
spin a new srpm.  Helps people make sure that we're all talking about the same
package:)

Hmm sorry I'n new to the whole review process so I'm not sure what
it takes to be "sponsored", is there some info on this someplace.

bump the release, ok will do.

(In reply to comment #5)
> Hmm sorry I'n new to the whole review process so I'm not sure what
> it takes to be "sponsored", is there some info on this someplace.

The process of becoming a Fedora Extras Contributor is described here:
http://fedoraproject.org/wiki/Extras/Contributors

You've already done much of what's described there. There's more information on
getting sponsored linked from there too.

(In reply to comment #5)
> Hmm sorry I'n new to the whole review process so I'm not sure what
> it takes to be "sponsored", is there some info on this someplace.

Basically, sponsorship is just a way for people with established track records
in the Extras community (sponsors) to mentor new packagers.  It's designed to
both educate (sponsoree) and ensure responsibility (sponsor).

There are a couple good pages in the wiki about this:
http://www.fedoraproject.org/wiki/Extras/Contributors
http://www.fedoraproject.org/wiki/Extras/HowToGetSponsored

The second would seem to be on point here...

Also, is it just me or should this spec have a buildrequires of ncurses-devel?

"reassigned" to the new nobody id to make it clear this has not been formally
taken for review yet.

Just an experiment.

Interesting, so you can get a package back to the "NEW" state by closing and
then reopening it.

Russell, lately I've been sponsoring various Red Hat folks on the basis of a
single package as long as they're responsive, which it looks like you've been. 
Unfortunately, I don't really have the means to test this properly.  If there's
someone who is willing to chip in with the testing, I'm willing to sponsor and
do the review.

I'll volunteer to test.  Every box of mine is XFS :)

Note the xfs-cmds cvs tree on oss.sgi.com contains the xfstest scripts, many of
which run xfsdump/restore regression tests.

This scripts are run nightly by Nathan Scott at SGI but only 
on x86 and ia64 machines.
If anybody has other architectures available to test it would
to good to have those results as well.

OK, I'll go ahead and review this.  The links at the top are the only ones I
could find for the package; is that actually the current version?

First off, it doesn't build due to a lack of ncurses-devel.

Once I add that it does build.  Here's what rpmlint says:

W: xfsdump symlink-should-be-relative /usr/sbin/xfsrestore /sbin/xfsrestore
W: xfsdump symlink-should-be-relative /usr/sbin/xfsdump /sbin/xfsdump
  Indeed, these should be relative symlinks.

Plus there are tons of these in the debuginfo package:
W: xfsdump-debuginfo dangling-relative-symlink
/usr/src/debug/xfsdump-2.2.38/dump/inv_stobj.c ../inventory/inv_stobj.c

It seems that rpmbuild doesn't include the "common" directory in the package for
whatever reason.  I don't know how to convince it to do so.  I guess that if it
were a big deal you could flatten the links.  Unfortunately I don't know whether
it's a big deal or not so I'll have to ask around.

(In reply to comment #12)
> Note the xfs-cmds cvs tree on oss.sgi.com contains the xfstest scripts, many of
> which run xfsdump/restore regression tests.
> 
> This scripts are run nightly by Nathan Scott at SGI but only 
> on x86 and ia64 machines.
> If anybody has other architectures available to test it would
> to good to have those results as well.

If there is a test suite, doesn't it make sense to include it in the package as
well?  Even if it's another tarball, %check makes such testing easy, and we'd be
able to take advantage of automating this routine testing during the build on
the builders (which include ppc).

The test suite doesn't just test xfsdump, it tests most all xfs functionality.
There is no specific xfsdump testsuite.

It also requires that other packages be installed, and requires scratch partitions
available.  And it can run for a very long time.  The full test suite is
probably much to invasive for an rpm %check phase.

-Eric

>
> OK, I'll go ahead and review this.  The links at the top are the only ones I
> could find for the package; is that actually the current version?
>
> First off, it doesn't build due to a lack of ncurses-devel.
>   
Ok added that to the BuildRequires
> Once I add that it does build.  Here's what rpmlint says:
>
> W: xfsdump symlink-should-be-relative /usr/sbin/xfsrestore /sbin/xfsrestore
> W: xfsdump symlink-should-be-relative /usr/sbin/xfsdump /sbin/xfsdump
>   Indeed, these should be relative symlinks.
>   
Ok fixed these up in the spec file.

(new spec file uploaded)
> Plus there are tons of these in the debuginfo package:
> W: xfsdump-debuginfo dangling-relative-symlink
> /usr/src/debug/xfsdump-2.2.38/dump/inv_stobj.c ../inventory/inv_stobj.c
>
> It seems that rpmbuild doesn't include the "common" directory in the package for
> whatever reason.  I don't know how to convince it to do so.  I guess that if it
> were a big deal you could flatten the links.  Unfortunately I don't know whether
> it's a big deal or not so I'll have to ask around.
>   
I appears that  /usr/lib/rpm/find-debuginfo.sh is not picking up the files.
using vpath vs symlinks would be the right thing to do. Unfortunately
there seems to be some ugly hacks with a .c file picking up a different include file
based on which directory is being compiled. Has to do with getop.h for each command
the c file is common but it picks up different options based on which getopt.h
it finds
in the current directory.

So ya it appears the debug package is not that trivial.

I agree about the debuginfo package, and it's OK if it's not really possible to
make it complete, but I don't know what to do about the dangling symlinks.  You
can't just delete them from the source directory as that would break
short-circuit builds.

I wanted to build the package with your changes but the src.rpm link is no
longer valid.

Sorry updated the wrong bug.
There are now links to the latest srpm

http://xfs.org/~cattelan/xfsdump.spec
http://xfs.org/~cattelan/xfsdump.src.rpm

I tried to come up with some way to clean up the rpmlint warnings from the
debuginfo package and I'm out of ideas.  Perhaps some expert has a solution, but
in the absense of one I'm not going to let that block things.  The only thing
rpmlint has to complain about is the debuginfo package.

Some remaining issues that I've notices while doing the full review:

Don't use Distribution:.

You don't use the %dist tag in your Release:.  It's not mandatory but strongly
recommended; if you don't use it, you must be very careful to keep your versions
straight across the potentially five different releases that this package will
be built for.

Really the only blocker is the use of Distribution:; I'll leave the dist tag up
to you but remind you to take care if you do not add it, especially with your
first FC5 build after you branch as it will have the same version and won't
permit you to tag.

At this point you should go ahead and request cvsextras membership, and
fedorabugs if you want it.  I'll approve you and then you'll be able to check
in.  http://fedoraproject.org/wiki/Extras/Contributors#GetAFedoraAccount has
details.
 
Review:
* source files match upstream:
   4e113a39b07723bbb140d2e5c5389cfe  xfsdump_2.2.42-1.tar.gz
* 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).
O debuginfo package has problems which aren't easily soluble.
O rpmlint has valid but unfixable complaints (-debuginfo package only)
* final provides and requires are sane:
   xfsdump = 2.2.42-1
  =
   attr >= 2.0.0
   libattr.so.1()(64bit)
   libattr.so.1(ATTR_1.0)(64bit)
   libhandle.so.1()(64bit)
   libncurses.so.5()(64bit)
   libuuid.so.1()(64bit)
   xfsprogs >= 2.6.30
* %check is not present; running upstream test suite is not reasonable.
* no shared libraries are added to the regular linker search paths.
* 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.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no libtool .la droppings.

APPROVED, provided you remove Distribution:

*** Bug 116016 has been marked as a duplicate of this bug. ***

The src rpm has been checked into the fedora cvs repository and
the build has passed the build test.
So at this point I think the xfsdump package should be ready to
go.
I'm not clear if there is anything else that needs to be done before it
it built and distributed as part of "extras"

You built on the development branch, so your package will automatically be
available to rawhide users at the next package push and will be in FE6 when it
branches.

If you want to make a release for FC5, request an FC5 branch at
http://fedoraproject.org/wiki/Extras/CVSSyncNeeded

Once that gets done, you can tag and build for FC5.

At this point, though, you can close this bug unless you want to wait until the
FC5 branch is complete.

Is there some reason this package hasn't been built yet?

Package Change Request
======================
Package Name: xfsdump
Updated Fedora Owners: cattelan,esandeen

cattelan doesn't seem to be listed in the account system. 
Did you mean that to stay cattelan?

Added esandeen. Re-request if you want to change the other address.

no cattelan
I don't work for RedHat anymore :-)

right, so you need a fedora acct w/ that email, Russell.

Package Change Request
======================
Package Name: xfsdump
Updated Fedora Owners: esandeen

(please remove cattelan - no such account anymore, cvs mail is
bouncing, no other account in the fedora system yet)

Also, if CC: doesn't have to be in the account system, please:

Updated Fedora CC: cattelan 

otherwise perhaps we can drop him until (if/when) he gets his fedora acct set up.

Thanks,
-Eric

cvs done. 

cattelan has to be in the account system to be in packagedb to be listed in CC
for the package. ;(