Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 453520 - Review Request: libUnihan - C library for Unihan character database in 5NF
Summary: Review Request: libUnihan - C library for Unihan character database in 5NF
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: UnihanDb
TreeView+ depends on / blocked
 
Reported: 2008-07-01 07:28 UTC by Ding-Yi Chen
Modified: 2008-09-18 07:08 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2008-09-18 07:08:20 UTC
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Ding-Yi Chen 2008-07-01 07:28:22 UTC
Spec URL: http://downloads.sourceforge.net/libunihan/libUnihan.spec
SRPM URL: http://downloads.sourceforge.net/libunihan/libUnihan-0.3-0.fc9.src.rpm  
Description:

Hi, I just finished packing the libUnihan and libUnihan-data. A review will be appreciated.

The project has two packages, one is libUnihan-data, which holds a large database (75M), the other is libUnihan, provides C library, header files, and so on.

The package to be reviewed is libUnihan. Please tell me what you think.

Regards,
Ding-Yi Chen

Comment 1 Ding-Yi Chen 2008-07-01 07:31:58 UTC
Sorry, the SRPM URL should be:
http://downloads.sourceforge.net/libunihan/libUnihan-0.3.0-0.fc9.src.rpm

Regards,
Ding-Yi Chen

Comment 2 Ding-Yi Chen 2008-07-04 08:36:01 UTC
I've updated the libUnihan to 0.3.1
New SRPM is now at:
http://downloads.sourceforge.net/libunihan/libUnihan-0.3.1-0.fc9.src.rpm  

Regards,
Ding-Yi Chen

Comment 3 Jens Petersen 2008-07-29 10:10:12 UTC
rpmlint is clean

Comment 5 Jens Petersen 2008-08-06 07:08:39 UTC
Still rpmlint clean :)

This looks good now.  I can do a full review tomorrow.

Comment 6 Mamoru TASAKA 2008-08-06 08:04:10 UTC
Some notes before Petersen-san do a full review:

* Seemingly redundant version specific (Build)Requires
  - Please explain why you want version specific (Build)Requires such as
    glib2-devel > 2.4, splite-devel > 3.0
    Even Fedora Core 3 shipped sqlite-3.1.2 and glib2-2.4.7.

* Cflags
  http://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags
  - Fedora specific compilation flags are not correctly honored:
    http://koji.fedoraproject.org/koji/taskinfo?taskID=761796
    And as a result the debuginfo rpm creation is not correct.

    By the way why do you call cmake twice?

* Unihan.h
  - %_includedir/Unihan.h contains:
-----------------------------------------------------------------
    29  #ifndef UNIHAN_H_
    30  #define UNIHAN_H_
    31  #include "config.h"
    32  #include "Unihan_enum.h"
    33  #include "str_functions.h"
------------------------------------------------------------------
    However I can find these 3 headers nowhere.
    Also please note that installing autotool-generated "config.h" as system-wide
    header file must be avoided:
    https://bugzilla.redhat.com/show_bug.cgi?id=208034#c43

* Directory ownership issue
  - %_datadir/doc/%name is not owned by any packages.
    By the way, do you really want two document directories: %_datadir/%doc/%name-version
    and %_datadir/doc/%name ? Please consider to unify document directories.

* linkage mistakes
  - rpmlint shows:
-------------------------------------------------------------------
$ rpmlint libUnihan
libUnihan.i386: W: undefined-non-weak-symbol /usr/lib/libUnihan.so.0.4 g_free
libUnihan.i386: W: undefined-non-weak-symbol /usr/lib/libUnihan.so.0.4 g_free
libUnihan.i386: W: undefined-non-weak-symbol /usr/lib/libUnihan.so.0.4 sqlite3_close
libUnihan.i386: W: undefined-non-weak-symbol /usr/lib/libUnihan.so.0.4 g_strsplit_set
libUnihan.i386: W: undefined-non-weak-symbol /usr/lib/libUnihan.so.0.4 sqlite3_value_int
libUnihan.i386: W: undefined-non-weak-symbol /usr/lib/libUnihan.so.0.4 sqlite3_result_text
libUnihan.i386: W: undefined-non-weak-symbol /usr/lib/libUnihan.so.0.4 g_strdup
...... (and many)
--------------------------------------------------------------------
   You can check this also by $ ldd -r /usr/lib/libUnihan.so.0.4 >/dev/null
   For shipping -devel subpackages this is not allowed because leaving undefined
   non-weak symbols will cause linkage error.

   This usually means that libUnihan.so is not correctly linked against proper libraries.
   Please fix this.

Comment 7 Mamoru TASAKA 2008-08-07 05:37:31 UTC
Assigning after some talk with Petersen-san.

Comment 8 Ding-Yi Chen 2008-08-08 04:13:33 UTC
Hi Mamoru,

I've fixed the most issues you raised. Except the double cmake,
as the cmake works a little bit like latex: first run generates various configure files, then the second run can build the Makefile successfully.

The ChangeLog is shown as follows:
========================

