Bug 173035 - Review Request: chmlib - Library for dealing with ITSS/CHM format files
Summary: Review Request: chmlib - Library for dealing with ITSS/CHM format files
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: chmlib   
(Show other bugs)
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: John Mahowald
QA Contact: David Lawrence
URL: http://66.93.236.84/~jedwin/projects/...
Whiteboard:
Keywords:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2005-11-12 19:36 UTC by Peter Lemenkov
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-08-28 21:01:47 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora-cvs+


Attachments (Terms of Use)
patch for the spec file with cleanings (1.13 KB, patch)
2005-11-12 23:17 UTC, Patrice Dumas
no flags Details | Diff

Description Peter Lemenkov 2005-11-12 19:36:48 UTC
Spec Name or Url: http://paula.comtv.ru/chmlib.spec
SRPM Name or Url: http://paula.comtv.ru/chmlib-0.37.4-1.src.rpm

Description: CHMLIB is a library for dealing with ITSS/CHM format files. Right now, it is a very simple library, but sufficient for dealing with all of the .chm files I've come across. Due to the fairly well-designed indexing built into this particular file format, even a small library is able to gain reasonably good performance indexing into ITSS archives.

Comment 1 Patrice Dumas 2005-11-12 23:16:30 UTC
I made a diff for the following issues:

- use simpler {__make} DESTDIR=%{buildroot} install instead of %makeinstall
- don't package .la files
- add %post/postun -p /sbin/ldconfig
- cleaner deffatr
- add %doc, especially licence
- don't own %{includedir}
- add version-release 
- in devel Requires:       %{name} = %{version}-%{release}

One thing remains to be corrected: the summary for the devel package is too long.

Comment 2 Patrice Dumas 2005-11-12 23:17:58 UTC
Created attachment 120994 [details]
patch for the spec file with cleanings

Comment 3 Peter Lemenkov 2005-11-13 05:47:43 UTC
OK, updated.

Spec Name or Url: http://paula.comtv.ru/chmlib.spec
SRPM Name or Url: http://paula.comtv.ru/chmlib-0.37.4-1.src.rpm

Comment 4 Ignacio Vazquez-Abrams 2005-11-14 09:41:22 UTC
Please consider adding -q to %setup.

