Bug 173035 - Review Request: chmlib - Library for dealing with ITSS/CHM format files
Review Request: chmlib - Library for dealing with ITSS/CHM format files
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: chmlib (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: John Mahowald
David Lawrence
http://66.93.236.84/~jedwin/projects/...
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-11-12 14:36 EST by Peter Lemenkov
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-08-28 17:01:47 EDT
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 18:17 EST, Patrice Dumas
no flags Details | Diff

  None (edit)
Description Peter Lemenkov 2005-11-12 14:36:48 EST
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 18:16:30 EST
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 18:17:58 EST
Created attachment 120994 [details]
patch for the spec file with cleanings
Comment 3 Peter Lemenkov 2005-11-13 00:47:43 EST
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 04:41:22 EST
Please consider adding -q to %setup.
Comment 5 Peter Lemenkov 2005-11-14 15:55:03 EST
(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-14 23:04:02 EST
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 13:57:23 EST
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 14:36:06 EST
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-09 20:11:19 EST
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 13:39:54 EST
> 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 10:34:00 EST
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 16:23:59 EST
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 16:27:49 EST
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 16:39:12 EDT
John, you should assign that bug to yourself.
Comment 15 John Mahowald 2006-08-28 17:00:48 EDT
Thanks, taking.
Comment 16 Peter Lemenkov 2007-06-02 00:19:56 EDT
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 11:50:39 EDT
CVS done.
Comment 18 Peter Lemenkov 2007-08-04 14:41:51 EDT
Package Change Request
======================
Package Name: chmlib
Updated EPEL Owners: lemenkov@gmail.com,pertusus@free.fr
Comment 19 Kevin Fenzi 2007-08-05 14:35:32 EDT
cvs done.

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