[Bug 887778] Review Request: cutter - A Unit Testing Framework for C

[bugzilla at redhat.com]

Product: Fedora
https://bugzilla.redhat.com/show_bug.cgi?id=887778

--- Comment #4 from HAYASHI Kentaro <kenhys at gmail.com> ---
Thank you for reviewing!!

Here is the updated spec and SRPM.

Spec URL: http://sourceforge.net/projects/cutter/files/tmp/cutter.spec/download
SRPM URL:
http://sourceforge.net/projects/cutter/files/tmp/cutter-1.2.2-3.fc17.src.rpm

(In reply to comment #2)
> 
> > Summary: A Unit Testing Framework for C/C++
> 
> Similar to your -devel package %summary, omitting the leading article makes
> it more readable. 
> 
>   Summary: Unit Testing Framework for C/C++
> 
> Once can more quickly focus on the importing part.
> 
> > Summary:        Libraries and header files for Cutter development
> 
> And as one can see, here you also didn't start with "The libraries and ...".
> :-)
> 

Fixed!

> 
> > Group: Development/Tools
> 
> Rather "Development/Libraries". Perhaps "Development/Languages". Afterall,
> this isn't just a utility/tool to install and run.
> 

Changed to Development/Libraries category.

> 
> > BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-%(%{__id_u} -n)
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag
> 
> The "fedora-review" tool complains about this tag, too.
> 

Removed BuildRoot tag.

> 
> > cutter-1.2.2/glib-compatible/pcre/pcre_fullinfo.c
> 
> The fedore-review tool here discovers license "BSD (3 clause)", but a closer
> look reveals that this is an old bundled PCRE lib for glib 2.12, which isn't
> used for a newer glib2.
> 
> glib-compatible/gregex.c:
> 
>     28  #ifdef USE_SYSTEM_PCRE
>     29  #include <pcre.h>
>     30  #else
>     31  #include "pcre/pcre.h"
>     32  #endif
> 
> So, only for completeness and in case any older target system features an
> old glib, I need to point at 
> https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries  and it may be
> safer to delete this bundled lib at the end of the %prep section, so if used
> it would cause a build failure. ;-)
> 

Removed in %prep section.

> 
> > BuildRequires: intltool
> > BuildRequires: glib2-devel
> > BuildRequires: libsoup-devel
> 
> > checking for GTK+ - version >= 2.12.0... no
> > *** Could not run GTK+ test program, checking why...
> > *** The test program failed to compile or link. See the file config.log for the
> > *** exact error that occured. This usually means GTK+ is incorrectly installed.
> 
> This warning in the build.log made me curious. What happens if one adds
> "BuildRequires: gtk2-devel"? The build fails:
> 
> error: Installed (but unpackaged) file(s) found:
>    /usr/lib64/libgdkcutter-pixbuf.so
>    /usr/lib64/libgdkcutter-pixbuf.so.0
>    /usr/lib64/libgdkcutter-pixbuf.so.0.1.0
> 
> Why isn't gtk2-devel built with?
> Is anything else missing? The build output contains a few more "No" answers
> to configure existance checks.
> https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires
> 
> For sake of "reproducible builds", it's common practice to either add
> missing BuildRequires or add --disable-foo options to explicitly build
> without certain features, even if the build dependencies are installed.
> 

According to dependency, split cutter package into subpackages!
-> cutter-report
   cutter-gui
   cutter-gstreamer
> 
> 
> > %package devel
> > Group:          Development/Tools
> 
> Same suggestion as above.
>

Fixed.

> > Requires:       %{name} = %{version}-%{release}
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package
> 

Fixed.

> 
> > make %{?_smp_mflags}
> 
> Please prepend  V=1  for verbose build output. That makes build system
> build.log contents much more useful in case of errors. That's also the only
> way to see that the global compiler flags are used actually (not just prior
> for the configure check):
> https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags
> 
> 
> > %install
> > rm -rf %{buildroot}
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag
> 

Fixed not to rm -rf.

> 
> > find %{buildroot} -name keep-me -size 0 -delete
> 
> Hmmm? :)  An excellent opportunity to add a brief comment to the spec file
> _why_  this is being done.
> 
> $ find cutter-1.2.2|grep keep
> cutter-1.2.2/sample/stack/config/keep-me
> 
'keep-me' file was added to distribute config directory purpose,
but that directory would be kind of automatically generated. 
So I've fixed in upstream. (patch will be dropped in the next release)

> 
> > %find_lang %{name}
> > %{_mandir}/ja/man1/*
> 
> You could use
> 
>   %find_lang %{name} --with-man --all-name
> 
> and not only would it find and add also the translated (currently only
> Japanese) manual pages (instead of you having to add then in the %files
> section manually), it would also flag them with the corresponding lang(…)
> marker. For example, the suggested %find_lang command would add these
> entries:
> 
> %lang(ja) /usr/share/locale/ja/LC_MESSAGES/cutter.mo
> %lang(ja) /usr/share/man/ja/man1/*
> 

Fixed.

> 
> > cutter-1.2.2/test/
> 
> Sounds like this could/should be added to %check section.
> 
>   %check
>   V=1 LD_LIBRARY_PATH=$(pwd)/cutter/.libs make check
> 
> Objections? Should I take a second look? ;)
> 
> 

Added %check entry.

> $ rpmls -p cutter-1.2.2-2.fc18.3.x86_64.rpm|grep ^d
> drwxr-xr-x  /usr/lib64/cutter/module
> drwxr-xr-x  /usr/lib64/cutter/module/factory
> drwxr-xr-x  /usr/lib64/cutter/module/factory/report
> drwxr-xr-x  /usr/lib64/cutter/module/factory/stream
> drwxr-xr-x  /usr/lib64/cutter/module/factory/ui
> drwxr-xr-x  /usr/lib64/cutter/module/report
> drwxr-xr-x  /usr/lib64/cutter/module/stream
> drwxr-xr-x  /usr/lib64/cutter/module/ui
> drwxr-xr-x  /usr/share/cutter/icons/kinotan
> drwxr-xr-x  /usr/share/cutter/ui
> drwxr-xr-x  /usr/share/doc/cutter-1.2.2
> 
> https://fedoraproject.org/wiki/Packaging:UnownedDirectories
> 
> /usr/lib64/cutter is not included in the package.
> 
> /usr/share/cutter is not included in the package.
> 
> /usr/share/cutter/icons is not included in the package.
> 

Fixed.

> > %{_datadir}/cutter/license/*
> 
> /usr/share/cutter/license is not included in the package.
> 
> The license files are not marked as %doc.
> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text
> 

Fixed.

> 
> * /usr/share/cutter/ui is empty. An oversight or future-proof?
> 
When cutter is built with GTK+, GTK+ frontend module contains definition file
under that directory. now there is gtk-menu.ui (cutter-gui package).

> 
> * cutter.1 man page (also the translated one) refers to
> /usr/local/share/doc/cutter/
> This path could be substituted at build-time.
> %{_defaultdocdir}/%{name}-%{version}-%{release}


Fixed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=MFZR7Paf20&a=cc_unsubscribe