Bug 887778 - Review Request: cutter - A Unit Testing Framework for C
Review Request: cutter - A Unit Testing Framework for C
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Michael Schwendt
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-17 05:26 EST by HAYASHI Kentaro
Modified: 2013-02-18 20:24 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-02-18 20:24:10 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
bugs.michael: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
remaining spec issues (1.40 KB, patch)
2013-02-05 09:26 EST, Michael Schwendt
no flags Details | Diff

  None (edit)
Description HAYASHI Kentaro 2012-12-17 05:26:27 EST
Spec URL: http://sourceforge.net/projects/cutter/files/tmp/cutter.spec
SRPM URL: http://sourceforge.net/projects/cutter/files/tmp/cutter-1.2.2-1.fc17.src.rpm

Description: 
Cutter is a xUnit family Unit Testing Framework for C.
Cutter provides easy to write test and easy to debug code environment.
Fedora Account System Username: kenhys

Hi, I am a member of Cutter projects (http://cutter.sourceforge.net/),
 and I had released Cutter a few times ever.
Cutter provides RPM packages for Fedora 17 (and CentOS 5 or CentOS 6) by cutter repositories,
 but It is a time to migrate maintance and release process into Fedora project, I think.

This is the first time, I have posted package review request.

Here is the result of rpmlint -i:

$ rpmlint -i cutter-1.2.2-1.fc17.x86_64.rpm|grep E
(Nothing, No errors)
$ rpmlint -i cutter-1.2.2-1.fc17.x86_64.rpm|grep W
cutter.x86_64: W: spelling-error %description -l en_US xUnit -> x Unit, unit
cutter.x86_64: W: devel-file-in-non-devel-package /usr/include/cutter/cutter/cut-test-iterator.h
cutter.x86_64: W: devel-file-in-non-devel-package /usr/lib64/pkgconfig/libcutter.pc
cutter.x86_64: W: devel-file-in-non-devel-package /usr/include/cutter/cutter.h
cutter.x86_64: W: devel-file-in-non-devel-package /usr/include/cutter/cutter/cut-test-utils.h
...

There are many warning but classified into two types ones.
The first spelling-error is false detection, the seconds one and followings are derived from cutter package does not
provide -devel package. (This is same as cxxtest package style)
Comment 1 HAYASHI Kentaro 2012-12-18 03:43:52 EST
Hi, I had found that cxxtest package style (no -devel package) is exceptional case, so I have split libraries and header files into 
-devel package.

Here is the updated spec and SRPM.

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

Here is the result of rpmlint -i:

% rpmlint -i cutter-1.2.2-2.fc17.x86_64.rpm cutter-devel-1.2.2-2.fc17.x86_64.rpm
cutter.x86_64: W: spelling-error %description -l en_US xUnit -> x Unit, unit
The value of this tag appears to be misspelled. Please double-check.

cutter-devel.x86_64: W: spelling-error %description -l en_US xUnit -> x Unit, unit
The value of this tag appears to be misspelled. Please double-check.

2 packages and 0 specfiles checked; 0 errors, 2 warnings.

There are two warnings above, but it's false detection.
Comment 2 Michael Schwendt 2012-12-18 15:22:23 EST
> cxxtest

Misleading IMO. I've filed bug 888509 about that.

Btw, there's "cpptest" as an example of how to package something like this.


[...]


A first look at the submitted package:


> 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 ...". :-)


> Group: Development/Tools

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


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


> 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. ;-)


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



> %package devel
> Group:          Development/Tools

Same suggestion as above.

> Requires:       %{name} = %{version}-%{release}

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


> 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


> 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


