Bug 505374 - Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems
Summary: Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Susi Lehtola
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-06-11 17:11 UTC by Eric Sandeen
Modified: 2010-01-04 17:45 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-01-01 22:58:12 UTC
Type: ---
Embargoed:
susi.lehtola: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)

Description Eric Sandeen 2009-06-11 17:11:48 UTC
Spec & src.rpm at: http://sandeen.fedorapeople.org/nilfs-utils/

Description:

Userspace utilities for creating and mounting NILFS v2 filesystems.

(nilfs is a new upstream filesystem, enabled in rawhide, having userspace around might be nice)

Comment 1 Susi Lehtola 2009-06-11 19:54:38 UTC
- nilfs_cleanerd links against libnilfs.so.0 so the libraries should go in %{_lib} as for other packages of this type.


rpmlint output:
nilfs-utils.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/lssu ['/usr/lib64']
nilfs-utils.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/mkcp ['/usr/lib64']
nilfs-utils.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/rmcp ['/usr/lib64']
nilfs-utils.x86_64: E: binary-or-shlib-defines-rpath /sbin/nilfs_cleanerd ['/usr/lib64']
nilfs-utils.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/lscp ['/usr/lib64']
nilfs-utils.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/dumpseg ['/usr/lib64']
nilfs-utils.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/chcp ['/usr/lib64']
nilfs-utils.x86_64: W: devel-file-in-non-devel-package /usr/lib64/libnilfs.so
nilfs-utils.x86_64: E: setuid-binary /sbin/mkfs.nilfs2 root 04755
nilfs-utils.x86_64: E: non-standard-executable-perm /sbin/mkfs.nilfs2 04755
nilfs-utils.x86_64: E: setuid-binary /sbin/nilfs_cleanerd root 04755
nilfs-utils.x86_64: E: non-standard-executable-perm /sbin/nilfs_cleanerd 04755
nilfs-utils.x86_64: E: setuid-binary /sbin/mount.nilfs2 root 04755
nilfs-utils.x86_64: E: non-standard-executable-perm /sbin/mount.nilfs2 04755
nilfs-utils.x86_64: E: setuid-binary /sbin/umount.nilfs2 root 04755
nilfs-utils.x86_64: E: non-standard-executable-perm /sbin/umount.nilfs2 04755
nilfs-utils.x86_64: W: one-line-command-in-%post /sbin/ldconfig
nilfs-utils.x86_64: W: one-line-command-in-%postun /sbin/ldconfig
nilfs-utils-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 15 errors, 4 warnings.

- Get rid of rpath.
- The .so file goes in -devel.
- Are the permissions as wanted? I have to check other file system tools.
- Use
 %post -p /sbin/ldconfig
 %postun -p /sbin/ldconfig

MUST: The spec file for the package is legible and macros are used consistently. ~OK
- I'd use a macro for /sbin, say
%global _sbin /sbin

MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license. OK

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. NEEDSWORK
2e056e7979ed77727a7798d79de5188f  nilfs-utils-2.0.12.tar.bz2
6819db4b59f9504abe68ebc7818fd6ae  ../SOURCES/nilfs-utils-2.0.12.tar.bz2

MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. NEEDSWORK
- Time stamps are not preserved,
 make install DESTDIR=$RPM_BUILD_ROOT INSTALL="install -p"
should do the trick.

MUST: Packages containing shared library files must call ldconfig. OK
- See comments on rpmlint warnings.

MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A

MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
- You might want to add
 [ -s AUTHORS ] && exit 1
 [ -s NEWS ] && exit 1
 [ -s README ] && exit 1
to the %setup phase so you get notified if these acquire content.

MUST: Header files must be in a -devel package. OK
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A

MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. NEEDSWORK

MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. OK
- You might use just
 Requires: %{name} = %{version}-%{release}
to be clearer.

MUST: Packages does not contain any .la libtool archives. OK
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK

Comment 2 Eric Sandeen 2009-06-11 19:58:23 UTC
wow, that was a bit messier than I expected ;)  Ok, thanks for the review, will fix up.

Heh, I got the original tarball from their original src.rpm which doesn't match their released tarball, sigh.  I'll fix that too.

Thanks!
-Eric

Comment 3 Eric Sandeen 2009-06-11 20:11:54 UTC
Shoot, I hadn't even noticed the chmod u+s on some of these files, that strikes me as pretty odd, I'll have to investigate more.

