Spec URL: http://rathann.fedorapeople.org/review/ming/ming.spec SRPM URL: http://rathann.fedorapeople.org/review/ming/ming-0.4.5-1.fc20.src.rpm Description: Ming is a library for generating Macromedia Flash files (.swf), written in C, and includes useful utilities for working with .swf files. Fedora Account System Username: rathann
Taking. Several minor issues: - Package contains a testsuite, but the spec doesn't excercise it. Feel strongly encouraged to add a %check section: %check LD_LIBRARY_PATH=$(pwd)/src/.libs make check - %defattr(-,root,root,-) is only needed to support ancient RHELs. Please remove these, unless you plan to support them. - I recommend to append --disable-silent-rules to %configure instead of using make V=1. This moves "switching on verbosity" from build-time to configure-time (It hard-codes verbosity into Makefiles). - src/ming.h (rsp. src/ming.h.in) contains "dirty code" (Read: a bug) It wraps stdio.h into extern "C" { ... } guards: #ifdef __cplusplus extern "C" { #endif ... #include <stdio.h> ... (#include <stdio.h> should be moved above the 'extern "C"') ATM, this bug seems hidden and harmless, but in general, such misplaced extern "C"s may interfere stdio.h's interaction with C++. Finally, there is a major [MUSTFIX] bug: - The *devel-package isn't multilib-capable, due to hard coded paths in /usr/bin/ming-config. One common approach to work around such issues is to modify such *-config scripts in such a way that they work as wrappers to pkg-config.
Thanks for the thorough review, Ralf. Here's an updated package. Spec URL: http://rathann.fedorapeople.org/review/ming/ming.spec SRPM URL: http://rathann.fedorapeople.org/review/ming/ming-0.4.5-2.fc20.src.rpm * Tue May 27 2014 Dominik Mierzejewski <rpm> - 0.4.5-2 - fix ming-config to be multilib-compatible - enable testsuite - disable silent rules in configure call - drop defattr - build perl, php, python and tcl bindings - don't change ChangeLog timestamp
The fact you enabled perl mandates (MUSTFIX!) further Perl-related changes to the spec: - BR: perl(Cwd), perl(strict) - Add R: perl(:MODULE_COMPAT_%...) to *-perl I'd propose the following change to your spec: # diff -u ming.spec.orig ming.spec --- ming.spec.orig 2014-06-03 07:53:23.180652587 +0200 +++ ming.spec 2014-06-03 13:46:11.970988831 +0200 @@ -43,7 +43,13 @@ %package perl Summary: A Perl module for generating Macromedia Flash files using the Ming library +Provides: perl-SWF = %{version}.%{release} +Provides: perl-SWF%{_isa} = %{version}.%{release} BuildRequires: perl(ExtUtils::MakeMaker) +BuildRequires: perl(Cwd) +BuildRequires: perl(strict) + +Requires: perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version)) %description perl A perl module for generating Macromedia Flash files using the Ming library. @@ -111,10 +117,8 @@ install -d %{buildroot}%{tcl_sitearch}/ming mv %{buildroot}%{_libdir}/ming/tcl/mingc.so %{buildroot}%{tcl_sitearch}/ming/ -%if 1 %check -LD_LIBRARY_PATH=$(pwd)/src/.libs make check -%endif +LD_LIBRARY_PATH=${RPM_BUILD_ROOT}%{_libdir} make check %post -p /sbin/ldconfig The P: perl-SWF... are optional and just "convenience" to perl-users. The LD_LIBRARY_PATH change would pickup shared libraries from RPM_BUILD_ROOT instead of the build-tree. It's an option I consider to be superior. (Sorry for having missed this possiblity in my previous comment). As I am sure you can handle these issues after git-import: APPROVED
(In reply to Ralf Corsepius from comment #3) > The fact you enabled perl mandates (MUSTFIX!) further Perl-related changes > to the spec: > - BR: perl(Cwd), perl(strict) Why? I can't find those anywhere in the guidelines. > - Add R: perl(:MODULE_COMPAT_%...) to *-perl Ack, I missed this one. > I'd propose the following change to your spec: > > # diff -u ming.spec.orig ming.spec > --- ming.spec.orig 2014-06-03 07:53:23.180652587 +0200 > +++ ming.spec 2014-06-03 13:46:11.970988831 +0200 > @@ -43,7 +43,13 @@ > > %package perl > Summary: A Perl module for generating Macromedia Flash files using the Ming > library > +Provides: perl-SWF = %{version}.%{release} > +Provides: perl-SWF%{_isa} = %{version}.%{release} Was the dot (.) here intentional? I'd think it should be a dash (-) > The P: perl-SWF... are optional and just "convenience" to perl-users. Right. A good idea to add them, though. > The LD_LIBRARY_PATH change would pickup shared libraries from RPM_BUILD_ROOT > instead of the build-tree. It's an option I consider to be superior. (Sorry > for having missed this possiblity in my previous comment). Right. > As I am sure you can handle these issues after git-import: APPROVED Thanks a lot for the review. Please mark it as ASSIGNED, too.
New Package SCM Request ======================= Package Name: ming Short Description: A library for generating Macromedia Flash files Upstream URL: http://www.libming.org/ Owners: rathann Branches: f19 f20 InitialCC:
Git done (by process-git-requests).
(In reply to Dominik 'Rathann' Mierzejewski from comment #4) > (In reply to Ralf Corsepius from comment #3) > > The fact you enabled perl mandates (MUSTFIX!) further Perl-related changes > > to the spec: > > - BR: perl(Cwd), perl(strict) > > Why? I can't find those anywhere in the guidelines. We have a rule to specify all perl-modules as BR:, a package directly depends upon while building: In this case, it's perl_ext/Makefile.PL # grep 'use ' perl_ext/Makefile.PL | sort -u use Cwd qw(abs_path cwd); use ExtUtils::MakeMaker; use strict; > > +Provides: perl-SWF = %{version}.%{release} > > +Provides: perl-SWF%{_isa} = %{version}.%{release} > > Was the dot (.) here intentional? I'd think it should be a dash (-) You are right - These should be dashes.
I know about the rule, but I couldn't find the dependencies in the code. I forgot that Makefile.PL is also perl code, so thanks for the explanation. Package imported and built.
ming-0.4.5-3.fc20 has been submitted as an update for Fedora 20. https://admin.fedoraproject.org/updates/ming-0.4.5-3.fc20
Hi all sorry for this belated comment: I'm working on packaging ming at the same time started long ago, and about the subpackage name I have a question: should -perl be named as perl-SWF?
*** Bug 232790 has been marked as a duplicate of this bug. ***
It provides perl-SWF, so yum install perl-SWF will work. The way I understand the guidelines, it should be fine.
ming-0.4.5-3.fc20 has been pushed to the Fedora 20 testing repository.
ming-0.4.5-3.fc20 has been pushed to the Fedora 20 stable repository.