Bug 676544 - Review Request: pidgin-logviewer - User friendly and intuitive way of viewing chat logs in Pidgin
Summary: Review Request: pidgin-logviewer - User friendly and intuitive way of viewin...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2011-02-10 07:40 UTC by Praveen Kumar
Modified: 2013-01-13 13:00 UTC (History)
8 users (show)

Fixed In Version: pidgin-logviewer-0.2-9.20110228svn15.fc15
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2011-03-03 13:11:08 UTC
Type: ---
Embargoed:
mtasaka: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)
screenshot of pidgin plugin setting (165.00 KB, image/png)
2011-02-26 18:48 UTC, Mamoru TASAKA
no flags Details

Description Praveen Kumar 2011-02-10 07:40:09 UTC
Spec URL: http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer.spec
SRPM URL: http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer-0.2-1.fc14.src.rpm
Description: This project aims to present an alternative, user-friendly and intuitive way of viewing chat logs in Pidgin. Currently the project is in an early stage, and is designed as a plugin, but it eventually aims to replace pidgin's default log viewer.

Comment 1 Rahul Sundaram 2011-02-11 04:30:20 UTC
I am not a sponsor and hence cannot approve but I will do a review.  You do not need to define a buildroot or have a %clean section any longer in the spec file.  These are defined by RPM automatically. 

Also you the license tag in the spec says GPLv2+ but the logplugin.c under src directory which is the only source file has a copyright notice that says:

--
/* Improved Log Viewer for Pidgin.
 * Tirtha Chatterjee
 * This code is licensed under GPL v2
 */ 
---

You need to talk to upstream on whether this is just GPLv2 or whether it is GPLv2 or later.  The plan for the author is to eventually merge this into Pidgin and hence I think GPLv2 is preferable but do confirm first.

Comment 2 Praveen Kumar 2011-02-11 08:52:35 UTC
Fixed all the issue which Mr. Rahul Sundaram pointed out, here is updated
spec file and srpm

Spec URL : http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer.spec

SRPM URL : http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer-0.2-2.fc14.src.rpm

Comment 3 Praveen Kumar 2011-02-11 10:46:09 UTC
Fixed all the issue which Mr. Rahul Sundaram pointed out, here is updated
spec file and srpm

Spec URL : http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer.spec

SRPM URL : http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer-0.2-2.fc14.src.rpm

Comment 4 Martin Gieseking 2011-02-14 14:54:50 UTC
The source archive contains prebuilt binaries in src/. These must be removed to ensure that all binaries are newly created during the build process.
Thus, add the following lines to %prep:
  cd src
  rm -rf .libs .deps *.o *.lo *.la

I also recommend to checkout the sources with "svn export" to avoid creating .svn directories.

Since you grabbed the sources from the svn trunk, the package provides a snapshot release. Please see here how to reflect this in the Release field:
http://fedoraproject.org/wiki/PackageNamingGuidelines#Snapshot_packages

If you want to maintain this package for EPEL < 6 too, you must add a %clean section. Otherwise, don't remove %clean only but also drop the BuildRoot tag and rm -rf $RPM_BUILD_ROOT (in %install).

Comment 5 Praveen Kumar 2011-02-15 04:39:00 UTC
Fixed all the issue which Mr. Martin Gieseking  pointed out, here is updated
spec file and srpm

Spec URL :
http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer.spec

SRPM URL :
http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer-0.2-3.20110208svn.fc14.src.rpm

Comment 6 Martin Gieseking 2011-02-15 08:41:52 UTC
The BuildRoot field is still present (below Source0). Please remove it.

I suggest to add the svn revision number to the Release tag. This helps to identify the exact sources used to build the package:
Release: 3.20110208svn13%{?dist}

Some (optional) cosmetic enhancements:

- You could shorten the Summary a bit, e.g.
  "User-friendly and intuitive chat log viewer for Pidgin"

- Please capitalize "Pidgin" in the %description (2nd sentence).


Besides that the package looks good now and works properly.


