Bug 625476 - Review Request: libpath_utils - File-system Path Utilities
Summary: Review Request: libpath_utils - File-system Path Utilities
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Rob Crittenden
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 625482
TreeView+ depends on / blocked
 
Reported: 2010-08-19 14:48 UTC by Stephen Gallagher
Modified: 2010-10-07 11:30 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-10-07 11:30:34 UTC
Type: ---
Embargoed:
rcritten: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Comment 1 Lameire Alexis 2010-08-19 18:31:04 UTC
This is an informal review, since I need a sponsor.
I've identified some (little) issues:

--- In the build logs:
  configure: WARNING: unrecognized options: --disable-rpath
Not a serious issue, but you could safely remove this option

--- Some BuildRequires may be useless here: autoconf, automake, libtool and m4 should not be necessary, since the ./configure script doesn't call any of these Autotools executables.

--- rpmlint outputs look good : all false positives:
libpath_utils-debuginfo.x86_64: W: spelling-error Summary(en_US) libpath -> lib path, lib-path, librate
libpath_utils-debuginfo.x86_64: W: spelling-error Summary(en_US) utils -> utile, utilizes, utilize
libpath_utils-debuginfo.x86_64: W: spelling-error %description -l en_US libpath -> lib path, lib-path, librate
libpath_utils-debuginfo.x86_64: W: spelling-error %description -l en_US utils -> utile, utilizes, utilize
libpath_utils-devel.x86_64: W: spelling-error Summary(en_US) libpath -> lib path, lib-path, librate
libpath_utils-devel.x86_64: W: spelling-error Summary(en_US) utils -> utile, utilizes, utilize
5 packages and 0 specfiles checked; 0 errors, 6 warnings.

--- COPYING file (with GPL) should not be delivered because this license only concerns the unit tests, not included in the binary packages.

Comment 2 Stephen Gallagher 2010-08-19 18:54:51 UTC
The BuildRequires on autotools are there because autotools will sometimes internally detect during 'make' that the files need to be regenerated. (This can happen if the version of certain files is different on the build system)

I didn't notice that --disable-rpath was ignored. That was left in there from the original package I split off from. Fixed.

COPYING file must be included. COPYING.LESSER is a set of extensions to COPYING, not a complete license itself.

Thank you for the review.


New spec:
http://sgallagh.fedorapeople.org/packagereview/ding-libs/libpath_utils.spec

New SRPM:
http://sgallagh.fedorapeople.org/packagereview/ding-libs/libpath_utils-0.2.1-2.fc13.src.rpm

Comment 4 Rob Crittenden 2010-08-19 20:05:27 UTC
* source files match upstream: c54f32773a8c78c08340be330882fdb990cfb107 
libpath_utils-0.2.1.tar.gz
* package meets naming and versioning guidelines.
* specfile is properly named and is cleanly written
* specfile uses macros consistently.
* summary is OK.
* description is OK
* dist tag is present.
* build root is OK.
* license field matches the actual license.
* license is open source-compatible.
* license text included in package. (both of them)
* latest version is being packaged.
* BuildRequires are proper.
* %clean is present.
* package builds in mock (F-13, x86_64).
* package installs properly
* rpmlint is silent.
* %check is present
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* documentation present in -devel package
* %docs are set correctly

APPROVED

Comment 5 Lameire Alexis 2010-08-19 22:55:03 UTC
Although this package quickly was approved, I still think the above-mentionned BuildRequires are still useless. If ./configure relaunches the autotools, it means that the configure.ac or Makefile.am files were more recent than it. This SHOULD not be the case, the developer MUST relaunchs the autotools to refresh the configure + Makefile.in files. So, since you seem to be also a developer of sssd, you must refresh these files instead of letting configure do the dirty job. You will certainly help packagers (and your potential co-mainteners!) to integrate and spread your stuff in other distributions...


I'd like more enlightenments about the license files: I can't understand how the provided LGPL text can extend the completely different GPL one.

Comment 6 Stephen Gallagher 2010-08-20 11:13:30 UTC
Regarding the GPL:
http://www.gnu.org/licenses/gpl-howto.html
"If you are releasing your program under the LGPL, you should also include the text version of the LGPL, usually in a file called COPYING.LESSER. Please note that, since the LGPL is a set of additional permissions on top of the GPL, it's important to include both licenses so users have all the materials they need to understand their rights."

The real reason behind including the autotools is future-compatibility with patches. For example, if I opt to backport a fix from the master branch of SSSD that requires a makefile or configure check update, I'll need to run autoreconf for it to work properly.

If you want, I can add this in explicitly in advance. I just usually don't bother to have it run autoreconf on any build without patches.

Comment 7 Stephen Gallagher 2010-08-20 12:31:38 UTC
On further reflection, I think you're right. I can add the dependencies later if and when it becomes necessary.

New Spec:
http://sgallagh.fedorapeople.org/packagereview/ding-libs/libpath_utils.spec

New SRPM:
http://sgallagh.fedorapeople.org/packagereview/ding-libs/libpath_utils-0.2.1-4.fc13.src.rpm

Koji scratch build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2414172

Comment 8 Stephen Gallagher 2010-08-20 12:53:19 UTC
New Package SCM Request
=======================
Package Name: libpath_utils
Short Description: File-system path utilities
Owners: sgallagh
Branches: f13 f14
InitialCC: dpal

Comment 9 Lameire Alexis 2010-08-20 13:12:38 UTC
thanks a lot for these enlightenments, i understand better why you include also the GPL. :)

Comment 10 Kevin Fenzi 2010-08-23 21:22:06 UTC
Git done (by process-git-requests).

Comment 11 Stephen Gallagher 2010-10-07 11:30:34 UTC
Withdrawing this package. Upstream has changed packaging and it will be bundled
into ding-libs.

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


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