Comment 5 Peter Lemenkov 2005-11-14 20:55:03 UTC
(In reply to comment #4)
> Please consider adding -q to %setup.

Done.

Spec Name or Url: http://paula.comtv.ru/chmlib.spec
SRPM Name or Url: http://paula.comtv.ru/chmlib-0.37.4-1.src.rpm



Comment 6 John Mahowald 2005-12-15 04:04:02 UTC
A couple warnings you want to tell upstream about, like "warning: "memcpy"
redefined"


- rpmlint's happy except for some no-documentation from the devel package
- package meets naming guidelines
- package meets packaging guidelines
- license (LGPL) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on FC4 i386
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- %clean ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file
- devel package ok
- no .la files
- post/postun ldconfig ok
- devel requires base package n-v-r 


One change, the BuildRoot needs to be cleaned at the beginning of %install, like
%clean. Do this and this is APPROVED.

Comment 7 Peter Lemenkov 2005-12-17 18:57:23 UTC
Failed build for PPC arch.

I haven't access to the PPC-computer, so I can't find out what's wrong exactly.

Log:

http://buildsys.fedoraproject.org/logs/fedora-development-extras/1868-chmlib-0.37.4-1.fc5/ppc/build.log

Error was generated by #error-directive at that string "src/chm_lib.c:186". If
we look there, we'll find this piece of code:

=========================================================

/* i386, 32-bit, Windows */
#ifdef WIN32
typedef unsigned char           UChar;
typedef __int16                 Int16;
typedef unsigned __int16        UInt16;
typedef __int32                 Int32;
typedef unsigned __int32        UInt32;
typedef __int64                 Int64;
typedef unsigned __int64        UInt64;

/* I386, 32-bit, non-Windows */
/* Sparc        */
/* MIPS         */
/* PPC          */
#elif __i386__ || __sun || __sgi || __ppc__
typedef unsigned char           UChar;
typedef short                   Int16;
typedef unsigned short          UInt16;
typedef long                    Int32;
typedef unsigned long           UInt32;
typedef long long               Int64;
typedef unsigned long long      UInt64;

/* x86-64 */
/* Note that these may be appropriate for other 64-bit machines. */
#elif __x86_64__
typedef unsigned char           UChar;
typedef short                   Int16;
typedef unsigned short          UInt16;
typedef int                     Int32;
typedef unsigned int            UInt32;
typedef long                    Int64;
typedef unsigned long           UInt64;

#else

/* yielding an error is preferable to yielding incorrect behavior */
#error "Please define the sized types for your platform in chm_lib.c"
#endif

=========================================================

Looks like wrong arch-definition at that place:

> #elif __i386__ || __sun || __sgi || __ppc__

What arch-macro defined by gcc at PPC-box? __ppc32__? __ppc64__?

Comment 8 Thomas Sailer 2005-12-17 19:36:06 UTC
Instead of using architecture defines, why don't you just #include <stdint.h>  
and then typedef uint32_t UInt32; ? 

Comment 9 David Woodhouse 2006-01-10 01:11:19 UTC
Peter, I did answer your question on the mailing list, although the broken list
Reply-To: settings mean that you didn't receive a direct copy of my mail -- it's
'__powerpc__'. 

However, Thomas's suggestion is the correct answer. Please make it use standard
C99 types, instead of the mess of hackish ifdefs.

Comment 10 Peter Lemenkov 2006-01-10 18:39:54 UTC
> Peter, I did answer your question on the mailing list, although the broken list
> Reply-To: settings mean that you didn't receive a direct copy of my mail -- it's
> '__powerpc__'. 

Thanks. Builds fine now.

> However, Thomas's suggestion is the correct answer. Please make it use standard
> C99 types, instead of the mess of hackish ifdefs.

I'll send e-mail to the developer of chmlib ASAP.

Comment 11 Jose Pedro Oliveira 2006-01-25 15:34:00 UTC
Peter,

Could you build chmlib for FC-4 and FC-3?
The FC-3 and FC-4 CVS branches can be created via the Wiki page: 
http://fedoraproject.org/wiki/Extras/CVSSyncNeeded

Thanks,
jpo

Comment 12 Jose Pedro Oliveira 2006-02-10 21:23:59 UTC
Peter,

As I didn't get any answer to my previous post I took the liberty of building
chmlib for FC-4.

Best regards,
jpo

Comment 13 Jose Pedro Oliveira 2006-02-10 21:27:49 UTC
Hi,

Is someone planning to submit and maintain one of these CHM viewers?

* kchmviewer
  http://www.kchmviewer.net/

* xchm
  http://xchm.sourceforge.net/

* kchm
  http://kchm.sourceforge.net/

Regards,
jpo

Comment 14 Patrice Dumas 2006-07-21 20:39:12 UTC
John, you should assign that bug to yourself.

Comment 15 John Mahowald 2006-08-28 21:00:48 UTC
Thanks, taking.

Comment 16 Peter Lemenkov 2007-06-02 04:19:56 UTC
Package Change Request
======================
Package Name: chmlib
New Branches: EL-4 EL-5
Updated Fedora Owners: lemenkov@gmail.com,pertusus@free.fr
Updated EPEL Owners: pertusus@free.fr

Comment 17 Jason Tibbitts 2007-06-02 15:50:39 UTC
CVS done.

Comment 18 Peter Lemenkov 2007-08-04 18:41:51 UTC
Package Change Request
======================
Package Name: chmlib
Updated EPEL Owners: lemenkov@gmail.com,pertusus@free.fr


Comment 19 Kevin Fenzi 2007-08-05 18:35:32 UTC
cvs done.


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