$ rpmlint /var/lib/mock/fedora-14-x86_64/result/*.rpm
pidgin-logviewer.src: W: spelling-error %description -l en_US plugin -> plug in, plug-in, plugging
pidgin-logviewer.src: W: invalid-url Source0: pidgin-logviewer-0.2.tar.gz
pidgin-logviewer.x86_64: W: spelling-error %description -l en_US plugin -> plug in, plug-in, plugging
3 packages and 0 specfiles checked; 0 errors, 3 warnings.

Comment 7 Praveen Kumar 2011-02-15 12:25:58 UTC
Issue Fixed. Please check out updated spec and srpm 

Spec URL :
http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer.spec

SRPM URL :
http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer-0.2-4.20110208svn13.fc14.src.rpm

Here is rpmlint output:
$rpmlint pidgin-logviewer.spec
pidgin-logviewer.spec: W: invalid-url Source0: pidgin-logviewer-0.2.tar.gz
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

Comment 8 Xavier Bachelot 2011-02-15 13:56:23 UTC
There something wrong with the commands to build the tarball :
#clone and tarred : svn export http://pidgin-logviewer.googlecode.com/svn/trunk/pidgin-logviewer-read-only
#tar -cvzf pidgin-logviewer-0.2.tar.gz pidgin-logviewer-0.2


$ svn export http://pidgin-logviewer.googlecode.com/svn/trunk/pidgin-logviewer-read-only
svn: URL 'http://pidgin-logviewer.googlecode.com/svn/trunk/pidgin-logviewer-read-only' doesn't exist

Comment 9 Praveen Kumar 2011-02-15 15:25:36 UTC
Fix svn(subversion) export issue. Please checkout updated specs and srpm


SPEC URL :
http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer.spec

SRPM URL :
http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer-0.2-5.20110208svn13.fc14.src.rpm

Here is rpmlint output:
$ rpmlint pidgin-logviewer.spec 
pidgin-logviewer.spec: W: invalid-url Source0: pidgin-logviewer-0.2.tar.gz
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

Comment 10 Xavier Bachelot 2011-02-15 22:00:32 UTC
That's still not right :
$ svn export http://pidgin-logviewer.googlecode.com/svn/trunk/ pidgin-logviewer-read-only
...snip...
Exported revision 13.
$ tar -cvzf pidgin-logviewer-0.2.tar.gz pidgin-logviewer-0.2
tar: pidgin-logviewer-0.2: Cannot stat: No such file or directory
tar: Exiting with failure status due to previous errors


The commands in the comments are expected to recreate the exact same tarball as you used for building. That is, if you made a snapshot at trunk revision 13, then the commands needs to do exactly that.

I would suggest something like that :
svn export -r13 http://pidgin-logviewer.googlecode.com/svn/trunk/ pidgin-logviewer
tar -cvzf pidgin-logviewer-0.2.tar.gz pidgin-logviewer

See https://fedoraproject.org/wiki/Packaging:SourceURL#Using_Revision_Control

Going further, you might want to have the snapshot revision and date defined in macros so you can reuse them in the spec.

So make it something like :
%global snapshot_date 20110215
%global snapshot_revision 13

Name: pidgin-logviewer
Version: 0.2
Release: 5.%{snapshot_date}svn%{snapshot_revision}%{?dist}

# svn export -r %{snapshot_revision} http://pidgin-logviewer.googlecode.com/svn/trunk/ pidgin-logviewer-%{version}
# tar -cvzf pidgin-logviewer-%{version}svn%{snapshot_revision}.tar.gz pidgin-logviewer-%{version}
Source0: %{name}-%{version}svn%{snapshot_revision}.tar.gz

Comment 11 Praveen Kumar 2011-02-16 04:15:03 UTC
Fixed Issue which Xavier Bachelot pointed out. Here is updated spec and srpm

SPEC URL :
http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer.spec
SRPM URL :
http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer-0.2-6.20110216svn13.fc14.src.rpm

Here is rpmlint output:
$rpmlint pidgin-logviewer.spec 
pidgin-logviewer.spec: W: invalid-url Source0: pidgin-logviewer-0.2svn13.tar.gz
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

Thanks for pointing those mistakes.

Comment 12 Mamoru TASAKA 2011-02-20 18:53:08 UTC
Some notes:

* Unwanted automated call of autotools
  - build.log shows that autotools scripts are automatically called
    after configure script is executed manually:
-------------------------------------------------------
    33  Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.wH1fI6
    46  + ./configure --build=i386-redhat-linux-gnu --host=i386-redhat-linux-gnu --program-prefix= --disable-dependency-tracking --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib --libexecdir=/usr/libexec --localstatedir=/var --sharedstatedir=/var/lib --mandir=/usr/share/man --infodir=/usr/share/info
   148  + make -j16
   149  CDPATH="${ZSH_VERSION+.}:" && cd . && /bin/sh /builddir/build/BUILD/pidgin-logviewer-0.2/config/missing --run aclocal-1.11 -I m4
   218  /bin/sh ./config.status --recheck
   369  rm -f stamp-h1
   370  touch config.h.in
   371  cd . && /bin/sh ./config.status config.h
   372  config.status: creating config.h
   373  config.status: config.h is unchanged
   374  make  all-recursive
-------------------------------------------------------

    This is not desirable because automated call of autotool methods
    may produce unwanted (or broken) scripts and anyway it should not
    be needed. Please fix this (usually timestamps on some files
    are wrong).

Comment 13 Mamoru TASAKA 2011-02-20 18:55:10 UTC
Then:
Praveen, do you have another review request submission or have tried
to do pre-review of other person's review request (at least one)?

Comment 14 Praveen Kumar 2011-02-20 19:04:21 UTC
(In reply to comment #13)
> Then:
> Praveen, do you have another review request submission or have tried
> to do pre-review of other person's review request (at least one)?
I have a another review request https://bugzilla.redhat.com/show_bug.cgi?id=627637

Thanks for reviewing.Soon I will fix those unwanted calls of autotools.

Comment 15 Mamoru TASAKA 2011-02-22 19:15:03 UTC
Removing needsponsor

Comment 16 Praveen Kumar 2011-02-24 09:26:30 UTC
(In reply to comment #12)
> Some notes:
> 
> * Unwanted automated call of autotools
>   - build.log shows that autotools scripts are automatically called
>     after configure script is executed manually:
> -------------------------------------------------------
>     33  Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.wH1fI6
>     46  + ./configure --build=i386-redhat-linux-gnu
> --host=i386-redhat-linux-gnu --program-prefix= --disable-dependency-tracking
> --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin
> --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include
> --libdir=/usr/lib --libexecdir=/usr/libexec --localstatedir=/var
> --sharedstatedir=/var/lib --mandir=/usr/share/man --infodir=/usr/share/info
>    148  + make -j16
>    149  CDPATH="${ZSH_VERSION+.}:" && cd . && /bin/sh
> /builddir/build/BUILD/pidgin-logviewer-0.2/config/missing --run aclocal-1.11 -I
> m4
>    218  /bin/sh ./config.status --recheck
>    369  rm -f stamp-h1
>    370  touch config.h.in
>    371  cd . && /bin/sh ./config.status config.h
>    372  config.status: creating config.h
>    373  config.status: config.h is unchanged
>    374  make  all-recursive
> -------------------------------------------------------
> 
>     This is not desirable because automated call of autotool methods
>     may produce unwanted (or broken) scripts and anyway it should not
>     be needed. Please fix this (usually timestamps on some files
>     are wrong).

I requested to upstream developer to check the timestamps and push on svn but according to him, he is not able to push updated timestamps files using svn. If any idea how to it please let me know.

Comment 17 Mamoru TASAKA 2011-02-24 18:23:06 UTC
One of workarounds is to execute
$ touch -r configure configure.ac Makefile.in */Makefile.in aclocal.m4 config.h.in
before calling %configure.

