Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=887778 --- Comment #4 from HAYASHI Kentaro <kenhys@xxxxxxxxx> --- 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 _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review