Bug 847811 - Review Request: libee - An event expression library
Review Request: libee - An event expression library
Status: CLOSED ERRATA
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: 847817
Blocks: 848388
  Show dependency treegraph
 
Reported: 2012-08-13 11:52 EDT by mahaveer darade
Modified: 2013-03-01 00:30 EST (History)
13 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-12-16 04:36:37 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-13 11:52:24 EDT
Spec URL: http://siddharths.fedorapeople.org/mahaveer/SPECS/libee.spec
SRPM URL: http://siddharths.fedorapeople.org/mahaveer/SRPMS/libee-0.4.1-1.fc15.src.rpm
Description: Libee is an event expression library which is inspired by the upcoming CEE standard. It provides capabilities to generate and emit messages in a set of standard format and read a set of different input formats. Its needed for rsyslog package. 

Fedora Account System Username: Mahaveer


This is my first package and I need a sponsor.

Here are rpmlint logs.

[mdarade@mdarade SPECS]$ rpmlint -i libee.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[mdarade@mdarade SPECS]$ rpmlint -i ../SRPMS/libee-0.4.1-1.fc15.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[mdarade@mdarade SPECS]$ rpmlint -i ../RPMS/x86_64/libee-*
libee-devel.x86_64: W: spelling-error %description -l en_US rsyslog -> serology
The value of this tag appears to be misspelled. Please double-check.

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

libee-devel.x86_64: W: no-manual-page-for-binary libee-convert
Each executable in standard binary directories should have a man page.

3 packages and 0 specfiles checked; 0 errors, 3 warnings.
[mdarade@mdarade SPECS]$
Comment 1 Jason Tibbitts 2012-08-15 15:09:09 EDT
If you set fedora-review, nobody will see your tickets.
Comment 2 Milan Bartos 2012-08-27 04:50:48 EDT
Hi, this is just an informational review.

1. use macros and variables consistently
2. %clean is not necessary
3. i'm not sure about %{_sbindir}/libee-convert, it could go in separate -utils package

Regards,
Milan
Comment 3 mahaveer darade 2012-08-28 07:17:26 EDT
Done. I have worked on all 3 suggestions and changes are in-place. 

Below is new SRPM link:
http://siddharths.fedorapeople.org/mahaveer/SRPMS/libee-0.4.1-2.fc15.src.rpm


Below are new rpmlint logs. 


[root@mdarade guest]# rpmlint  -i libee.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.
[root@mdarade guest]# rpmlint  -i libee-0.4.1-2.fc15.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
[root@mdarade guest]# rpmlint  -i libee-*.x86_64.rpm 
libee-devel.x86_64: W: spelling-error %description -l en_US rsyslog -> serology
The value of this tag appears to be misspelled. Please double-check.

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

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

libee-utils.x86_64: W: no-manual-page-for-binary libee-convert
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 4 Michael Schwendt 2012-09-14 06:38:26 EDT
> Summary: An event expression library inspired by CEE

Not covered by any guidelines, but in many cases the leading article is superfluous and doesn't increase conciseness.

  Summary: Event expression library inspired by CEE

That makes a much better reading when displayed by Anaconda and package tools.


> License: LGPLv2+ 