Comment 18 Praveen Kumar 2011-02-25 14:19:36 UTC

(In reply to comment #17)
> One of workarounds is to execute
> $ touch -r configure configure.ac Makefile.in */Makefile.in aclocal.m4
> config.h.in
> before calling %configure.
Thanks for suggestion 

Fix automated unwanted calls. Here is updated spec and srpm

SPEC URL : http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer.spec
SRPM URL :
http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer-0.2-7.20110225svn14.fc14.src.rpm

Comment 19 Mamoru TASAKA 2011-02-26 18:48:36 UTC
Created attachment 481173 [details]
screenshot of pidgin plugin setting

Now in good shape, however one point:

* I noticed that when launching pidgin and check "Tools ->
  Plugins", "Log Viewer" item does not show version information.
  This is because:
  - "version" file is missing
  - With autoconf 2.68 (F-15 uses this), "m4_esyscmd" (on
    the line 12 of configure.ac) doesn't work as expected.
    So even if I add "version" file and call "autoreconf -fi",
    VERSION macro cannot yet be defined.

  I prepared the srpm fixing this issue on:
  http://koji.fedoraproject.org/scratch/mtasaka/task_2869292/
  Would you check the srpm and consider to submit Patch0
  ( in my srpm ) to the upstream?

Comment 20 Praveen Kumar 2011-02-27 19:45:03 UTC
I updated configure.ac with your patch. Here is updated Spec and Srpm

SPEC URL : http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer.spec

SRPM URL: http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer-0.2-8.20110228svn15.fc14.src.rpm

Comment 21 Mamoru TASAKA 2011-02-27 20:23:53 UTC
Well, when "autoreconf -fi" is called, "touch -r ...." line should not
be needed (please check), however not a blocker.

------------------------------------------------------------
    This package (pidgin-logviewer) is APPROVED by mtasaka
------------------------------------------------------------

Comment 22 Praveen Kumar 2011-02-28 01:46:02 UTC
Removed "touch -r " from build section. Updated spec and srpm urls here

SPEC URL: http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer.spec

SRPM URL: http://kumarpraveen.fedorapeople.org/pidgin-logviewer/pidgin-logviewer-0.2-9.20110228svn15.fc14.src.rpm

Comment 23 Praveen Kumar 2011-02-28 01:49:19 UTC
New Package SCM Request
=======================
Package Name: pidgin-logviewew
Short Description: User-friendly and intuitive chat log viewer for Pidgin
Owners: kumarpraveen
Branches: f13 f14 f15

Comment 24 Jason Tibbitts 2011-02-28 14:58:26 UTC
The requested package name does not match the name in the ticket summary.

Please fix whichever is wrong and re-raise the fedora-cvs flag.

Comment 25 Praveen Kumar 2011-02-28 15:17:58 UTC
New Package SCM Request
=======================
Package Name: pidgin-logviewer
Short Description: User-friendly and intuitive chat log viewer for Pidgin
Owners: kumarpraveen
Branches: f13 f14 f15

Comment 26 Jason Tibbitts 2011-02-28 15:59:10 UTC
Git done (by process-git-requests).

Comment 27 Fedora Update System 2011-02-28 17:28:40 UTC
Package pidgin-logviewer-0.2-9.20110228svn15.fc15:
* should fix your issue,
* was pushed to the Fedora 15 updates-testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing pidgin-logviewer-0.2-9.20110228svn15.fc15'
as soon as you are able to, then reboot.
Please go to the following url:
https://admin.fedoraproject.org/updates/pidgin-logviewer-0.2-9.20110228svn15.fc15
then log in and leave karma (feedback).

Comment 28 Fedora Update System 2011-02-28 17:30:00 UTC
Package pidgin-logviewer-0.2-9.20110228svn15.fc14:
* should fix your issue,
* was pushed to the Fedora 14 updates-testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing pidgin-logviewer-0.2-9.20110228svn15.fc14'
as soon as you are able to, then reboot.
Please go to the following url:
https://admin.fedoraproject.org/updates/pidgin-logviewer-0.2-9.20110228svn15.fc14
then log in and leave karma (feedback).

Comment 29 Fedora Update System 2011-02-28 17:31:12 UTC
Package pidgin-logviewer-0.2-9.20110228svn15.fc13:
* should fix your issue,
* was pushed to the Fedora 13 updates-testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing pidgin-logviewer-0.2-9.20110228svn15.fc13'
as soon as you are able to, then reboot.
Please go to the following url:
https://admin.fedoraproject.org/updates/pidgin-logviewer-0.2-9.20110228svn15.fc13
then log in and leave karma (feedback).

Comment 30 Fedora Update System 2011-03-01 04:22:24 UTC
pidgin-logviewer-0.2-9.20110228svn15.fc13 has been pushed to the Fedora 13 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update pidgin-logviewer'.  You can provide feedback for this update here: https://admin.fedoraproject.org/updates/pidgin-logviewer-0.2-9.20110228svn15.fc13

Comment 31 Mamoru TASAKA 2011-03-03 13:11:08 UTC
For F-16, I fixed gstreamer-plugins-bad-free and then rebuilt
pidgin-logviewer.

Closing as NEXTRELEASE.

Comment 32 Fedora Update System 2011-03-11 20:51:40 UTC
pidgin-logviewer-0.2-9.20110228svn15.fc14 has been pushed to the Fedora 14 stable repository.

Comment 33 Fedora Update System 2011-03-11 20:56:38 UTC
pidgin-logviewer-0.2-9.20110228svn15.fc13 has been pushed to the Fedora 13 stable repository.

Comment 34 Fedora Update System 2011-03-17 03:25:19 UTC
pidgin-logviewer-0.2-9.20110228svn15.fc15 has been pushed to the Fedora 15 stable repository.


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