Bug 229342 - Review Request: nfs4-acl-tools - ACL utilities for NFSv4
Review Request: nfs4-acl-tools - ACL utilities for NFSv4
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
Depends On:
  Show dependency treegraph
Reported: 2007-02-20 09:37 EST by Steve Dickson
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2007-05-08 10:57:49 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+
wtogami: fedora‑cvs+

Attachments (Terms of Use)
Quick patching fixing a few minor issues (911 bytes, patch)
2007-03-01 21:45 EST, Jason Tibbitts
no flags Details | Diff
New, cleaned up spec file. (1.83 KB, text/plain)
2007-03-06 15:41 EST, Tom "spot" Callaway
no flags Details
Patch to enable DESTDIR (and fix symlink) (5.18 KB, patch)
2007-03-06 15:45 EST, Tom "spot" Callaway
no flags Details | Diff
Actually use the LDFLAGS that are passed by the user (413 bytes, patch)
2007-03-06 15:47 EST, Tom "spot" Callaway
no flags Details | Diff

  None (edit)
Description Steve Dickson 2007-02-20 09:37:39 EST
Spec URL: 


This package contains commandline and GUI ACL utilities for the Linux
NFSv4 client.
Comment 1 Steve Dickson 2007-03-01 19:49:28 EST
Ping.... I'm getting pressure to make sure this is included in FC7...
I'm hoping that since I opened this bz well be for the feature freeze,
there should not be a problem getting this in... right??
Comment 2 Jason Tibbitts 2007-03-01 19:59:21 EST
This got missed by many because you set the fedora-review flag to '?' indicating
it's already under review.

I'll take a look.
Comment 3 Steve Dickson 2007-03-01 20:30:32 EST
that was my mistake... in the RHEL world '?' means needs attention.
I guess I need to go back a re-read the guidelines... 
Comment 4 Jason Tibbitts 2007-03-01 21:44:26 EST
Let's see if I can avoid losing this review because of an intervening comment....

I note 0.3.0 came out yesterday.

Let's check rpmlint:

W: nfs4-acl-tools non-standard-group System Environment/Tools
   Not a problem; group is pretty much meaningless and what you have at least
   seems accurate.

W: nfs4-acl-tools no-version-in-last-changelog
W: nfs4-acl-tools-debuginfo no-version-in-last-changelog
   Changelog entries must include the relevant version info:

W: nfs4-acl-tools no-documentation
   There is documentation included in the tarball, including two contradictory
   license statements.  Great.  But that documentation needs to get into the
   final package.

W: nfs4-acl-tools empty-%post
W: nfs4-acl-tools empty-%preun
W: nfs4-acl-tools empty-%postun
   These should go if there's nothing to put in them.

This package seems to have three licenses, BSD, LGPL and GPL, and it's not
entirely clear to me which parts are under which license.  As a bonus, there
are two COPYING files and one LICENSE file.  They've managed to
make a relatively simple package quite complicated.  However, doc/README seems
to indicate that only the build scripts remain from the GPL-licensed package,
so I suppose all of the actual code is under the 3-clause BSD license included
in COPYING (not doc/COPYING).

Did you intend to build the graphical bits?  The summary seems to indicate so,
but they aren't built.  And the INSTALL file talks of naughty things like
static linking.  Scary.

The way that CFLAGS gets set is a bit weird.  $ARCH_OPT_FLAGS doesn't seem to
be defined on any platform I have access to.  ANd the whole configure thing is
pretty bad; you shouldn't put the buildroot in --prefix or --bindir.  But the
package fails to install if you do that.  And to boot, the resulting binaries
are statically linked.

I know this isn't your fault as you're just working around what seems to be a
really broken upstream build process, but I think this package is busted
enough that I'd like someone else with more experience dealing with really
broken upstreams to comment.

I'll attach a patch that fixes some of the issues, but perhaps a rebase to
0.3.0 is in order just to see if the build process has improved.

* source files match upstream:
* package meets naming and versioning guidelines.
* specfile is properly named
X macros should probably be used instead of 
* dist tag is present.
* build root is correct.
? license field matches the actual license.
* license is open source-compatible.
X license text is not included in package.
X latest version is being packaged.
* BuildRequires are proper.
* compiler flags are appropriate.
* %clean is present.
* package builds in mock (development, x86_64).
* package installs properly
* debuginfo package looks complete.
X rpmlint has valid complaints.
* final provides and requires are sane:
   nfs4-acl-tools = 0.2.0-1.fc7

* %check is not present; no test suite upstream.  I personally have no NFS4
  install and so no way to test this.
* 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 -docs 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 droppings.
* not a GUI app (but perhaps it should be)
Comment 5 Jason Tibbitts 2007-03-01 21:45:19 EST
Created attachment 149085 [details]
Quick patching fixing a few minor issues
Comment 6 Steve Dickson 2007-03-02 11:21:30 EST
> I note 0.3.0 came out yesterday.
Updated to this version which now include man pages.

