Bug 510865 (ccze) - Review Request: ccze - A robust log colorizer
Summary: Review Request: ccze - A robust log colorizer
Keywords:
Status: CLOSED ERRATA
Alias: ccze
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Susi Lehtola
QA Contact: Fedora Extras Quality Assurance
URL: http://bonehunter.rulez.org/CCZE.html
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-07-11 19:34 UTC by Pavel Alexeev
Modified: 2009-08-26 23:34 UTC (History)
5 users (show)

Fixed In Version: 0.2.1-5.fc11
Clone Of:
Environment:
Last Closed: 2009-08-06 23:37:50 UTC
Type: ---
Embargoed:
susi.lehtola: fedora-review+
j: fedora-cvs+


Attachments (Terms of Use)
Kwrite with tab with=5 space (102.90 KB, image/png)
2009-08-04 14:38 UTC, Pavel Alexeev
no flags Details
Mcedit screenshot (92.25 KB, image/png)
2009-08-04 14:39 UTC, Pavel Alexeev
no flags Details

Description Pavel Alexeev 2009-07-11 19:34:02 UTC
Spec URL: http://hubbitus.net.ru/rpm/Fedora11/ccze/ccze.spec
SRPM URL: http://hubbitus.net.ru/rpm/Fedora11/ccze/ccze-0.2.1-3.fc11.src.rpm
Description:
CCZE is a roboust and modular log colorizer, with plugins for apm,
exim, fetchmail, httpd, postfix, procmail, squid, syslog, ulogd,
vsftpd, xferlog and more.

http://koji.fedoraproject.org/koji/taskinfo?taskID=1467983

Comment 1 Susi Lehtola 2009-07-11 21:15:14 UTC
Doesn't the makefile support DESTDIR?
https://fedoraproject.org/wiki/Packaging/Guidelines#Why_the_.25makeinstall_macro_should_not_be_used

Comment 2 Pavel Alexeev 2009-07-12 09:12:24 UTC
Jussi Lehtola, you are right - http://hubbitus.net.ru/rpm/Fedora11/ccze/ccze.spec

Comment 3 Juha Tuomala 2009-07-29 20:18:34 UTC
http://bonehunter.rulez.org/CCZE.html
Not Found
The requested URL /CCZE.html was not found on this server.

Comment 4 Pavel Alexeev 2009-07-29 20:47:30 UTC
Upstream website dead many time. Is it really problem? You can see it in archive - link in comment in spec file.

Comment 5 Martin Gieseking 2009-08-04 07:46:41 UTC
I'd like to do the (informal) review, but the above linked SRPM is not available.
Also, the URL of the source tarball given in the spec file is dead.

Due to the lack of the sources, I'm unable to build the package. 

Concerning the spec file, you should correct BuildRoot. According to https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag
it must contain name, version and revision.

Comment 6 Susi Lehtola 2009-08-04 08:24:40 UTC
It seems the SRPM url is now

http://hubbitus.net.ru/rpm/Fedora11/ccze/ccze-0.2.1-4.fc11.src.rpm

Comment 7 Martin Gieseking 2009-08-04 10:29:51 UTC
rpmlint SRPMS/ccze-0.2.1-4.fc11.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

rpmlint RPMS/i586/ccze-*
2 packages and 0 specfiles checked; 0 errors, 0 warnings.

+ package name: ok
+ spec file name: ok
+ valid license: ok
+ license tag: ok
- source tarball not available upstream
- BuildRoot doesn't comply with packaging guidelines (see comment #5)
+ BuildRequire tags seem to be complete
- you should add %{?_smp_mflags} to the make statement
+ no locales available
+ no duplicate files listed
+ files section and permissions: ok
+ package owns its files
+ no development files (headers, libraries)
+ no large documentation
+ no sub-packages
+ no pkgconfig files
+ install section: ok
+ no scriptlets

Comment 8 Susi Lehtola 2009-08-04 10:40:06 UTC
I'm going to sponsor Martin, doing full review.

Comment 10 Susi Lehtola 2009-08-04 10:51:48 UTC
rpmlint output is clean.


MUST: The package does not yet exist in Fedora. The Review Request is not a duplicate. OK
MUST: The spec file for the package is legible and macros are used consistently. NEEDSWORK
- If you use tabbing, do it properly: remove the extra space from Summary, Version,  Release and so on.
- There is no comment on what patch0 does.
- Fix the buildroot as per comment #5.

MUST: The package must be named according to the Package Naming Guidelines. OK
MUST: The spec file name must match the base package %{name}. OK
MUST: The package must be licensed with a Fedora approved license and meet the  Licensing Guidelines. OK
MUST: The License field in the package spec file must match the actual license. OK

MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. ??
- Upstream website is down.

MUST: The package MUST successfully compile and build into binary rpms. OK
MUST: The spec file MUST handle locales properly. N/A

MUST: Optflags are used and time stamps preserved. OK
- Man page is generated, so no need to preserve time stamp in install.
- Header file time stamp should be preserved, but it is not included in the tarball.

MUST: Packages containing shared library files must call ldconfig. N/A
MUST: A package must own all directories that it creates or require the package that owns the directory. OK
MUST: Files only listed once in %files listings. OK
MUST: Debuginfo package is complete. OK
MUST: Permissions on files must be set properly. OK
MUST: Clean section exists. OK
MUST: Large documentation files must go in a -doc subpackage. N/A
MUST: All relevant items are included in %doc. Items in %doc do not affect runtime of application. OK
MUST: Header files must be in a -devel package. N/A
MUST: Static libraries must be in a -static package. N/A
MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig'. N/A
MUST: If a package contains library files with a suffix then library files ending in .so must go in a -devel package. N/A
MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency. N/A
MUST: Packages does not contain any .la libtool archives. N/A
MUST: Desktop files are installed properly. N/A
MUST: No file conflicts with other packages and no general names. OK
MUST: Buildroot cleaned before install. OK
SHOULD: %{?dist} tag is used in release. OK
SHOULD: If the package does not include license text(s) as separate files from upstream, the packager should query upstream to include it. OK
SHOULD: The package builds in mock. OK

