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 201779
Summary: | Review Request: xfsdump | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Russell Cattelan <cattelan> |
Component: | Package Review | Assignee: | Jason Tibbitts <j> |
Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | cweyl, esandeen, j, mgarski, npearl, panemade |
Target Milestone: | --- | Flags: | kevin:
fedora-cvs+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | FC6 | Doc Type: | Bug Fix |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2006-09-20 15:46:41 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
Russell Cattelan
2006-08-08 19:56:14 UTC
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. ;( |