This package contains commandline and GUI ACL utilities for the Linux
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??
This got missed by many because you set the fedora-review flag to '?' indicating
it's already under review.
I'll take a look.
that was my mistake... in the RHEL world '?' means needs attention.
I guess I need to go back a re-read the guidelines...
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
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
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)
Created attachment 149085 [details]
Quick patching fixing a few minor issues
> 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
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
- 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
With that done, I was able to simplify the spec significantly, normalize it, and
lose the need for the autotools BuildRequires.
Created attachment 149378 [details]
New, cleaned up spec file.
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.
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.
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
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
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.
Were you going to get this imported and built?
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?
You did not provide the necessary information for fedora-cvs requests. Please
copy & paste and fill out the request template.
New Package CVS Request
Package Name: nfs4-acl-tools
Short Description: nfs4 ACL tools
This should be closed if the package was built.