Bug 551258 - Review Request: libgcal - A library to access google calendar events and contacts
Review Request: libgcal - A library to access google calendar events and cont...
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: leigh scott
Fedora Extras Quality Assurance
:
Depends On:
Blocks: 551274
  Show dependency treegraph
 
Reported: 2009-12-29 12:21 EST by Mario Ceresa
Modified: 2010-08-26 03:57 EDT (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-08-06 21:47:30 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
leigh123linux: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
spec file (2.29 KB, application/octet-stream)
2010-04-14 17:52 EDT, leigh scott
no flags Details

  None (edit)
Description Mario Ceresa 2009-12-29 12:21:18 EST
Spec URL: http://mrceresa.fedorapeople.org/libgcal.spec
SRPM URL: http://mrceresa.fedorapeople.org/libgcal-0.9.3-1.fc12.src.rpm
Description: 
This is a library to access google calendar events and contacts, its purpose is
   - provide easy access to available events/contacts
   - enable common operations: add, delete, edit
   - have few dependencies (up until now, only requires libcurl and libxml)

It implements Google Data API 2.0 and is tested on Linux and MacOSX.

Hi! this is a nice package to sync with with google address and calendar
A second package is required which is akonadi-googledata to be actually able to do the sync with akonadi.
The couple of them worked for me flawlessy

$ rpmlint ../RPMS/i686/libgcal-0.9.3-1.fc12.i686.rpm
libgcal.i686: W: incoherent-version-in-changelog libgcal-0.9.3-1.fc12 ['0.9.3-1.fc12', '0.9.3-1']
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

$ rpmlint ../RPMS/i686/libgcal-devel-0.9.3-1.fc12.i686.rpm
libgcal-devel.i686: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 1 warnings

$ rpmlint ../SRPMS/libgcal-0.9.3-1.fc12.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint libgcal.spec
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

builds ok in koji:
https://koji.fedoraproject.org/koji/taskinfo?taskID=1894557
Comment 1 Andrea Musuruane 2010-01-02 08:16:29 EST
Changelog format is not correct:
http://fedoraproject.org/wiki/Packaging/Guidelines#Changelogs

This is why you get a warning in rpmlint.
Comment 2 Michael Schwendt 2010-01-11 08:06:37 EST
> Name:           libgcal
> Group:          Development/Libraries

The base shared library still belongs into "System Environment/Libraries".


> %doc COPYING
> %doc INSTALL
> %doc README
> %doc Changelog.txt

You can put multiple files behind a single %doc.

File "INSTALL" is irrelevant to RPM package users.


> %postun
> /sbin/ldconfig
> 
> #---------------------------------------------------------------------------------
> %package        devel

Caution! Avoid '#comments' after scriptlet sections. They would be included as scriptlet bodies, which breaks -p /sbin/ldconfig for example. In this particular case, simply delete that silly #---- line. It doesn't serve any purpose and only causes trouble here. Then "%postun -p /sbin/ldconfig" will work again.

$ rpm -qp --scripts libgcal-0.9.3-1.fc12.i686.rpm 
postinstall program: /sbin/ldconfig
postuninstall scriptlet (using /bin/sh):
/sbin/ldconfig

#---------------------------------------------------------------------------------


> %description devel
> 
> libgcal library Header Files and Link Libraries

This description would benefit from some cleanup. Please make this a complete sentence in English.


* Have you looked into enabling the built-in test-suite and creating a proper %check section for it?


* The -devel packages depends on libxml and libcurl headers, so adding "Requires: libxml2-devel libcurl-devel" would be necessary for all targets that don't add automatic pkgconfig dependencies (Fedora 10 and older, EPEL 5 and older).

If you want to rely on the automatic pkgconfig dependencies, you can drop the "Requires: pkgconfig".


* The header files in libgcal-devel bear a huge risk of causing file conflicts. This is a blocker IMO. This library ought to install them into a libgcal sub-directory:

$ rpmls -p libgcal-devel-0.9.3-1.fc12.i686.rpm 
-rw-r--r--  /usr/include/atom_parser.h
-rw-r--r--  /usr/include/gcal.h
-rw-r--r--  /usr/include/gcal_parser.h
-rw-r--r--  /usr/include/gcal_status.h
-rw-r--r--  /usr/include/gcalendar.h
-rw-r--r--  /usr/include/gcont.h
-rw-r--r--  /usr/include/gcontact.h
-rw-r--r--  /usr/include/internal_gcal.h
-rw-r--r--  /usr/include/xml_aux.h
[...]
Comment 3 leigh scott 2010-01-16 16:16:45 EST
Any progress?
Comment 4 Mario Ceresa 2010-01-21 10:22:56 EST
Hello Scott, Andrea and Michael,
I'm very sorry to reply only now, but I've been out for work and had so little time to focus on the review:

@Andrea: thanks a lot for pointing me to the right source!
@Michael: thanks for the comments:

> The base shared library still belongs into "System Environment/Libraries".
> You can put multiple files behind a single %doc.
> File "INSTALL" is irrelevant to RPM package users.

Fixed. Install added as a doc for development subpackage.

> Caution! Avoid '#comments' after scriptlet sections. They would be included as
> scriptlet bodies, which breaks -p /sbin/ldconfig for example. 

Thanks! This one was trubling me for a while but I'd never thought the comments were the problem :)

> This description would benefit from some cleanup. Please make this a complete
> sentence in English.

Oops! I must have missed how ugly the previous sentence was. Hope now it's a bit better

> Have you looked into enabling the built-in test-suite and creating a proper
%check section for it?
Yes, but I had some problems so thought to overlook. If it is really important I could give it another shot... 

> The -devel packages depends on libxml and libcurl headers, so adding
>"Requires: libxml2-devel libcurl-devel" would be necessary for all targets that
>don't add automatic pkgconfig dependencies (Fedora 10 and older, EPEL 5 and
>older).
> If you want to rely on the automatic pkgconfig dependencies, you can drop the
> "Requires: pkgconfig".


