Bug 453520

Sorry, the SRPM URL should be:
http://downloads.sourceforge.net/libunihan/libUnihan-0.3.0-0.fc9.src.rpm

Regards,
Ding-Yi Chen

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

rpmlint is clean

New SPEC and SRPM for libUnihan 0.4.0 are at:

SPEC: http://downloads.sourceforge.net/libunihan/libUnihan.spec?use_mirror=voxel&filesize=3062

SRPM: http://downloads.sourceforge.net/libunihan/libUnihan-0.4.0-0.fc9.src.rpm?use_mirror=voxel&filesize=72990

Still rpmlint clean :)

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

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.

Assigning after some talk with Petersen-san.

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.

The NEW SPEC: http://downloads.sourceforge.net/libunihan/libUnihan.spec?use_mirror=internap&filesize=3748

and the NEW SRPM: http://downloads.sourceforge.net/libunihan/libUnihan-0.4.1-0.fc9.src.rpm?use_mirror=internap&filesize=75811

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?

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

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
-------------------------------------------------------------------

Please make a CVS request.

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

cvs done.

requested by Jens Petersen (#27995)

Would you rebuild this also on F-8, and submit push request on bodhi?

Hi,
I've submitted the libUnihan-0.4.1.fc8 to bodhi. Thanks for informing it. 

Regards,
Ding-Yi Chen

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.