Bug 1101043 - Review Request: ming - A library for generating Macromedia Flash files
Summary: Review Request: ming - A library for generating Macromedia Flash files
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ralf Corsepius
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 232790 (view as bug list)
Depends On:
Blocks: 455553
TreeView+ depends on / blocked
 
Reported: 2014-05-25 23:18 UTC by Dominik 'Rathann' Mierzejewski
Modified: 2014-07-04 12:29 UTC (History)
4 users (show)

Fixed In Version: ming-0.4.5-3.fc20
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-07-04 12:29:22 UTC
Type: ---
Embargoed:
rc040203: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Dominik 'Rathann' Mierzejewski 2014-05-25 23:18:47 UTC
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

Comment 1 Ralf Corsepius 2014-05-26 05:35:01 UTC
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.

Comment 2 Dominik 'Rathann' Mierzejewski 2014-06-02 21:54:23 UTC
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

Comment 3 Ralf Corsepius 2014-06-03 12:16:36 UTC
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

Comment 4 Dominik 'Rathann' Mierzejewski 2014-06-05 13:30:21 UTC
(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.

Comment 5 Dominik 'Rathann' Mierzejewski 2014-06-05 16:07:46 UTC
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:

Comment 6 Gwyn Ciesla 2014-06-05 17:31:36 UTC
Git done (by process-git-requests).

Comment 7 Ralf Corsepius 2014-06-06 02:29:22 UTC
(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.

Comment 8 Dominik 'Rathann' Mierzejewski 2014-06-06 20:59:39 UTC
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.

Comment 9 Fedora Update System 2014-06-06 22:07:42 UTC
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

Comment 10 Christopher Meng 2014-06-09 07:23:58 UTC
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?

Comment 11 Christopher Meng 2014-06-09 07:24:10 UTC
*** Bug 232790 has been marked as a duplicate of this bug. ***

Comment 12 Dominik 'Rathann' Mierzejewski 2014-06-09 09:12:48 UTC
It provides perl-SWF, so yum install perl-SWF will work. The way I understand the guidelines, it should be fine.

Comment 13 Fedora Update System 2014-06-10 03:08:28 UTC
ming-0.4.5-3.fc20 has been pushed to the Fedora 20 testing repository.

Comment 14 Fedora Update System 2014-07-04 12:29:22 UTC
ming-0.4.5-3.fc20 has been pushed to the Fedora 20 stable repository.


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