Bug 453520 - Review Request: libUnihan - C library for Unihan character database in 5NF
Review Request: libUnihan - C library for Unihan character database in 5NF
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Mamoru TASAKA
Fedora Extras Quality Assurance
:
Depends On:
Blocks: UnihanDb
  Show dependency treegraph
 
Reported: 2008-07-01 03:28 EDT by Ding-Yi Chen
Modified: 2008-09-18 03:08 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-09-18 03:08:20 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
mtasaka: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Ding-Yi Chen 2008-07-01 03:28:22 EDT
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 03:31:58 EDT
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 04:36:01 EDT
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 06:10:12 EDT
rpmlint is clean
Comment 5 Jens Petersen 2008-08-06 03:08:39 EDT
Still rpmlint clean :)

This looks good now.  I can do a full review tomorrow.
Comment 6 Mamoru TASAKA 2008-08-06 04:04:10 EDT
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 01:37:31 EDT
Assigning after some talk with Petersen-san.
Comment 8 Ding-Yi Chen 2008-08-08 00:13:33 EDT
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 04:14:27 EDT
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-10 21:49:57 EDT
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 01:56:49 EDT
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 07:43:27 EDT
Please make a CVS request.
Comment 14 Ding-Yi Chen 2008-08-13 19:25:08 EDT
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 13:37:02 EDT
cvs done.
Comment 16 Tony Fu 2008-09-09 23:13:07 EDT
requested by Jens Petersen (#27995)
Comment 17 Mamoru TASAKA 2008-09-17 11:29:10 EDT
Would you rebuild this also on F-8, and submit push request on bodhi?
Comment 18 Ding-Yi Chen 2008-09-18 01:19:06 EDT
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 03:08:20 EDT
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.