Bug 848388 - Review Request: liblognorm - Tool to normalize log data
Review Request: liblognorm - Tool to normalize log data
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Tomas Mraz
Fedora Extras Quality Assurance
:
Depends On: 847811 847817
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-15 09:17 EDT by mahaveer darade
Modified: 2013-03-01 00:30 EST (History)
12 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-11-14 21:38:31 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tmraz: fedora‑review+
tibbs: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description mahaveer darade 2012-08-15 09:17:23 EDT
Spec URL: http://theinric.fedorapeople.org/liblognorm/liblognorm.spec
SRPM URL: http://theinric.fedorapeople.org/liblognorm/liblognorm-0.3.4-1.src.rpm
Description: Liblognorm is a tool to normalize log data.
Fedora Account System Username: Mahaveer 

This is my third package and I need a sponsor.


rpmlint logs:
-------------

[mdarade@mdarade SPECS]$ rpmlint -i liblognorm.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[mdarade@mdarade SPECS]$ rpmlint -i ../SRPMS/liblognorm-0.3.4-1.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[mdarade@mdarade SPECS]$ rpmlint -i ../RPMS/x86_64/liblognorm-
liblognorm-0.3.4-1.x86_64.rpm            liblognorm-devel-0.3.4-1.x86_64.rpm      
liblognorm-debuginfo-0.3.4-1.x86_64.rpm  liblognorm-utils-0.3.4-1.x86_64.rpm      
[mdarade@mdarade SPECS]$ rpmlint -i ../RPMS/x86_64/liblognorm-*
liblognorm-devel.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

liblognorm-utils.x86_64: W: spelling-error Summary(en_US) Normalizer -> Normalize, Normalizes, Normalized
The value of this tag appears to be misspelled. Please double-check.

liblognorm-utils.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

liblognorm-utils.x86_64: W: no-manual-page-for-binary normalizer
Each executable in standard binary directories should have a man page.

4 packages and 0 specfiles checked; 0 errors, 4 warnings.
[mdarade@mdarade SPECS]$
Comment 1 Milan Bartos 2012-08-27 04:42:09 EDT
Hi, this is just an informational review.

1. liblognorm requires libestr, it should be in BuildRequires
2. %clean tag is not necessary
3. use INSTALL='install -p'

Regards,
Milan
Comment 2 mahaveer darade 2012-08-28 05:04:38 EDT
Done. All the changes suggested in comment#1 are in-place & below are new rpmlint logs. 



[root@mdarade guest]# rpmlint -i liblognorm.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[root@mdarade guest]# rpmlint -i liblognorm-0.3.4-2.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[root@mdarade guest]# rpmlint -i liblognorm*x86_64*.rpm
liblognorm-devel.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

liblognorm-utils.x86_64: W: spelling-error Summary(en_US) Normalizer -> Normalize, Normalizes, Normalized
The value of this tag appears to be misspelled. Please double-check.

liblognorm-utils.x86_64: W: no-documentation
The package contains no documentation (README, doc, etc). You have to include
documentation files.

liblognorm-utils.x86_64: W: no-manual-page-for-binary normalizer
Each executable in standard binary directories should have a man page.

4 packages and 0 specfiles checked; 0 errors, 4 warnings.
[root@mdarade guest]#
Comment 3 mahaveer darade 2012-08-28 07:29:23 EDT
Forgot to mention updated link to SRPM so here it is:

http://theinric.fedorapeople.org/liblognorm/liblognorm-0.3.4-2.src.rpm
Comment 4 Michael Schwendt 2012-09-15 09:23:49 EDT
> Release:	2

Not using %dist during review is okay, but be aware of future trouble when building the package for multiple dists.


> Summary:	Tool to normalize log data

The library is not really "a tool". The pkgconfig file contains a good description that could be used for a better %summary:

    Description: fast samples-based log normalization library


* The pkgconfig file as is won't work flawlessly. It contains

    Libs: -L${libdir} -llognorm -lee -lestr

which means a "Requires" dependency is missing in the file, or else one could not link successfully. However, explicitly relinking with shared libee and libestr should not be necessary when linking with liblognorm, so these two linker options should be dropped.

Some of liblognorm's header files access libee/libestr headers, so clearly a "Requires: libee-devel libestr-devel" is needed in the liblognorm-devel package. Note that adding a dependency to the .pc file would result in automatic RPM dependencies.



> %package devel
> Requires:	%{name} = %{version}-%{release}

> %package utils
> Requires:	%{name} = %{version}-%{release}

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


> %description utils
> The normalizer is the core of liblognorm, it's utility for normalizing
> log files.

s/it's/its/ ?


> %build
> make

"V=1 make" for more verbose output (compiler and linker options) in build log would be nice. Plus (albeit this is just a small source project): 
https://fedoraproject.org/wiki/Packaging:Guidelines#Parallel_make


> %install
> make install -p DESTDIR=%{buildroot}

Option -p is very likely a mistake here as it is passed to "make" and is short for "--print-data-base". Assumably, what you actually want is:

  make install INSTALL="install -p"  DESTDIR=%{buildroot}


> $ rpmls -p liblognorm-utils-0.3.4-2.t1.x86_64.rpm 
> -rwxr-xr-x  /usr/bin/normalizer

