[Bug 1105015] Review Request: lua-ldap - LDAP client library for Lua

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



--- Comment #3 from Jeff Backus <jeff.backus@xxxxxxxxx> ---
(In reply to Dan Callaghan from comment #2)
> 
> The license is in the tarball as doc/us/license.html (easily missed since
> it's not included as plain text in the root, like most packages).
> 
> It's in the corresponding place under /usr/share/doc for the lua-ldap and
> lua-ldap-compat packages as well.
> 

Found it. Thanks!

> > [-]: %check is present and all tests pass.
> >      Source tarball contains what looks like some sort of test. Maybe add as
> > %check?
> 
> I wrote a little script to allow running the tests against a dummy slapd in
> %check.
> 
> https://github.com/luaforge/lualdap/pull/2

Looks good. Thanks!

> 
> > [!]: Packages should try to preserve timestamps of original installed files.
> >      Please work with upstream (or create a patch) to change the following
> > line in the Makefile:
> >         cp src/$(LIBNAME) $(LUA_LIBDIR)
> >      to:
> > 	cp -p src/$(LIBNAME) $(LUA_LIBDIR)
> 
> Preserving timestamps in this case doesn't serve much purpose, since the
> file is built right before that and is not included in multiple subpackages
> or anything like that. However I can easily add it.
> 

Good point. Also, unfortunately, I seem to have given you erroneous advice. cp
-p copies all permissions, so now I'm getting a rpmlint error. The correct
method is to use install -p. I verified that this is correct. Please change
destdir.patch to either use install -p or to not use cp -p on line 12. Sorry
about that!

> http://fedorapeople.org/~dcallagh/lua-ldap/lua-ldap.spec
> http://fedorapeople.org/~dcallagh/lua-ldap/lua-ldap-1.1.0-2.fc21.src.rpm

Since this package hasn't been built by the system, increasing the version
number is optional. Please see the section on multiple Changelog Entries per
Release in the guidelines for more information. 
https://fedoraproject.org/wiki/Packaging:Guidelines#Multiple_Changelog_Entries_per_Release

So, in summary, please fix the previous "fix" I requested ('cp -p' -> 'install
-p' or just 'cp'). Otherwise, the package looks good!

Regards,
Jeff

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]