Comment 4 Susi Lehtola 2009-07-05 10:39:45 UTC
ping?

Comment 5 Eric Sandeen 2009-07-05 14:51:11 UTC
Sorry for the delay, other stuff came up.  I need to see what the rules are for setuid binaries etc, and probably at least do some good code review of it before it goes in.

I appreciate the very quick review, sorry for the slowness on my end.

Comment 6 Susi Lehtola 2009-07-22 12:38:32 UTC
ping

Comment 8 Eric Sandeen 2009-07-30 01:48:03 UTC
Ayup, I'll get a new version out for review soon.  Sorry for the delays, Jussi.

Comment 10 Susi Lehtola 2009-07-30 06:08:27 UTC
Package does not build :)

In rawhide http://koji.fedoraproject.org/koji/taskinfo?taskID=1565083

checking for uuid_generate in -luuid... no
configure: error: UUID library not found
error: Bad exit status from /var/tmp/rpm-tmp.edEi49 (%build)
RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.edEi49 (%build)
Child returncode was: 1


and in F-11

chown: changing ownership of `/builddir/build/BUILDROOT/nilfs-utils-2.0.14-1.fc11.x86_64/sbin/nilfs_cleanerd': Operation not permitted
make[4]: Leaving directory `/builddir/build/BUILD/nilfs-utils-2.0.14/sbin/cleanerd'
make[3]: Leaving directory `/builddir/build/BUILD/nilfs-utils-2.0.14/sbin/cleanerd'
make[4]: *** [install-exec-hook] Error 1
make[3]: *** [install-exec-am] Error 2
make[2]: Leaving directory `/builddir/build/BUILD/nilfs-utils-2.0.14/sbin/cleanerd'

Comment 11 Eric Sandeen 2009-07-30 14:20:23 UTC
Oh sigh.  Bitten by my own package change (libuuid), sorry.  :(

Not sure what's up w/ f11, I'll look into that, it's weird.

Comment 13 Susi Lehtola 2009-07-30 18:39:23 UTC
Patch0 needs a comment.

You should use %global instead of %define, and preferably move the _root_libdir definition to the top of the spec file. For consistency I suggest defining also  a macro for /sbin.

**

rpmlint output:
nilfs-utils.src: E: no-cleaning-of-buildroot %clean
nilfs-utils-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 1 errors, 1 warnings.

Uncomment the rm command in %clean.


**

MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. OK
MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license. OK
MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. OK
MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. NEEDSWORK
- There are headers and man files that aren't generated and thus have an original time stamp. Add INSTALL="install -p" to make install to preserve time stamps.

MUST: Packages containing shared library files must call ldconfig. OK
MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly.
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. OK
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. OK
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. OK
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK

**

Fix the issues before importing to cvs. The package has been

APPROVED

Comment 14 Eric Sandeen 2009-07-30 18:52:46 UTC
(In reply to comment #13)
> Patch0 needs a comment.

ok.  upstream is fixing this (and the ldconfig problem) so it'll be dropped soon.

> You should use %global instead of %define, and preferably move the _root_libdir
> definition to the top of the spec file. For consistency I suggest defining also
>  a macro for /sbin.

ok

4 packages and 0 specfiles checked; 1 errors, 1 warnings.
> 
> Uncomment the rm command in %clean.

argh debugging leftovers.

> 
> Fix the issues before importing to cvs. The package has been
> 
> APPROVED  

Thanks!  And thanks for your patience - Sorry it was so iterative, I guess I'm just not very good at this ;)

-Eric

Comment 15 Eric Sandeen 2009-07-30 19:00:47 UTC
New Package CVS Request
=======================
Package Name: nilfs-utils
Short Description: Utilities for managing NILFS v2 filesystems
Owners: sandeen
Branches: F-11
InitialCC:

(note - no F10 due to lack of nilfs support in f10 kernel, and unlikely to be added)

Comment 16 Jason Tibbitts 2009-07-30 22:55:21 UTC
CVS done.

Comment 17 Susi Lehtola 2010-01-01 22:58:12 UTC
Seems like Eric forgot to file this bug as solved by the nilfs-utils update that was (or should have been?) shipped in Fedora 11.

Closing.

Comment 18 Eric Sandeen 2010-01-04 17:45:08 UTC
Yep, sorry for not closing.


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