> %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/*


> 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? ;)


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

> %{_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


* /usr/share/cutter/ui is empty. An oversight or future-proof?


* 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}
Comment 3 Michael Schwendt 2012-12-18 15:30:57 EST
> %clean
> rm -rf %{buildroot}

Unless you want to build this for EL-5, %clean is not necessary anymore:
https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean
Comment 4 HAYASHI Kentaro 2013-01-15 04:58:42 EST
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.
Comment 5 Michael Schwendt 2013-01-17 13:56:17 EST
Great update with many changes and a detailed %changelog entry!

A few issues remain or have been introduced with the new subpackages, however. More about that further below. Some of the issues are found in hundreds of other packages, too, despite the existance of related packaging guidelines.
Sometimes the issues are not noticed during review. In other cases, they are reintroduced with updates to a package. Not all issues are trivial to fix. Sometimes a fix is nice to have, but no more than that.

[...]

rpmlint doesn't report any important errors or warnings for the update and its builds:

  $ rpmlint *|grep -v spell|grep -v no-doc
  6 packages and 0 specfiles checked; 0 errors, 8 warnings.

[...]

So, let's examine the spec changes:

> BuildRequires: autoconf
>
> %prep
> ...
> autoconf

Rebuilding Autotools' files typically belongs into the %build section. That's the more appropriate/convenient place, because one could still run "rpmbuild -bp cutter.spec" (or even with --nodeps if the build requirements are not installed) and get only the patched/prepared source tree, e.g. when rediffing patches.

The "fedora-review" script also fails when the %prep section runs tools that are not available outside the Mock build environment.


> %package report
> Requires:       goffice08

Explicitly depending on the "goffice08" package name is not necessary and bears a higher risk of needing an unnecessary rebuild/update in the future or being wrong.

Building with goffice08 libraries results in automatically detected dependencies on the actual libraries:

  https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires

For example:

  $ rpm -qpR cutter-report-1.2.2-3.fc18.x86_64.rpm |grep off
  goffice08
  libgoffice-0.8.so.8()(64bit)

  $ repoquery --whatprovides 'libgoffice-0.8.so.8()(64bit)'
  goffice08-0:0.8.17-8.fc18.x86_64

Imagine the lib moving to package goffice08-libs.


> %package gui
> Requires:       gtk2

> %package gstreamer
> Requires:       gstreamer

Similarly for those two subpackages.


> %package gstreamer
> Summary:        Cutter GStreamer plugin

> %description gstreamer
> Cutter GStreamer plugin.

Not a blocker, but the description (and %summary) could be a bit more verbose and try to tell _what_ this package does related to GStreamer. That is not obvious even with the two lines of the description copied from the base package. As not to confuse ordinary GStreamer users, who search for "real" GStreamer plug-ins.


> +%files gui
> +%defattr(-, root, root, -)

%defattr is optional for Fedora and EL >= 5:

https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions
( https://fedoraproject.org/wiki/EPEL/GuidelinesAndPolicies#Distribution_specific_guidelines )


> +%dir %{_libdir}/cutter
> +%dir %{_libdir}/cutter/module
> +%dir %{_libdir}/cutter/module/factory
> +%dir %{_libdir}/cutter/module/factory/ui
> +%dir %{_libdir}/cutter/module/ui

Here it gets a little bit complicated, because there are multiple subpackages that store files in these directories. The guidelines say:

https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership

| Directory ownership is a little more complex than file
| ownership. Packages must own all directories they put files in,
| except for:
|
| [...]
|
| any directories owned by the filesystem, man, or other explicitly
| created -filesystem packages
| 
| any directories owned by other packages in your package's natural
| dependency chain 

For cutter this means that since the subpackages depend on the base package, it is sufficient that the base package contains the needed %dir entries. Example:

  %files report
  %{_libdir}/cutter/module/factory/report/pdf_factory.so
  %{_libdir}/cutter/module/report/pdf.so

  $ rpm -qpR cutter-report-1.2.2-3.fc18.x86_64.rpm |grep cut
  cutter(x86-64) = 1.2.2-3.fc18
  libcutter.so.0()(64bit)

If there are many (more) subpackages, it can make sense to move directory ownership into a *-filesystem package and have all subpackages depend on that one, but this would be overhead for the Cutter packages so far.


> %doc %{_datadir}/gtk-doc/html/cutter/

The file/directory ownership guidelines covers this special directory. It is okay for cutter-devel to include the two leading directories instead of depending on "gtk-doc":

  %dir %{_datadir}/gtk-doc/
  %dir %{_datadir}/gtk-doc/html/
  %doc %{_datadir}/gtk-doc/html/cutter/

Or simply this to include the dir and everything below it:

  %doc %{_datadir}/gtk-doc/

https://fedoraproject.org/wiki/Packaging:Guidelines#The_directory_is_owned_by_a_package_which_is_not_required_for_your_package_to_function

Note that several packages don't get it right at all, since directory
ownership isn't simple. 

[...]

Finally, and probably the most advanced packaging topic related to Cutter, is this in the guidelines:

  https://fedoraproject.org/wiki/Packaging:Guidelines#Filtering_Auto-Generated_Requires
  https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering

  | MUST: Packages must not provide RPM dependency information when that
  | information is not global in nature, or are otherwise handled
  | (e.g. through a virtual provides system). e.g. a plugin package
  | containing a binary shared library must not "provide" that library
  | unless it is accessible through the system library paths.

It refers to the automatic Provides for Cutter's private module libs, e.g. cutter-report and cutter-gui:

  $ rpm -qp --provides cutter-report-1.2.2-3.fc18.x86_64.rpm 
  cutter-report = 1.2.2-3.fc18
  cutter-report(x86-64) = 1.2.2-3.fc18
  pdf.so()(64bit)
  pdf_factory.so()(64bit)

  $ rpm -qp --provides cutter-gui-1.2.2-3.fc18.x86_64.rpm 
  cutter-gui = 1.2.2-3.fc18
  cutter-gui(x86-64) = 1.2.2-3.fc18
  gtk.so()(64bit)
  gtk_factory.so()(64bit)

It's not an immediate problem/blocker, and the plugin names don't start with "lib" either (fortunately!). But packagers need to be aware of this, because it can lead to issues when any other package depends on a library with the same name, and then the Cutter subpackage would be the wrong one to satisfy such a dependency.

Again, other packages don't get this right either despite the "MUST" in the guidelines. This is because it's easily overlooked, misunderstood, or not considered more than a potential problem. 

  $ repoquery --whatprovides 'pdf.so()(64bit)'
  zathura-pdf-poppler-0:0.2.1-4.fc18.x86_64
  GraphicsMagick-0:1.3.17-1.fc18.x86_64
  zathura-pdf-poppler-0:0.2.1-5.fc18.x86_64
  ImageMagick-0:6.7.7.5-3.fc18.x86_64
  GraphicsMagick-0:1.3.17-1.fc18.x86_64
  libabiword-1:2.8.6-18.fc18.x86_64

  $ repoquery --whatprovides 'gtk.so()(64bit)'
  ekg2-gtk2-0:0.3.1-5.fc18.x86_64
  rep-gtk-0:0.90.8-2.fc18.x86_64

  $ repoquery --whatrequires 'gtk.so()(64bit)'
  $

Afterall, most real system libraries are named "libsomething" and are versioned, too, so "pdf.so" and other plugin libs are unlikely to disturb real library dependencies.

There's also the following in the guidelines, which tells when the
Provides/Requires filtering macros must not be used:

  https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering#Usage

A work-around since RPM 4.9 would be this line in the spec file:
%global __provides_exclude_from ^%{_libdir}/%{name}/module/.*\\.so$

I cannot tell what the Fedora Packaging Committee says about that, however. It is not covered by the guidelines yet.
Comment 6 HAYASHI Kentaro 2013-01-30 05:03:58 EST
Thank you for reviewing!

Here is the updated(1.2.2-3 -> 1.2.2-4) 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-4.fc18.src.rpm

(In reply to comment #5)

Here is the result of rpmlint:

$ rpmlint RPMS/x86_64/* |grep -v spell|grep -v no-doc
6 packages and 0 specfiles checked; 0 errors, 8 warnings.


> 
> [...]
> 
> So, let's examine the spec changes:
> 
> > BuildRequires: autoconf
> >
> > %prep
> > ...
> > autoconf
> 
> Rebuilding Autotools' files typically belongs into the %build section.
> That's the more appropriate/convenient place, because one could still run
> "rpmbuild -bp cutter.spec" (or even with --nodeps if the build requirements
> are not installed) and get only the patched/prepared source tree, e.g. when
> rediffing patches.
> 
> The "fedora-review" script also fails when the %prep section runs tools that
> are not available outside the Mock build environment.
> 

Fixed! (fedora-review works)

> 
> > %package report
> > Requires:       goffice08
> 
> Explicitly depending on the "goffice08" package name is not necessary and
> bears a higher risk of needing an unnecessary rebuild/update in the future
> or being wrong.
> 
> Building with goffice08 libraries results in automatically detected
> dependencies on the actual libraries:
> 
>   https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires
> 
> For example:
> 
>   $ rpm -qpR cutter-report-1.2.2-3.fc18.x86_64.rpm |grep off
>   goffice08
>   libgoffice-0.8.so.8()(64bit)
> 
>   $ repoquery --whatprovides 'libgoffice-0.8.so.8()(64bit)'
>   goffice08-0:0.8.17-8.fc18.x86_64
> 
> Imagine the lib moving to package goffice08-libs.
> 
> 
> > %package gui
> > Requires:       gtk2
> 
> > %package gstreamer
> > Requires:       gstreamer
> 
> Similarly for those two subpackages.
> 

Fixed!

> 
> > %package gstreamer
> > Summary:        Cutter GStreamer plugin
> 
> > %description gstreamer
> > Cutter GStreamer plugin.
> 
> Not a blocker, but the description (and %summary) could be a bit more
> verbose and try to tell _what_ this package does related to GStreamer. That
> is not obvious even with the two lines of the description copied from the
> base package. As not to confuse ordinary GStreamer users, who search for
> "real" GStreamer plug-ins.
> 

Updated package description.

> 
> > +%files gui
> > +%defattr(-, root, root, -)
> 
> %defattr is optional for Fedora and EL >= 5:
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions
> (
> https://fedoraproject.org/wiki/EPEL/
> GuidelinesAndPolicies#Distribution_specific_guidelines )
> 
> 
> > +%dir %{_libdir}/cutter
> > +%dir %{_libdir}/cutter/module
> > +%dir %{_libdir}/cutter/module/factory
> > +%dir %{_libdir}/cutter/module/factory/ui
> > +%dir %{_libdir}/cutter/module/ui
> 
> Here it gets a little bit complicated, because there are multiple
> subpackages that store files in these directories. The guidelines say:
> 
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#File_and_Directory_Ownership
> 
> | Directory ownership is a little more complex than file
> | ownership. Packages must own all directories they put files in,
> | except for:
> |
> | [...]
> |
> | any directories owned by the filesystem, man, or other explicitly
> | created -filesystem packages
> | 
> | any directories owned by other packages in your package's natural
> | dependency chain 
> 
> For cutter this means that since the subpackages depend on the base package,
> it is sufficient that the base package contains the needed %dir entries.
> Example:
> 
>   %files report
>   %{_libdir}/cutter/module/factory/report/pdf_factory.so
>   %{_libdir}/cutter/module/report/pdf.so
> 
>   $ rpm -qpR cutter-report-1.2.2-3.fc18.x86_64.rpm |grep cut
>   cutter(x86-64) = 1.2.2-3.fc18
>   libcutter.so.0()(64bit)
> 

Fixed to aggregate %dir entries.


> If there are many (more) subpackages, it can make sense to move directory
> ownership into a *-filesystem package and have all subpackages depend on
> that one, but this would be overhead for the Cutter packages so far.
> 
> 
> > %doc %{_datadir}/gtk-doc/html/cutter/
> 
> The file/directory ownership guidelines covers this special directory. It is
> okay for cutter-devel to include the two leading directories instead of
> depending on "gtk-doc":
> 
>   %dir %{_datadir}/gtk-doc/
>   %dir %{_datadir}/gtk-doc/html/
>   %doc %{_datadir}/gtk-doc/html/cutter/
> 
> Or simply this to include the dir and everything below it:
> 
>   %doc %{_datadir}/gtk-doc/
> 
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#The_directory_is_owned_by_a_package_which_is_not_required_for_your
> _package_to_function
> 
> Note that several packages don't get it right at all, since directory
> ownership isn't simple. 
> 

Fixed %doc entry for gtk-doc.


> [...]
> 
> Finally, and probably the most advanced packaging topic related to Cutter,
> is this in the guidelines:
> 
>  
> https://fedoraproject.org/wiki/Packaging:Guidelines#Filtering_Auto-
> Generated_Requires
>   https://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering
> 
>   | MUST: Packages must not provide RPM dependency information when that
>   | information is not global in nature, or are otherwise handled
>   | (e.g. through a virtual provides system). e.g. a plugin package
>   | containing a binary shared library must not "provide" that library
>   | unless it is accessible through the system library paths.
> 
> It refers to the automatic Provides for Cutter's private module libs, e.g.
> cutter-report and cutter-gui:
> 
>   $ rpm -qp --provides cutter-report-1.2.2-3.fc18.x86_64.rpm 
>   cutter-report = 1.2.2-3.fc18
>   cutter-report(x86-64) = 1.2.2-3.fc18
>   pdf.so()(64bit)
>   pdf_factory.so()(64bit)
> 
>   $ rpm -qp --provides cutter-gui-1.2.2-3.fc18.x86_64.rpm 
>   cutter-gui = 1.2.2-3.fc18
>   cutter-gui(x86-64) = 1.2.2-3.fc18
>   gtk.so()(64bit)
>   gtk_factory.so()(64bit)
> 
> It's not an immediate problem/blocker, and the plugin names don't start with
> "lib" either (fortunately!). But packagers need to be aware of this, because
> it can lead to issues when any other package depends on a library with the
> same name, and then the Cutter subpackage would be the wrong one to satisfy
> such a dependency.
> 
> Again, other packages don't get this right either despite the "MUST" in the
> guidelines. This is because it's easily overlooked, misunderstood, or not
> considered more than a potential problem. 
> 
>   $ repoquery --whatprovides 'pdf.so()(64bit)'
>   zathura-pdf-poppler-0:0.2.1-4.fc18.x86_64
>   GraphicsMagick-0:1.3.17-1.fc18.x86_64
>   zathura-pdf-poppler-0:0.2.1-5.fc18.x86_64
>   ImageMagick-0:6.7.7.5-3.fc18.x86_64
>   GraphicsMagick-0:1.3.17-1.fc18.x86_64
>   libabiword-1:2.8.6-18.fc18.x86_64
> 
>   $ repoquery --whatprovides 'gtk.so()(64bit)'
>   ekg2-gtk2-0:0.3.1-5.fc18.x86_64
>   rep-gtk-0:0.90.8-2.fc18.x86_64
> 
>   $ repoquery --whatrequires 'gtk.so()(64bit)'
>   $
> 
> Afterall, most real system libraries are named "libsomething" and are
> versioned, too, so "pdf.so" and other plugin libs are unlikely to disturb
> real library dependencies.
> 
> There's also the following in the guidelines, which tells when the
> Provides/Requires filtering macros must not be used:
> 
>  
> https://fedoraproject.org/wiki/Packaging:
> AutoProvidesAndRequiresFiltering#Usage
> 
> A work-around since RPM 4.9 would be this line in the spec file:
> %global __provides_exclude_from ^%{_libdir}/%{name}/module/.*\\.so$
> 
> I cannot tell what the Fedora Packaging Committee says about that, however.
> It is not covered by the guidelines yet.

Fixed not to provide private module soname.
There is no private module soname such as 'pdf.so' or 'gtk.so'.

$ rpm -qp --provides RPMS/x86_64/cutter-*.rpm
cutter = 1.2.2-4.fc18
cutter(x86-64) = 1.2.2-4.fc18
libcppcutter.so.0()(64bit)
libcutter.so.0()(64bit)
libgdkcutter-pixbuf.so.0()(64bit)
libsoupcutter.so.0()(64bit)
cutter-debuginfo = 1.2.2-4.fc18
cutter-debuginfo(x86-64) = 1.2.2-4.fc18
cutter-devel = 1.2.2-4.fc18
cutter-devel(x86-64) = 1.2.2-4.fc18
pkgconfig(cppcutter) = 1.2.2
pkgconfig(cutter) = 1.2.2
pkgconfig(gcutter) = 1.2.2
pkgconfig(gdkcutter-pixbuf) = 1.2.2
pkgconfig(libcutter) = 1.2.2
pkgconfig(soupcutter) = 1.2.2
cutter-gstreamer = 1.2.2-4.fc18
cutter-gstreamer(x86-64) = 1.2.2-4.fc18
gstreamer0.10(element-cutter-console-output)()(64bit)
gstreamer0.10(element-cutter-server)()(64bit)
gstreamer0.10(element-cutter-test-runner)()(64bit)
libgstcuttertest.so()(64bit)
cutter-gui = 1.2.2-4.fc18
cutter-gui(x86-64) = 1.2.2-4.fc18
cutter-report = 1.2.2-4.fc18
cutter-report(x86-64) = 1.2.2-4.fc18
Comment 7 Michael Schwendt 2013-02-05 09:26:58 EST
Created attachment 693385 [details]
remaining spec issues

APPROVED

Here are comments on a few last issues, which need not be fixed with another src.rpm, but can be fixed when/after importing into Fedora package git:


* The "rm -fr %{buildroot}/glib-compatible/pcre" after %prep effectively is a no-op, because %buildroot doesn't exist at that time yet. And to delete files from the extracted source archive, it would need to happen _after_ %setup, not before it. That's why I moved %setup a few lines in the attached patch. However, I had to comment out the "rm -rf …" line, because one cannot simply remove the directory, because that breaks the build very early (missing the Makefile.in template) or (if only removing the source files) very late during %check:

  Making check in po
  make[1]: Entering directory `/home/personal/tmp/rpm/BUILD/cutter-1.2.2/po'
  make[1]: *** No rule to make target `../glib-compatible/pcre/pcre_chartables.c', needed by `cutter.pot'.  Stop.
  make[1]: Leaving directory `/home/personal/tmp/rpm/BUILD/cutter-1.2.2/po'
  make: *** [check-recursive] Error 1

So, with regard to the unbundling policy, it's okay if you ensure that the included PCRE is not built with in favour of the system lib.


* "%dir %{_libdir}/gstreamer-0.10" is owned by the "gstreamer" package already, which cutter-gstreamer depends on automatically via libraries.


* rpmbuild for Fedora 19 development notices a few "bogus date" warnings in the %changelog, which I've corrected with the help of "date -d …".
Comment 8 HAYASHI Kentaro 2013-02-05 11:43:30 EST

Thank you for reviewing!!
but I'm a bit confused.

 (In reply to comment #7)
> Created attachment 693385 [details]
> remaining spec issues
> 
> APPROVED
> 
> Here are comments on a few last issues, which need not be fixed with another
> src.rpm, but can be fixed when/after importing into Fedora package git:
> 

I will apply attachment 693385 [details] when importing package, but fedora-review remains set to "?" flag.

So the next thing for this review process is fixing remaining issues and update spec (1.2.2-5) and SRPM, isn't it? (not making Package SCM admin requests)
Comment 9 Michael Schwendt 2013-02-05 13:28:15 EST
Oh, I've simply toggled a wrong flag (for the attachment), because I thought it would be the same one. Should have looked more closer. :-/

fedora-review flag is '+' now.
Comment 10 HAYASHI Kentaro 2013-02-05 22:34:40 EST
(In reply to comment #9)
> Oh, I've simply toggled a wrong flag (for the attachment), because I thought
> it would be the same one. Should have looked more closer. :-/
> 
> fedora-review flag is '+' now.

Oh, I see, thanks!

New Package SCM Request
=======================
Package Name: cutter
Short Description: Unit Testing Framework for C/C++
Owners: kenhys
Branches: f18
InitialCC:
Comment 11 Jon Ciesla 2013-02-06 08:33:41 EST
Git done (by process-git-requests).
Comment 12 Fedora Update System 2013-02-09 05:07:36 EST
cutter-1.2.2-4.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/cutter-1.2.2-4.fc18
Comment 13 Fedora Update System 2013-02-09 23:39:25 EST
cutter-1.2.2-4.fc18 has been pushed to the Fedora 18 testing repository.
Comment 14 Fedora Update System 2013-02-18 20:24:12 EST
cutter-1.2.2-4.fc18 has been pushed to the Fedora 18 stable repository.

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