> W: nfs4-acl-tools no-version-in-last-changelog
> W: nfs4-acl-tools-debuginfo no-version-in-last-changelog
> W: nfs4-acl-tools no-documentation
> W: nfs4-acl-tools empty-%post
> W: nfs4-acl-tools empty-%preun
> W: nfs4-acl-tools empty-%postun
All fixed by applying patch in Comment #5 (thank you very much!)

> Did you intend to build the graphical bits?
No, since I was not able to build them. I'll add them
in an upcoming release.
> you shouldn't put the buildroot in --prefix or --bindir.  But the
> package fails to install if you do that. 
With the addition of the man pages, I also had to add --mandir to
this configuration line... But by setting these during the 
configuration, removed the need to set the DESTDIR during
the "make install" 

Updated Spec is in: 

Updated SRPM is in 
Comment 7 Tom "spot" Callaway 2007-03-06 15:40:30 EST
Steve, you owe me a Stuffed Pizza for this. ;)

I took a look at fixing up the source for this package a bit, and I found the
following items:

- The source has no concept of "DESTDIR", which is pretty easy to fix.
- It was doing some brain-dead checking for the existence of nfs_editfacl. I
fixed it up a bit, and now it makes a correct symlink.
- The source was completely obliterating any LDFLAGS passed to it, configure saw
them, but the include/buildmacros was just overwriting them blindly. I fixed
that too.

With that done, I was able to simplify the spec significantly, normalize it, and
lose the need for the autotools BuildRequires.
Comment 8 Tom "spot" Callaway 2007-03-06 15:41:19 EST
Created attachment 149378 [details]
New, cleaned up spec file.
Comment 9 Tom "spot" Callaway 2007-03-06 15:45:08 EST
Created attachment 149379 [details]
Patch to enable DESTDIR (and fix symlink)

This patch enables DESTDIR functionality in the source. If DESTDIR is not set,
the behavior is identical to before, with one exception:

One of the Makefiles was attempting to make a symlink for nfs_editfacl if it
didn't exist. Ignoring the fact that this is a bit braindead to do (just
because it doesn't exist at buildtime doesn't mean it won't get installed
immediately after nfs4-acl-tools), it wasn't making a very good symlink. I
fixed it so that it makes the symlink properly.
Comment 10 Tom "spot" Callaway 2007-03-06 15:47:04 EST
Created attachment 149380 [details]
Actually use the LDFLAGS that are passed by the user

Passing LDFLAGS, and using them too? That's too much to ask! ;)

Well, maybe not. This simple patch actually lets the source use the LDFLAGS
that the user exports/passes to configure.
Comment 11 Steve Dickson 2007-03-07 14:44:42 EST
Yes, my friend, I do owe you a stuffed pizza and maybe even a
"pop" or two... ;-) and believe me... the next time I can get
back to the homeland... I'll be more than happy to pay up!

I've incorporated all of Sir Callaway's changes and 
patches in to cvs and tagged them as nfs4-acl-tools-0_3_0-1_fc7_1

I've also passed the patches onto the upstream maintainer
and have promise that he will incorporate Spot's changes
in the next release... which should be in a couple of 

The source srpm and spec file have been updated in
Comment 12 Steve Dickson 2007-03-08 16:05:44 EST
As promised, the upstream maintainer incorporated
Sir Callaway's patch and put out new release, 0.3.1

The source srpm and spec file have been updated in
Comment 13 Jason Tibbitts 2007-03-09 10:22:14 EST
OK, let's check it out.

Yes, it builds fine now; the only thing rpmlint has to say now is the
  previously addressed "non-standard-group" bit.
The spec is far cleaner, and things look good there.
The license tag is fine and the license is in the package.

The only thing that gives me any pause is the fact that the package is
building a static library and then statically linking the binaries against
it.  I don't think this actually violates any guidelines, however, since the
static library isn't being shipped.  I guess converting to a shared library
is something that upstream could consider for a future release, especially if
other packages could benefit from being able to use it.

In any case, all of the blockers have been dealt with.

Comment 14 Steve Dickson 2007-03-09 13:15:03 EST
Thank you!
Comment 15 Jason Tibbitts 2007-03-16 21:26:16 EDT
Were you going to get this imported and built?
Comment 16 Steve Dickson 2007-03-19 08:37:10 EDT

I get the following error when I try to build this package:

FAILED: BuildError: package nfs4-acl-tools not in list for tag dist-fc7

What needs to happen?
Comment 17 Warren Togami 2007-03-19 19:37:09 EDT
You did not provide the necessary information for fedora-cvs requests.  Please
copy & paste and fill out the request template.
Comment 18 Steve Dickson 2007-03-26 12:57:13 EDT
New Package CVS Request
Package Name: nfs4-acl-tools
Short Description: nfs4 ACL tools
Owners: steved@redhat.com
Branches: FC-7
Comment 19 Jason Tibbitts 2007-05-05 22:30:40 EDT
This should be closed if the package was built.

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