Spec URL: http://people.redhat.com/steved/nfs4-acl-tools/nfs4-acl-tools.spec SRPM URL: http://people.redhat.com/steved/nfs4-acl-tools/nfs4-acl-tools-0.2.0-1.fc7.src.rpm Description: This package contains commandline and GUI ACL utilities for the Linux NFSv4 client.
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 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: http://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs 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. Checklist: * source files match upstream: f4a55f18bda2df3ea6b7afb8b755a9f3f95010c7c022c66b4102a030720f9045 nfs4-acl-tools-0.2.0.tar.gz * 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.x86_64.rpm nfs4-acl-tools = 0.2.0-1.fc7 = /bin/sh libattr.so.1()(64bit) libattr.so.1(ATTR_1.0)(64bit) * %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: http://people.redhat.com/steved/nfs4-acl-tools/nfs4-acl-tools.spec Updated SRPM is in http://people.redhat.com/steved/nfs4-acl-tools/nfs4-acl-tools-0.3.0-1.fc7.src.rpm
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.
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 days... The source srpm and spec file have been updated in http://people.redhat.com/steved/nfs4-acl-tools/
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 http://people.redhat.com/steved/nfs4-acl-tools/
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. APPROVED
Thank you!
Were you going to get this imported and built?
Jesse, 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?
http://fedoraproject.org/wiki/PackageMaintainers/CVSAdminProcedure 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 Owners: steved Branches: FC-7 InitialCC:
This should be closed if the package was built.