* Fri Aug 08 2008 Ding-Yi Chen <dchen at redhat dot com> - 0.4.1-0
- Header files have moved to {include}/libUnihan
- Completed the description of UnihanTable enumeration.
- Completed the description of str_functions.h
- In str_functions.h: unsignedStr_to_signedStr and signedStr_to_unsignedStr
clones the inputs now, while _buffer postfix for the circumstances that the buffer is given.
Addressing C#6 [Bug 453520] Review Request: libUnihan 
- rpmlint undefined-non-weak-symbol problems solved.
- Moved the include statement of private header files to Unihan_SQL_gen.c files.
- Move the doc/{name} files  to doc/{name}-{version}.
- Uhihan_enum.h and str_functions.h are now in API.

Comment 10 Mamoru TASAKA 2008-08-08 08:14:27 UTC
Now for 0.4.1-0:

* Source tarball
  - in the srpm does not coincide with what I downloaded from the written
    URL?
----------------------------------------------------------
70713 2008-08-08 11:28 libUnihan-0.4.1-0.fc9/libUnihan-0.4.1-Source.tar.gz
71729 2008-08-08 16:02 libUnihan-0.4.1-Source.tar.gz
----------------------------------------------------------

* Requires
** For main package
  - Now "Requires: glib2  sqlite" can (and should) be dropped as these 
    libraries' dependency are automatically checked by rpmbuild itself.

** For -devel subpackage:
  - For example, %_includedir/%name/Unihan_enum.h contains:
----------------------------------------------------------
    31  #ifndef UNIHAN_ENUM_H_
    32  #define UNIHAN_ENUM_H_
    33  #include <glib.h>
    34  #include <sqlite3.h>
----------------------------------------------------------
    This means libUnihan-devel should have "Requires: glib2-devel, sqlite-devel"
    Please also check for other header files.

* cflags
  - Fedora specific compilation flags are not correctly honored yet:
    http://koji.fedoraproject.org/koji/taskinfo?taskID=765959
    I am not a expert of cmake, however as far as I looked Makefiles,
-----------------------------------------------------------
make VERBOSE=1 C_DEFINES="$RPM_OPT_FLAGS" %{?_smp_mflags}
-----------------------------------------------------------
    seems to work for this issue

* Removing document files at %install
-----------------------------------------------------------
%install
rm -rf $RPM_BUILD_ROOT
make install DESTDIR=$RPM_BUILD_ROOT
rm  $RPM_BUILD_ROOT%{_docdir}/%{name}-%{version}/AUTHORS
rm  $RPM_BUILD_ROOT%{_docdir}/%{name}-%{version}/NEWS
rm  $RPM_BUILD_ROOT%{_docdir}/%{name}-%{version}/ChangeLog
rm  $RPM_BUILD_ROOT%{_docdir}/%{name}-%{version}/README
rm  $RPM_BUILD_ROOT%{_docdir}/%{name}-%{version}/COPYING
rm  $RPM_BUILD_ROOT%{_docdir}/%{name}-%{version}/COPYING.LESSER
-----------------------------------------------------------
  - Does this mean that you want to remove these files and
    _leave_ some other files which are installed under 
    $RPM_BUILD_ROOT%{_docdir}/%{name}-%{version}/ ?

    Actually build.log says that many html files and others are installed under
    $RPM_BUILD_ROOT%{_docdir}/%{name}-%{version}/ like:
-----------------------------------------------------------
   348  -- Installing: /builddir/rpmbuild/BUILDROOT/libUnihan-0.4.1-0.fc10.i386/usr/share/doc/libUnihan-0.4.1/html/tabs.css
   349  -- Installing: /builddir/rpmbuild/BUILDROOT/libUnihan-0.4.1-0.fc10.i386/usr/share/doc/libUnihan-0.4.1/html/doxygen.css
   350  -- Installing: /builddir/rpmbuild/BUILDROOT/libUnihan-0.4.1-0.fc10.i386/usr/share/doc/libUnihan-0.4.1/html/index.html
   351  -- Installing: /builddir/rpmbuild/BUILDROOT/libUnihan-0.4.1-0.fc10.i386/usr/share/doc/libUnihan-0.4.1/html/str__functions_8h-sou
rce.html
-----------------------------------------------------------

    However these files are removed anyway because you write
-----------------------------------------------------------
%doc AUTHORS NEWS ChangeLog README COPYING COPYING.LESSER
-----------------------------------------------------------
    With this %doc usage (i.e. the case in which %doc list is not given
    by full path), %doc first removes all files under 
    $RPM_BUILD_ROOT%{_docdir}/%{name}-%{version} and then (re-)installs
    files listed in %doc like:
