Bug 625476 - Review Request: libpath_utils - File-system Path Utilities
Review Request: libpath_utils - File-system Path Utilities
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Rob Crittenden
Fedora Extras Quality Assurance
Depends On:
Blocks: 625482
  Show dependency treegraph
Reported: 2010-08-19 10:48 EDT by Stephen Gallagher
Modified: 2010-10-07 07:30 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2010-10-07 07:30:34 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
rcritten: fedora‑review+
kevin: fedora‑cvs+

Attachments (Terms of Use)

  None (edit)
Comment 1 Lameire Alexis 2010-08-19 14:31:04 EDT
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 14:54:51 EDT
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:

Comment 4 Rob Crittenden 2010-08-19 16:05:27 EDT
* source files match upstream: c54f32773a8c78c08340be330882fdb990cfb107 
* 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

Comment 5 Lameire Alexis 2010-08-19 18:55:03 EDT
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 07:13:30 EDT
Regarding the GPL:
"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 08:31:38 EDT
On further reflection, I think you're right. I can add the dependencies later if and when it becomes necessary.

New Spec:


Koji scratch build:
Comment 8 Stephen Gallagher 2010-08-20 08:53:19 EDT
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 09:12:38 EDT
thanks a lot for these enlightenments, i understand better why you include also the GPL. :)
Comment 10 Kevin Fenzi 2010-08-23 17:22:06 EDT
Git done (by process-git-requests).
Comment 11 Stephen Gallagher 2010-10-07 07:30:34 EDT
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.