Bug 225945 - Merge Review: jfsutils
Merge Review: jfsutils
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Susi Lehtola
Fedora Package Reviews List
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 14:12 EST by Nobody's working on this, feel free to take it
Modified: 2009-03-25 14:15 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-03-25 14:15:25 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
susi.lehtola: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 14:12:29 EST
Fedora Merge Review: jfsutils

http://cvs.fedora.redhat.com/viewcvs/devel/jfsutils/
Initial Owner: jgarzik@redhat.com
Comment 1 Josh Boyer 2007-02-05 21:38:58 EST
Jeff has disclaimed ownership of jfsutils.  See bug 226558 for the specific comment.
Comment 2 Josh Boyer 2007-02-06 09:36:40 EST
I plan on taking this package over via the orphan package process.
Comment 3 Susi Lehtola 2009-03-21 09:32:52 EDT
I'll take on the review.
Comment 4 Susi Lehtola 2009-03-21 09:48:16 EDT
rpmlint output:
jfsutils.src:28: W: configure-without-libdir-spec
jfsutils.src: W: summary-ended-with-dot Utilities for managing the JFS filesystem.
jfsutils.x86_64: W: summary-ended-with-dot Utilities for managing the JFS filesystem.
jfsutils-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/jfsutils-1.1.13/include/jfs_dtree.h
jfsutils-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/jfsutils-1.1.13/include/jfs_dinode.h
jfsutils-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/jfsutils-1.1.13/libfs/log_work.c
jfsutils-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/jfsutils-1.1.13/include/jfs_superblock.h
jfsutils-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/jfsutils-1.1.13/mkfs/inodes.h
jfsutils-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/jfsutils-1.1.13/include/jfs_dmap.h
jfsutils-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/jfsutils-1.1.13/libfs/message.c
jfsutils-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/jfsutils-1.1.13/include/jfs_unicode.h
jfsutils-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/jfsutils-1.1.13/include/jfs_xtree.h
jfsutils-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/jfsutils-1.1.13/include/jfs_types.h
jfsutils-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/jfsutils-1.1.13/include/jfs_btree.h
jfsutils-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/jfsutils-1.1.13/libfs/devices.c
jfsutils-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/jfsutils-1.1.13/mkfs/mkfs.c
jfsutils-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/jfsutils-1.1.13/include/jfs_imap.h
3 packages and 0 specfiles checked; 0 errors, 17 warnings.


- Change buildroot to 
%(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)

- Use %configure --sbindir=/sbin, and enable smp make.