A very generic name, not in a special namespace, and almost caused a conflict already:

  # repoquery --whatprovides /usr/bin/normalize
  normalize-0:0.7.7-6.fc17.x86_64

Upstream is highly encouraged to try to avoid using such generic names.
Comment 5 Milan Bartos 2012-09-19 03:44:44 EDT
Normalizer has been renamed in liblognorm 0.3.5 to lognormalizer to be more poignant. (http://www.liblognorm.com/tag/0-3-5/)
Comment 6 mahaveer darade 2012-09-20 22:20:23 EDT
I've worked on all the comments provided by Michael and here is a link to updated package. 

http://siddharths.fedorapeople.org/mahaveer/SPECS/liblognorm.spec
http://siddharths.fedorapeople.org/mahaveer/SRPMS/liblognorm-0.3.4-3.fc15.src.rpm
Comment 7 Tomas Mraz 2012-10-01 10:05:47 EDT
The %description for the main package should be at least slightly more descriptive. And the %description of the utils subpackage contains error 'its' is mistaken for 'it is'.

The same mistake as in libee and libestr is repeated here - the first sentence in COPYING contradicts with the contents of COPYING and the source files.
Comment 8 Michael Schwendt 2012-10-01 13:41:03 EDT
> And the %description of the utils subpackage contains error
> 'its' is mistaken for 'it is'.

See comment 4. "it is" or "it's" would not form a valid sentence, because an article such as "a" or "the" would be missing:

| The lognormalizer is the core of liblognorm, it's utility for
| normalizing log files.

So, either add the missing article or rephrase.
Comment 9 mahaveer darade 2012-10-05 10:34:32 EDT
I've worked on comments provided by Tomas & Michael. Below is a link to updated package.

http://mdarade.fedorapeople.org/SPECS/liblognorm.spec
http://mdarade.fedorapeople.org/SRPMS/liblognorm-0.3.4-4.fc15.src.rpm
Comment 10 Tomas Mraz 2012-10-05 11:26:56 EDT
Please do not change the COPYING file, it needs to be corrected upstream. You can correct this mistake before you import the package into Fedora git.

rpmlint -v liblognorm-0.3.4-4.fc16.src.rpm liblognorm-0.3.4-4.fc16.x86_64.rpm liblognorm-devel-0.3.4-4.fc16.x86_64.rpm liblognorm-utils-0.3.4-4.fc16.x86_64.rpm liblognorm-debuginfo-0.3.4-4.fc16.x86_64.rpm
liblognorm.src: I: checking
liblognorm.src: I: checking-url http://www.liblognorm.com (timeout 10 seconds)
liblognorm.src: I: checking-url http://www.liblognorm.com/files/download/liblognorm-0.3.4.tar.gz (timeout 10 seconds)
liblognorm.x86_64: I: checking
liblognorm.x86_64: I: checking-url http://www.liblognorm.com (timeout 10 seconds)
liblognorm-devel.x86_64: I: checking
liblognorm-devel.x86_64: I: checking-url http://www.liblognorm.com (timeout 10 seconds)
liblognorm-devel.x86_64: W: no-documentation
liblognorm-utils.x86_64: I: checking
liblognorm-utils.x86_64: W: spelling-error Summary(en_US) Lognormalizer -> Normalize
liblognorm-utils.x86_64: I: checking-url http://www.liblognorm.com (timeout 10 seconds)
liblognorm-utils.x86_64: W: no-documentation
liblognorm-utils.x86_64: W: no-manual-page-for-binary lognormalizer
liblognorm-debuginfo.x86_64: I: checking
liblognorm-debuginfo.x86_64: I: checking-url http://www.liblognorm.com (timeout 10 seconds)
5 packages and 0 specfiles checked; 0 errors, 4 warnings.
The spelling error is not a real error.

Missing documentation and manual page is upstream issue.

Tarball matches the upstream sources.

The package complies with Fedora packaging and licensing guidelines.

The package is ACCEPTED.
Comment 11 mahaveer darade 2012-10-05 23:24:23 EDT
Thank you all for the review of all three packages.
Comment 12 Steven Dake 2012-10-06 00:09:29 EDT
reassigning to package review component since this is not pacemaker-cloud component and pacemaker-cloud has been dropped from Fedora.
Comment 13 Tomas Mraz 2012-10-08 02:58:57 EDT
Reverting the accidental changes.
Comment 14 Mahaveer Darade 2012-10-08 09:06:04 EDT
New Package SCM Request
=======================
Package Name: liblognorm
Short Description: Fast samples-based log normalization library
Owners: mdarade
Branches: f17 f18
InitialCC: mbartos
Comment 15 Jason Tibbitts 2012-10-08 12:27:16 EDT
Git done (by process-git-requests).
Comment 16 Fedora Update System 2012-11-07 02:12:33 EST
liblognorm-0.3.4-4.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/liblognorm-0.3.4-4.fc17
Comment 17 Fedora Update System 2012-11-07 21:01:22 EST
liblognorm-0.3.4-4.fc17 has been pushed to the Fedora 17 testing repository.
Comment 18 Fedora Update System 2012-11-08 01:36:44 EST
liblognorm-0.3.4-4.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/liblognorm-0.3.4-4.fc18
Comment 19 Fedora Update System 2012-11-14 21:38:34 EST
liblognorm-0.3.4-4.fc17 has been pushed to the Fedora 17 stable repository.

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