I don't understand this: when is that I have to manually specify Requires and when it's better to let them be picked up by rpm?


> The header files in libgcal-devel bear a huge risk of causing file conflicts.
> This is a blocker IMO. This library ought to install them into a libgcal
> sub-directory:

Ok I added two lines that solve the problems in the INSTALL section:

mkdir -p %{buildroot}/usr/include/libgcal
mv %{buildroot}/usr/include/*.h %{buildroot}/usr/include/libgcal/

but I'm not sure this is the best way to do it.

New files are:

http://mrceresa.fedorapeople.org/libgcal.spec
http://mrceresa.fedorapeople.org/libgcal-0.9.3-2.fc12.src.rpm

Mario
Comment 5 Andrea Musuruane 2010-01-21 10:43:14 EST
Mario, you can drop the %{?dist} macro from changelog entries.
Comment 6 Michael Schwendt 2010-01-28 16:05:23 EST
> > File "INSTALL" is irrelevant to RPM package users.
>
> Fixed. Install added as a doc for development subpackage.

It is irrelevant to RPM package users. It explains how to compile libgcal.


[%check]

> Yes, but I had some problems so thought to overlook.
> If it is really important I could give it another shot... 

Well, "BuildRequires: check-devel" plus --enable-check plus "make check", and here the first test makes glibc find a realloc problem.

Surely that is something to look into. Upstream should tell you whether this release is expected to pass the test-suite.


> when is that I have to manually specify Requires and
> when it's better to let them be picked up by rpm?

In Fedora 11 and newer, rpmbuild examines .pc files for dependencies on other .pc files. Example:

$ grep Req libgcal.pc 
Requires: libcurl libxml-2.0

$ rpm -qpR libgcal-devel-0.9.3-2.fc12.i686.rpm | grep pkg
/usr/bin/pkg-config  
pkgconfig                <-- this is your manual one
pkgconfig(libcurl)  
pkgconfig(libxml-2.0)  

The dependencies on /usr/bin/pkg-config, libcurl.pc and libxml-2.0.pc are added by rpmbuild.

On older platforms, rpmbuild doesn't do this.  And where the pkgconfig .pc file does not specify dependencies, it may be necessary to add them to your .spec file manually (by examining C/C++ header files for needed dependencies).


> mkdir -p %{buildroot}/usr/include/libgcal
>
> mv %{buildroot}/usr/include/*.h %{buildroot}/usr/include/libgcal/
> 
> but I'm not sure this is the best way to do it.

This is incomplete, because libgcal.pc still thinks the files are found in /usr/include.  Please talk to upstream about this. [One cannot move header file locations without either adjusting all "#include <...>" statements or header search paths.]
Comment 7 Thomas Janssen 2010-03-06 14:27:14 EST
The review is stalled. Please respond soon.
Comment 8 Mario Ceresa 2010-03-08 03:38:32 EST
Hello Scott, Andrea, Michael and Thomas,
thanks for your messages. I'm sorry to have proceeded slowly for this package so far: I hope to collect all the data from the revisions and post a new version of the package this week.

Thanks for your patience,

Mario
Comment 9 Mario Ceresa 2010-03-09 09:25:58 EST
Contacted upstream about libgcal.pc include path (Comment 6)

http://code.google.com/p/libgcal/issues/detail?id=59

I'll wait for their answer before going on with the review.

With best regards,

Mario
Comment 10 Mario Ceresa 2010-03-13 13:27:55 EST
Hello everybody,
thanks to a patch from Peter Lemenkov: 
http://peter.fedorapeople.org/patches/libgcal--pc_include_fix.diff

I can resubmit the package:
http://mrceresa.fedorapeople.org/libgcal-0.9.3-3.fc12.src.rpm
http://mrceresa.fedorapeople.org/libgcal.spec

the package builds ok with koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2050755

BTW rpmlist is not silent:

[makerpms@shadow rpmbuild]$ rpmlint SPECS/libgcal.spec SRPMS/libgcal-0.9.3-3.fc12.src.rpm RPMS/i686/libgcal-0.9.3-3.fc12.i686.rpm RPMS/i686/libgcal-devel-0.9.3-3.fc12.i686.rpm 
SPECS/libgcal.spec: W: invalid-url Source0: http://libgcal.googlecode.com/files/libgcal-0.9.3.tar.bz2 HTTP Error 404: Not Found
libgcal.src: W: spelling-error Summary(en_US) google -> Google, goggle, googly
libgcal.src: W: spelling-error %description -l en_US google -> Google, goggle, googly
libgcal.src: W: spelling-error %description -l en_US libcurl -> lib curl, lib-curl, liberal
libgcal.src: W: spelling-error %description -l en_US libxml -> Librium, libel, libido
libgcal.src: W: invalid-url Source0: http://libgcal.googlecode.com/files/libgcal-0.9.3.tar.bz2 HTTP Error 404: Not Found
libgcal.i686: W: spelling-error Summary(en_US) google -> Google, goggle, googly
libgcal.i686: W: spelling-error %description -l en_US google -> Google, goggle, googly
libgcal.i686: W: spelling-error %description -l en_US libcurl -> lib curl, lib-curl, liberal
libgcal.i686: W: spelling-error %description -l en_US libxml -> Librium, libel, libido
libgcal-devel.i686: W: spelling-error %description -l en_US google -> Google, goggle, googly
3 packages and 1 specfiles checked; 0 errors, 11 warnings.
Comment 11 leigh scott 2010-03-13 14:28:14 EST
Ok you will need to remove INSTALL from the devel package, all the rpmlint warnings can be safely ignored ( the invalid-url warning is invalid ).
As a personal ( I'm not sure if it's a guideline ) preference could you use the standard format for the spec file  

i.e run this to generate a standard format spec file

rpmdev-newspec  libgcal
Comment 12 Mario Ceresa 2010-03-21 19:02:17 EDT
ok these are the new files without the INSTALL file and with the standard format spec file:

http://mrceresa.fedorapeople.org/libgcal-0.9.3-4.fc12.src.rpm
http://mrceresa.fedorapeople.org/libgcal.spec
Comment 13 leigh scott 2010-04-08 10:37:32 EDT
I haven't forgotten this review request, hopefully I should have some free time in the next couple of weeks to complete it.
Comment 14 Mario Ceresa 2010-04-08 11:35:41 EDT
That's ok Scott, 

thanks for the message.

Mario
Comment 15 leigh scott 2010-04-14 17:43:01 EDT
Package Review
==============

Key:
 - = N/A
 x = Check
 ! = Problem
 ? = Not evaluated

=== REQUIRED ITEMS ===
 [x] Package is named according to the Package Naming Guidelines.
 [x] Spec file name must match the base package %{name}, in the format
%{name}.spec.
 [x] Package meets the Packaging Guidelines.
 [x] Package successfully compiles and builds into binary rpms on at least one
supported architecture.
     Tested on: f13/x86_64
 [x] Rpmlint output:

 rpmlint libgcal-0.9.3-4.fc13.src.rpm 
libgcal.src: W: spelling-error Summary(en_US) google -> Google, goggle, googly
libgcal.src: W: spelling-error %description -l en_US google -> Google, goggle, googly
libgcal.src: W: spelling-error %description -l en_US libcurl -> lib curl, lib-curl, liberal
libgcal.src: W: spelling-error %description -l en_US libxml -> Librium, libido, Libyan
libgcal.src: W: invalid-url Source0: http://libgcal.googlecode.com/files/libgcal-0.9.3.tar.bz2 HTTP Error 404: Not Found
1 packages and 0 specfiles checked; 0 errors, 5 warnings.

spelling errors can be ignored (you should capitalize google in summary and description)
invalid-url warning is invalid

rpmlint libgcal-0.9.3-4.fc13.x86_64.rpm 
libgcal.x86_64: W: spelling-error Summary(en_US) google -> Google, goggle, googly
libgcal.x86_64: W: spelling-error %description -l en_US google -> Google, goggle, googly
libgcal.x86_64: W: spelling-error %description -l en_US libcurl -> lib curl, lib-curl, liberal
libgcal.x86_64: W: spelling-error %description -l en_US libxml -> Librium, libido, Libyan
1 packages and 0 specfiles checked; 0 errors, 4 warnings. 

rpmlint libgcal-devel-0.9.3-4.fc13.x86_64.rpm
libgcal-devel.x86_64: W: spelling-error %description -l en_US google -> Google, goggle, googly
libgcal-devel.x86_64: W: no-documentation
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

rpmlint libgcal-debuginfo-0.9.3-4.fc13.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.


 [x] Package is not relocatable.
 [x] Buildroot is correct
(%{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n))
 [x] Package is licensed with an open-source compatible license and meets other
legal requirements as defined in the legal section of Packaging Guidelines.
 [x] License field in the package spec file matches the actual license.
     License type per spec: BSD
 [x] If (and only if) the source package includes the text of the license(s) in
its own file, then that file, containing the text of the license(s) for the
package is included in %doc.
 [x] Spec file is legible and written in American English.
 [x] Sources used to build the package match the upstream source, as provided
in the spec URL.
     SHA1SUM of package: da2368f6ccd9b4a77fa435b3181d298f814ef4ff
libgcal-0.9.3.tar.bz2
 [x] Package is not known to require ExcludeArch
 [x] All build dependencies are listed in BuildRequires, except for any that
are listed in the exceptions section of Packaging Guidelines.
 [-] The spec file handles locales properly.
 [x] ldconfig called in %post and %postun if required.
 [x] Package must own all directories that it creates.
 [x] Package requires other packages for directories it uses.
 [x] Package does not contain duplicates in %files.
 [x] Permissions on files are set properly.
 [x] Package has a %clean section, which contains rm -rf $RPM_BUILD_ROOT.
 [x] Package consistently uses macros.
 [x] Package contains code, or permissable content.
 [-] Large documentation files are in a -doc subpackage, if required.
 [-] Package uses nothing in %doc for runtime.
 [x] Header files in -devel subpackage, if present.
 [-] Static libraries in -devel subpackage, if present.
 [x] Package requires pkgconfig, if .pc files are present.
 [x] Development .so files in -devel subpackage, if present.
 [-] Fully versioned dependency in subpackages, if present.
 [x] Package does not contain any libtool archives (.la).
 [-] Package contains a properly installed %{name}.desktop file if it is a GUI
application.
=>see preamble
 [x] Package does not own files or directories owned by other packages.
 [x] Final provides and requires are sane.

=== SUGGESTED ITEMS ===
 [x] Latest version is packaged.
 [x] Package does not include license text files separate from upstream.
 [-] Description and summary sections in the package spec file contains
translations for supported Non-English languages, if available.
 [x] Reviewer should test that the package builds in mock.
     Tested on: http://koji.fedoraproject.org/koji/taskinfo?taskID=2115844
 [x] Package should compile and build into binary rpms on all supported
architectures.
     Tested on: F13/x86_64
 [x] Package functions as described.
 [-] Scriptlets must be sane, if used.
 [x] The placement of pkgconfig(.pc) files is correct.
 [-] File based requires are sane.
 [-] %check is present and the test passes.







1) You Must drop the %{?dist} tag from the changelog
2) I Would like the spec file in standard format, but this isn't covered by review guidelines 

Package approved
Comment 16 leigh scott 2010-04-14 17:49:27 EDT
https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs


Your changelog should look like this (note the email address as well)


%changelog
* Sun Mar 21 2010 Mario Ceresa <mrceresa@gmail.com> 0.9.3-4
- Removed INSTALL from the devel package

* Thu Mar 13 2010 Mario Ceresa <mrceresa@gmail.com> 0.9.3-3
- Patched pc_include (Thanks to Peter Lemenkov for the patch)

* Thu Jan 21 2010 Mario Ceresa <mrceresa@gmail.com> 0.9.3-2
- Fixed the comments from the reviewers

* Tue Dec 29 2009 Mario Ceresa <mrceresa@gmail.com> 0.9.3-1
- Initial RPM Release
Comment 17 leigh scott 2010-04-14 17:52:28 EDT
Created attachment 406653 [details]
spec file

I would like to see your spec file use this format.
Comment 18 Mario Ceresa 2010-04-16 11:53:56 EDT
Hello Scott,
thanks for the approval.  I'll be out of town all the week and unable to apply your modifications on the package for now: I'll do that as soon as I'm back.
Comment 19 Mario Ceresa 2010-04-23 06:36:12 EDT
Hello Scott,
here there are the new spec and source rpm:

http://mrceresa.fedorapeople.org/libgcal.spec
http://mrceresa.fedorapeople.org/libgcal-0.9.3-4.fc12.src.rpm

and this is the CVS request:

New Package CVS Request
=======================
Package Name: libgcal
Short Description: A library to access google calendar events and contacts
Owners: mrceresa
Branches: F-12 F-13 EL-5
InitialCC: leigh123linux
Comment 20 Kevin Fenzi 2010-04-23 20:31:23 EDT
CVS done (by process-cvs-requests.py).
Comment 21 Rex Dieter 2010-08-06 21:47:30 EDT
Looks like this got imported and build for dist-f14 at least (closing).  I'd recommend getting at least f12, f13 builds done soonish too.
Comment 22 Mario Ceresa 2010-08-26 03:57:00 EDT
Hello everybody! 
Adenilson Cavalcanti from the libgcal's team, very kindly changed the new version of libgcal cmake's script to install headers in include/libgcal, as previously requested in comment 9.

The patch is visible at:

http://gitorious.org/libgcal/libgcal/commit/c06ef5e2269cea1d7533b999db19845b9a4a7a3f

When a new version of libgcal is released, we should remove from the specfile the part that copies the header in the new location.

I could make the change if you wish.

Thanks and regards,

Mario

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