Comment 11 Pavel Alexeev 2009-08-04 11:28:29 UTC
(In reply to comment #10)
> MUST: The package does not yet exist in Fedora. The Review Request is not a
> duplicate. OK
> MUST: The spec file for the package is legible and macros are used
> consistently. NEEDSWORK
> - If you use tabbing, do it properly: remove the extra space from Summary,
> Version,  Release and so on.
There only tabs. On case mixing it, rpmlint comply it.

> - There is no comment on what patch0 does.
Ok. I comment it.
> - Fix the buildroot as per comment #5.
It was already done.

> MUST: The sources used to build the package must match the upstream source, as
> provided in the spec URL. ??
> - Upstream website is down.
I known. :(
What you can suggest do with that? Must I "fork" project to continue?

Comment 12 Susi Lehtola 2009-08-04 11:52:23 UTC
(In reply to comment #11)
> (In reply to comment #10)
> > MUST: The package does not yet exist in Fedora. The Review Request is not a
> > duplicate. OK
> > MUST: The spec file for the package is legible and macros are used
> > consistently. NEEDSWORK
> > - If you use tabbing, do it properly: remove the extra space from Summary,
> > Version,  Release and so on.
> There only tabs. On case mixing it, rpmlint comply it.

That was a general expression - remove the extra tabs.
 
> > MUST: The sources used to build the package must match the upstream source, as
> > provided in the spec URL. ??
> > - Upstream website is down.
> I known. :(
> What you can suggest do with that? Must I "fork" project to continue?  

I guess we just have to live with it.

Comment 13 Pavel Alexeev 2009-08-04 12:43:10 UTC
(In reply to comment #12)
> That was a general expression - remove the extra tabs.
There is not extra tabs! I think this misunderstood because I use tab with in 5 space (this is cose t easy distinguish tabs and space aligments).

Comment 14 Susi Lehtola 2009-08-04 12:49:25 UTC
(In reply to comment #13)
> (In reply to comment #12)
> > That was a general expression - remove the extra tabs.
> There is not extra tabs! I think this misunderstood because I use tab with in 5
> space (this is cose t easy distinguish tabs and space aligments).  

Summary:                A robust log colorizer
Summary(ru):    Мощный коллоризатор логов
Name:           ccze
Version:                0.2.1
Release:                5%{?dist}
# http://web.archive.org/web/20040803024236/bonehunter.rulez.org/CCZE.phtml
URL:                    http://bonehunter.rulez.org/CCZE.html
License:                GPLv2+
Group:          Applications/Text

This is ugly. What it should look like is

Summary:        A robust log colorizer
Summary(ru):    Мощный коллоризатор логов
Name:           ccze
Version:        0.2.1   
Release:        5%{?dist}
# http://web.archive.org/web/20040803024236/bonehunter.rulez.org/CCZE.phtml
URL:            http://bonehunter.rulez.org/CCZE.html
License:        GPLv2+  

You have one tab too much in some lines.

Comment 15 Pavel Alexeev 2009-08-04 13:19:11 UTC
>You have one tab too much in some lines.
Again - ONLY if as you, expand tab to 8 spaces! If you expand its in 5, all indentation seems ok. :)

Comment 16 Susi Lehtola 2009-08-04 13:36:52 UTC
(In reply to comment #15)
> >You have one tab too much in some lines.
> Again - ONLY if as you, expand tab to 8 spaces! If you expand its in 5, all
> indentation seems ok. :)  

The tab '\t' is not expanded to ' 's. The spec file is messy in Firefox, Chromium, vim, emacs and nano. Just remove the extra tabs, OK?

Comment 17 Pavel Alexeev 2009-08-04 14:38:02 UTC
Created attachment 356180 [details]
Kwrite with tab with=5 space

There are NO EXTRA TABS!

Please see screenshot in Kwrite and console mcedit. What tabs there "extra"??

Comment 18 Pavel Alexeev 2009-08-04 14:39:00 UTC
Created attachment 356181 [details]
Mcedit screenshot

Comment 19 Susi Lehtola 2009-08-04 15:01:27 UTC
Okay.

I tested with Chromium, Firefox, Emacs, Vim and Nano and all of them displayed the spec file otherwise. 8 character tabbing is the standard, and you should use it.

**

The package has been

APPROVED.

Comment 20 Pavel Alexeev 2009-08-04 16:13:13 UTC
(In reply to comment #19)
> 8 character tabbing is the standard
Off course
>, and you should use it.
but not must ;)

> The package has been
> 
> APPROVED.  

Thank you very much for review!

Comment 21 Pavel Alexeev 2009-08-04 16:15:12 UTC
New Package CVS Request
=======================
Package Name: ccze
Short Description: A robust log colorizer
Owners: hubbitus
Branches: F-10 F-11 EL-5
InitialCC:

Comment 22 Jason Tibbitts 2009-08-04 20:31:45 UTC
CVS done.

Comment 23 Ralf Corsepius 2009-08-05 04:02:48 UTC
(In reply to comment #19)
> 8 character tabbing is the standard,
You are in error. 8 char tabs are pretty common, but that's all.

> and you should use it.
You've got a learning curve ahead - Better concentrate on technical issues and not on cosmetic issues in reviews.

Comment 24 Susi Lehtola 2009-08-05 11:14:15 UTC
(In reply to comment #23)
> > and you should use it.
> You've got a learning curve ahead - Better concentrate on technical issues and
> not on cosmetic issues in reviews.  

Yes, this was a minor cosmetic issue (would have been easy to fix too). I've worked on 130 package reviews so far, and this has been the only one using a different tab width.

Having two incompatible conventions is really not very useful. Converting the tabs to spaces might be a good option, since that way the tabbing is always correct.

Comment 25 Ralf Corsepius 2009-08-05 12:17:52 UTC
(In reply to comment #24)
> (In reply to comment #23)
> > > and you should use it.
> > You've got a learning curve ahead - Better concentrate on technical issues and
> > not on cosmetic issues in reviews.  
> 
> Yes, this was a minor cosmetic issue (would have been easy to fix too). I've
> worked on 130 package reviews so far, and this has been the only one using a
> different tab width.
Again, tab widths are personal preference, you can agree with or not.

That said, your personal preference is irrelevant.

Also, what might be news to apparent newcomers with only little programming experience like you: There exist large communities of programmers who are using other tab widths as their standard (e.g. FreeBSD uses 4, many people with vi-background use 3, etc.)


> Having two incompatible conventions is really not very useful. 
Cosmetics are irrelevant.

> Converting the
> tabs to spaces might be a good option, since that way the tabbing is always
> correct.  
That's a newbie's bikeshed.

Comment 26 Pavel Alexeev 2009-08-06 22:52:54 UTC
Ralf Corsepius, thank you for the support...

Comment 27 Fedora Update System 2009-08-06 22:54:31 UTC
ccze-0.2.1-5.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/ccze-0.2.1-5.el5

Comment 28 Fedora Update System 2009-08-06 22:55:03 UTC
ccze-0.2.1-5.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/ccze-0.2.1-5.fc10

Comment 29 Fedora Update System 2009-08-06 22:55:37 UTC
ccze-0.2.1-5.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/ccze-0.2.1-5.fc11

Comment 30 Fedora Update System 2009-08-25 04:27:33 UTC
ccze-0.2.1-5.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 31 Fedora Update System 2009-08-25 04:37:14 UTC
ccze-0.2.1-5.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 32 Fedora Update System 2009-08-26 23:34:33 UTC
ccze-0.2.1-5.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.


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