True. The majority of source files contain an LGPLv2+ preamble. Only src/cjson/* contains merged files with an MIT-style preamble. There's a minor typo at the top of file COPYING, which refers to GPL rather than LGPL.


> Group: Development/Libraries 

Base library packages still enter group "System Environment/Libraries".


> %description

I would rearrange the two paragraphs to begin with the explanation of what libee does, then continue with the explanation of what CEE is.


> %package devel
> Summary: Include files for libee

More generally, it's

  Summary: Development files for libee

because not just the header files are included.


> Requires: %name = %version-%release

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



> %package utils
> Summary:   The libee-convert utility provided by event expression library 
> Requires:   %{name} = %{version}-%{release}

Same here.

The summary restricts the package contents to just the libee-convert utility. It could be made more general to sum up whether these tools are optional or strictly needed.


> %{__make}

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

Plus: if you don't have a compelling reason to use a macro here, feel free to run "make" from $PATH just as done with many other commands one runs in .spec files.


> %install
> rm -rf %{buildroot}

It will be automatically cleaned:
https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag
Comment 5 Michael Schwendt 2012-09-16 14:52:06 EDT
* As noticed in the related reviews, there are inter-package dependencies involved. libee-devel should contain

  Requires: libestr-devel%{?_isa}

as headers like libee.h include libestr headers.


> %build
> %configure 
> %{__make}

Calling "V=1 make" for more verbose build output would be nice. For this package, the linking stage would be affected, so a test-build was necessary to notice this.


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

That error is in here, too:

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


* The "tests" directory contains a testsuite that's suitable for running it in a %check section in the package.
Comment 6 mahaveer darade 2012-09-20 22:19:12 EDT
I have worked on all the comments provided by Michael and here is a link to updated package. 

http://siddharths.fedorapeople.org/mahaveer/SPECS/libee.spec
http://siddharths.fedorapeople.org/mahaveer/SRPMS/libee-0.4.1-3.fc15.src.rpm
Comment 7 Tomas Mraz 2012-10-01 09:54:53 EDT
I think the license tag is wrong - it should be probably either LGPLv2+ and MIT or LGPLv2+ alone. The statement at the beginning of COPYING file is probably an upstream mistake as the full text of the license is LGPL2.1 and not GPL.

Please correct the licence tag and notify upstream about the mistake.

I will approve the package anyway as the licence tag mistake can be fixed before import.

rpmlint -v libee-0.4.1-3.fc16.src.rpm libee-0.4.1-3.fc16.x86_64.rpm libee-devel-0.4.1-3.fc16.x86_64.rpm libee-utils-0.4.1-3.fc16.x86_64.rpm
libee.src: I: checking
libee.src: I: checking-url http://www.libee.org (timeout 10 seconds)
libee.src: I: checking-url http://www.libee.org/files/download/libee-0.4.1.tar.gz (timeout 10 seconds)
libee.x86_64: I: checking
libee.x86_64: I: checking-url http://www.libee.org (timeout 10 seconds)
libee-devel.x86_64: I: checking
libee-devel.x86_64: W: spelling-error %description -l en_US rsyslog -> serology
libee-devel.x86_64: I: checking-url http://www.libee.org (timeout 10 seconds)
libee-devel.x86_64: W: no-documentation
libee-utils.x86_64: I: checking
libee-utils.x86_64: I: checking-url http://www.libee.org (timeout 10 seconds)
libee-utils.x86_64: W: no-documentation
libee-utils.x86_64: W: no-manual-page-for-binary libee-convert
4 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 8 Michael Schwendt 2012-10-01 13:36:54 EDT
> I think the license tag is wrong

In comment 4 I explicitly acknowledge that "License: LGPLv2+" was correct. No idea why it has been changed in the newer spec file.

The %changelog should have mentioned such changes to _the packaging_. That's one of the things packagers ought to practise.
Comment 9 mahaveer darade 2012-10-05 10:52:59 EDT
I've worked on comments by Michael & Tomas. Below are the links to updated files.

http://mdarade.fedorapeople.org/SPECS/libee.spec
http://mdarade.fedorapeople.org/SRPMS/libee-0.4.1-4.fc15.src.rpm
Comment 10 Tomas Mraz 2012-10-05 11:23:48 EDT
Please do not change the COPYING file, it needs to be corrected upstream. Also the package is already ACCEPTED, so please follow the packaging process to build it into Fedora.
Comment 11 Mahaveer Darade 2012-10-08 09:05:22 EDT
New Package SCM Request
=======================
Package Name: libee
Short Description: Event expression library inspired by CEE
Owners: mdarade
Branches: f17 f18
InitialCC:
Comment 12 Jason Tibbitts 2012-10-08 12:26:22 EDT
Git done (by process-git-requests).
Comment 13 Fedora Update System 2012-10-24 07:30:49 EDT
libee-0.4.1-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/libee-0.4.1-3.fc18
Comment 14 Fedora Update System 2012-10-24 12:16:41 EDT
libee-0.4.1-3.fc18 has been pushed to the Fedora 18 testing repository.
Comment 15 Fedora Update System 2012-10-25 02:30:37 EDT
libee-0.4.1-3.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/libee-0.4.1-3.fc17

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