[Bug 505374] New: Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

Summary: Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

https://bugzilla.redhat.com/show_bug.cgi?id=505374

Summary: Review Request: nilfs-utils - Utilities for managing
                    NILFS v2 filesystems
           Product: Fedora
           Version: rawhide
          Platform: All
        OS/Version: Linux
            Status: NEW
          Severity: medium
          Priority: medium
         Component: Package Review
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]
         QAContact: [e-mail]
                CC: [e-mail], [e-mail]
   Estimated Hours: 0.0
    Classification: Fedora
    Target Release: ---

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)

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

Jussi Lehtola  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |[e-mail]
         AssignedTo|[e-mail]    |[e-mail]
               Flag|                            |fedora-review?

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

--- Comment #1 from Jussi Lehtola   2009-06-11 15:54:38 EDT ---
- 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

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

--- Comment #2 from Eric Sandeen   2009-06-11 15:58:23 EDT ---
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

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

--- Comment #3 from Eric Sandeen   2009-06-11 16:11:54 EDT ---
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.

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

--- Comment #4 from Jussi Lehtola   2009-07-05 06:39:45 EDT ---
ping?

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

--- Comment #5 from Eric Sandeen   2009-07-05 10:51:11 EDT ---
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.

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

--- Comment #6 from Jussi Lehtola   2009-07-22 08:38:32 EDT ---
ping

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

Tomas Hoger  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #7 from Tomas Hoger   2009-07-23 04:02:00 EDT ---
Upstream version 2.0.14 released on monday should no longer install /sbin files
setuid, thanks to Eric's / Steve's report:

http://www.nilfs.org/git/?p=nilfs2-utils.git;a=commitdiff;h=d807e1c968c1f288...
http://www.nilfs.org/git/?p=nilfs2-utils.git;a=commitdiff;h=5c95a57102e23e69...
http://www.nilfs.org/git/?p=nilfs2-utils.git;a=commitdiff;h=a5cb60e624e4863c...

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

--- Comment #8 from Eric Sandeen   2009-07-29 21:48:03 EDT ---
Ayup, I'll get a new version out for review soon.  Sorry for the delays, Jussi.

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

--- Comment #9 from Eric Sandeen   2009-07-29 23:38:48 EDT ---
Updated:

http://sandeen.fedorapeople.org/nilfs-utils/nilfs-utils.spec
http://sandeen.fedorapeople.org/nilfs-utils/nilfs-utils-2.0.14-1.fc12.src.rp...

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

--- Comment #10 from Jussi Lehtola   2009-07-30 02:08:27 EDT ---
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'

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

--- Comment #11 from Eric Sandeen   2009-07-30 10:20:23 EDT ---
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.

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

--- Comment #12 from Eric Sandeen   2009-07-30 11:20:50 EDT ---
Ok, once more with feeling.

http://sandeen.fedorapeople.org/nilfs-utils/nilfs-utils-2.0.14-2.fc10.src.rp...
http://sandeen.fedorapeople.org/nilfs-utils/nilfs-utils-2.0.14-2.fc10.src.rp...
http://sandeen.fedorapeople.org/nilfs-utils/nilfs-utils.spec

f10 vs. f12 just has the different BuildRequires.

thanks,
-Eric

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

Jussi Lehtola  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-review?              |fedora-review+

--- Comment #13 from Jussi Lehtola   2009-07-30 14:39:23 EDT ---
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

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

--- Comment #14 from Eric Sandeen   2009-07-30 14:52:46 EDT ---
(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

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

Eric Sandeen  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-cvs?

--- Comment #15 from Eric Sandeen   2009-07-30 15:00:47 EDT ---
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)

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

Jason Tibbitts  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-cvs?                 |fedora-cvs+

--- Comment #16 from Jason Tibbitts   2009-07-30 18:55:21 EDT ---
CVS done.

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

Jussi Lehtola  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |CLOSED
         Resolution|                            |CURRENTRELEASE

--- Comment #17 from Jussi Lehtola   2010-01-01 17:58:12 EDT ---
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.

[Bug 505374] Review Request: nilfs-utils - Utilities for managing NILFS v2 filesystems

[Red Hat Bugzilla]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=505374

--- Comment #18 from Eric Sandeen   2010-01-04 12:45:08 EDT ---
Yep, sorry for not closing.