- Remove
# let brp-compress handle this policy
rm -f $RPM_BUILD_ROOT/%{_mandir}/*/*.gz

- Remove buildroot check from clean phase.
Comment 5 Josh Boyer 2009-03-21 11:01:20 EDT
I've done most of this locally now.  Still need to look at the weird debuginfo stuff.
Comment 6 Susi Lehtola 2009-03-21 14:07:44 EDT
While you're at it, add %{?dist} to the release tag.
Comment 7 Susi Lehtola 2009-03-22 05:02:21 EDT
(In reply to comment #5)
> I've done most of this locally now.  Still need to look at the weird debuginfo
> stuff.  

The debuginfo stuff should be trivial to fix, just remove the executable bits from the source code files and headers in the setup phase.
Comment 8 Josh Boyer 2009-03-23 08:40:59 EDT
All fixed in devel
Comment 9 Susi Lehtola 2009-03-23 10:10:57 EDT
.. Not so fast! I didn't do the review yet, or approve the package!

- Please use %configure instead of ./configure. If you
%global _sbindir /sbin
you can use plain %configure and %{_sbindir}/* in the files section.

The review as follows:

- source files match upstream:
 * Tarball matches upstream, but
MUST: Source URL is missing!
X package meets naming and versioning guidelines.
X specfile is properly named, is cleanly written and uses macros consistently.
X dist tag is present.
X build root is correct.
X license field matches the actual license.
X license is open source-compatible.
- license text included in package.
MUST: Missing from documentation: AUTHORS ChangeLog COPYING NEWS
X latest version is being packaged.
X BuildRequires are proper.
X compiler flags are appropriate.
X %clean is present.
X package builds in mock.
X package installs properly.
X debuginfo package looks complete.
X rpmlint is silent.
X final provides and requires are sane.
X no shared libraries are added to the regular linker search paths.
X owns the directories it creates.
X doesn't own any directories it shouldn't.
X no duplicates in %files.
X file permissions are appropriate.
X no scriptlets present.
X code, not content.
X documentation is small, so no -docs subpackage is necessary.
X %docs are not necessary for the proper functioning of the package.
X no headers.
X no pkgconfig files.
X no libtool .la droppings.
X no desktop files

Fix the above and I'll approve the package.
Comment 10 Josh Boyer 2009-03-23 10:21:29 EDT
(In reply to comment #9)
> .. Not so fast! I didn't do the review yet, or approve the package!
> 
> - Please use %configure instead of ./configure. If you
> %global _sbindir /sbin

Why is that needed?  It should be done in the buildsys already...
Comment 11 Susi Lehtola 2009-03-23 10:25:12 EDT
(In reply to comment #10)
> (In reply to comment #9)
> > .. Not so fast! I didn't do the review yet, or approve the package!
> > 
> > - Please use %configure instead of ./configure. If you
> > %global _sbindir /sbin
> 
> Why is that needed?  It should be done in the buildsys already...  

Because by default _sbindir is /usr/sbin. This redefinition makes the spec file cleaner, and is used in other system tool specs too.
Comment 12 Josh Boyer 2009-03-23 10:26:27 EDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > .. Not so fast! I didn't do the review yet, or approve the package!
> > > 
> > > - Please use %configure instead of ./configure. If you
> > > %global _sbindir /sbin
> > 
> > Why is that needed?  It should be done in the buildsys already...  
> 
> Because by default _sbindir is /usr/sbin. This redefinition makes the spec file
> cleaner, and is used in other system tool specs too.  

Yeah, noticed that.  I'm not thrilled with overriding the macro though, so I'll just specify /sbin.
Comment 13 Susi Lehtola 2009-03-23 10:32:27 EDT
(In reply to comment #9)
> - license text included in package.
> MUST: Missing from documentation: AUTHORS ChangeLog COPYING NEWS

Sorry, disregard this. However, you can drop INSTALL and README from the
package as they are related to the compilation.

(In reply to comment #12)
> Yeah, noticed that.  I'm not thrilled with overriding the macro though, so I'll
> just specify /sbin.  

OK, you can do that if you use %configure --sbindir=/sbin as mentioned above.

Also, please change %{_mandir}/*/* to %{_mandir}/man8/*, since if translations appear then the package will end up owning system directories.
Comment 14 Josh Boyer 2009-03-23 10:41:32 EDT
Changes are in cvs now (except the INSTALL/README thing because I forgot it and it's not really that important).

I'll wait before doing another build until I get the magic approval
Comment 15 Susi Lehtola 2009-03-23 15:22:48 EDT
(In reply to comment #14)
> Changes are in cvs now (except the INSTALL/README thing because I forgot it and
> it's not really that important).
> 
> I'll wait before doing another build until I get the magic approval  

Drop the unnecessary stuff from the configure phase, it should read just %configure --sbindir=/sbin
since all of the other stuff is already in the %configure script.

Or, if you redefine sbindir as I mentioned before the configure phase is just
%configure
as the redefined sbindir is automatically expanded.

IMHO it's nicer to just redefine sbindir and use %{_sbindir} everywhere instead of /sbin.
Comment 16 Josh Boyer 2009-03-25 08:49:09 EDT
Changes in CVS
Comment 17 Susi Lehtola 2009-03-25 14:15:25 EDT
Well, you still don't need to
CFLAGS="${RPM_OPT_FLAGS}"

since it is already in %configure. Remove the CFLAGS stuff (and INSTALL and README from %doc since they only contain information about building the package, which is not necessary in the binary rpm).

Anyway, all of the blockers have been fixed, so the package is

APPROVED.


Josh: please remove the unnecessary stuff before you rebuild in rawhide.

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