Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 452691 - Review Request: btrfs-progs - supporting programs for btrfs
Summary: Review Request: btrfs-progs - supporting programs for btrfs
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2008-06-24 14:28 UTC by Josef Bacik
Modified: 2008-08-11 12:11 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-08-11 12:11:30 UTC
tibbs: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Josef Bacik 2008-06-24 14:28:59 UTC
Spec URL: http://people.redhat.com/jwhiter/btrfs-progs.spec
SRPM URL: http://people.redhat.com/jwhiter/btrfs-progs-0.15-3.fc8.src.rpm
Description: The btrfs-progs package provides all the userpsace programs needed to create, check, modify and correct any inconsistencies in the btrfs filesystem.

Comment 1 Bill Nottingham 2008-06-24 14:46:04 UTC
This seems somewhat early, considering there's no kernel support (and the
on-disk format isn't even stabilized yet....) How does having this in now help?

Comment 2 Josef Bacik 2008-06-24 14:49:27 UTC
Just helps me stay ahead of the game, when it does hit mainline I'm going to
have a bunch of other crap to do, so I'd like to go ahead and get this into
fedora now to cut down on the amount of work I have to do later on.

Comment 3 Jason Tibbitts 2008-06-24 17:15:15 UTC
I'll go ahead and review this, but I can't test it at all so I'm asking some of
the kernel folks if they'll sign off on it.

Full review forthcoming....

Comment 4 Jason Tibbitts 2008-06-24 17:36:54 UTC
The only question I have involves the compiler flags; assuming x86_64, this
package uses:
   -Wall -fno-strict-aliasing -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2 -g 
   -Werror -Os
whereas the defaults are
   -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector 
   --param=ssp-buffer-size=4 -m64 -mtune=generic
I've no particular issues with -Os versus -O2, but my understanding is that
-fstack-protector is of some importance, at least.

Any reason not to use our default compiler flags, perhaps with s/-O2/-Os/?

* source files match upstream:
  ca261e50a5a66f7169b60d7bb5b9835e6284f4fa81d58e17d942fd3b4934618a  
   btrfs-progs-0.15.tar.bz2
* package meets naming and versioning guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* summary is OK.
* description is OK.
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package.
* latest version is being packaged.
* BuildRequires are proper.
? Not sure about compiler flags:
   -Wall -fno-strict-aliasing -D_FILE_OFFSET_BITS=64 -D_FORTIFY_SOURCE=2 -g 
   -Werror -Os
* %clean is present.
* package builds in mock (rawhide, x86_64).
* package installs properly.
* debuginfo package looks complete.
* rpmlint is silent.
* final provides and requires are sane:
   btrfs-progs = 0.15-3.fc10
  =
   libuuid.so.1()(64bit)

* %check is present; no test suite upstream.  I cannot test this package.
* no shared libraries are added to the regular linker search paths.
* 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 -doc subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no static libraries.
* no libtool .la files.



Comment 5 Josef Bacik 2008-06-24 20:11:33 UTC
i can change the cflags in the spec file for now if you like, the flags that are
used are just what chris happened to pick when he wrote the makefile :).  I'll
probably send something up to add -fstack-protector and -fexceptions.

Comment 6 Jason Tibbitts 2008-06-24 20:21:56 UTC
It would probably be best to pass CFLAGS="$RPM_OPT_FLAGS" into the makefile, or
to tweak that for -Os if you prefer.  That way if we need to mess with the flags
for whatever reason the package just needs a rebuild.

Comment 8 Jason Tibbitts 2008-06-26 22:53:54 UTC
This looks fine to me.  I guess there's little hope of getting any of the kernel
folks to chime in here, and I can't really see any reason to hold this up
because I can at least run the tools, I just can't mount any filesystems.

APPROVED

Comment 9 Josef Bacik 2008-06-30 15:29:55 UTC
New Package CVS Request
=======================
Package Name: btrfs-progs
Short Description: Userspace programs for btrfs
Owners: josef
Branches: F-9
InitialCC:
Cvsextras Commits: yes


Comment 10 Kevin Fenzi 2008-06-30 16:24:00 UTC
cvs done.

Comment 11 Jason Tibbitts 2008-08-09 15:07:55 UTC
This was built and is in rawhide currently; any reason not to close this ticket?


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