-----------------------------------------------------------
   403  Executing(%doc): /bin/sh -e /var/tmp/rpm-tmp.t2hcK9
   404  + umask 022
   405  + cd /builddir/build/BUILD
   406  + cd libUnihan-0.4.1-Source
   407  + DOCDIR=/builddir/rpmbuild/BUILDROOT/libUnihan-0.4.1-0.fc10.i386/usr/share/doc/libUnihan-0.4.1
   408  + export DOCDIR
   409  + rm -rf /builddir/rpmbuild/BUILDROOT/libUnihan-0.4.1-0.fc10.i386/usr/share/doc/libUnihan-0.4.1
   410  + /bin/mkdir -p /builddir/rpmbuild/BUILDROOT/libUnihan-0.4.1-0.fc10.i386/usr/share/doc/libUnihan-0.4.1
   411  + cp -pr AUTHORS NEWS ChangeLog README COPYING COPYING.LESSER /builddir/rpmbuild/BUILDROOT/libUnihan-0.4.1-0.fc10.i386/usr/share
/doc/libUnihan-0.4.1
   412  + exit 0
-----------------------------------------------------------

   So:
   - If you want to once clean up all installed files under 
     $RPM_BUILD_ROOT%{_docdir}/%{name}-%{version}/ and use
     "%doc AUTHORS NEWS" method, simply use
-----------------------------------------------------------
rm -rf $RPM_BUILD_ROOT%{_docdir}/%{name}-%{version}/
-----------------------------------------------------------
     in %install.
   - Or you can leave the installed document files as it is and use:
-----------------------------------------------------------
%files
%defattr(-,root,root,-)
%dir %_docdir/%name-%version
%_docdir/%name-%version/AUTHORS
%_docdir/%name-%version/NEWS
....
....
%files doc
%defattr(-,root,root,-)
%_docdir/%name-%version/html/
-----------------------------------------------------------
     here all document files are installed %_docdir/%name-%version, while
     currently 3 directories are used to install document files.

     Note that all files under %_docdir are marked as %doc, so when
     writing
-----------------------------------------------------------
%files
%_docdir/%name-%version/AUTHORS
-----------------------------------------------------------
     %doc attribute is not needed.

! Duplicate/unneeded files
  - I don't think that "%doc AUTHORS NEWS ChangeLog README" is needed
    for -devel subpackage because -devel subpackage always "Requires"
    main package (but this is not a blocker).
    ! Here again see the usage of "%_docdir/%name-%version/AUTHORS" above.

  - By the way why is the file GPL (not LGPL) "COPYING" file needed?

Comment 11 Ding-Yi Chen 2008-08-11 01:49:57 UTC
Hi,

Thanks for point out those for me. 
I have addressed following concerns you raised:
* Requires
* cflags
* Removing document files at %install

However, as http://www.gnu.org/licenses/gpl-howto.html
states, LGPL software package has to include both COPYING and COPYING.LESSED.

The revised SPEC and SRPM are located at:
SPEC: http://downloads.sourceforge.net/libunihan/libUnihan.spec
SRPM: http://downloads.sourceforge.net/libunihan/libUnihan-0.4.1-1.fc9.src.rpm


Regards,
Ding-Yi Chen

Comment 12 Mamoru TASAKA 2008-08-11 05:56:49 UTC
Okay.

! Note:
  - I prefer to remove installed files once explicitly like:
    rm -rf  $RPM_BUILD_ROOT%{_docdir}/
    However this is not a blocker

-------------------------------------------------------------------
    This package (libUnihan) is APPROVED by mtasaka
-------------------------------------------------------------------

Comment 13 Mamoru TASAKA 2008-08-12 11:43:27 UTC
Please make a CVS request.

Comment 14 Ding-Yi Chen 2008-08-13 23:25:08 UTC
New Package CVS Request
=======================
Package Name: libUnihan
Short Description: C library for 5NF Unihan character database.
Owners: Ding-Yi Chen
Branches: F-8 F-9 EL-5
InitialCC: dchen
Packager Commits: yes

Comment 15 Kevin Fenzi 2008-08-23 17:37:02 UTC
cvs done.

Comment 16 Tony Fu 2008-09-10 03:13:07 UTC
requested by Jens Petersen (#27995)

Comment 17 Mamoru TASAKA 2008-09-17 15:29:10 UTC
Would you rebuild this also on F-8, and submit push request on bodhi?

Comment 18 Ding-Yi Chen 2008-09-18 05:19:06 UTC
Hi,
I've submitted the libUnihan-0.4.1.fc8 to bodhi. Thanks for informing it. 

Regards,
Ding-Yi Chen

Comment 19 Mamoru TASAKA 2008-09-18 07:08:20 UTC
Thanks. Closing this ticket.

Some notes:
* Now I guess you can build UnihanDb on F-9. When building please make it sure that
  F-10 branch UnihanDb has higher EVR (Epoch-Version-Release) than F-9 one.
  When you want to modify only F-9 branch rpms, you can use "4%{?dist}.1" release
  number, for example:
  https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Minor_release_bumps_for_old_branches

* For F-8 branch, if you want to build UnihanDb on F-8 please ask rel-eng team
  (rel-eng_AT_fedoraproject.org) to make F-8 libUnihanDb package tagged also as
   dist-f8-override. After that you should be able to build UnihanDb on F-8.


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