[Bug 1153] Review request: m2vmp2cut - MPEG2 frame accurate cutter

[RPM Fusion Bugzilla]

http://bugzilla.rpmfusion.org/show_bug.cgi?id=1153





--- Comment #5 from Göran Uddeborg <goeran at uddeborg.se>  2010-12-13 20:54:42 ---
Thanks for all the good comments!  I have updated the package now.

> 1. Not compiled with %{optflags}:

CFLAGS is already used, and there are compilation commands in C files (!).  So
this was slightly more involved than usual.  But I think I've fixed all
occurrences.

> 2. Even if transcode is optional, it is strongly recommended. Please add it
> among Requires.

Done

> 3. License is GPLv2 only, not GPLv3+.

I didn't say v3+ but v2+. :-)  But you are quite right, it is v2 only, no plus.

> 5. Package uses docs at runtime! m2vmp2cut.pl and wrapper.sh look for docs from
> /usr/libexec/m2vmp2cut-0.79-dev/doc/ and they do not find them because you do
> not install them.

I thought there was no way to invoke m2vmp2cut so it would run m2vmp2cut.pl so
it in turn would show the Examples and Usage files.  So I excluded them.  But
looking at this once more I see one can actually invoke m2vmp2cut in a way so
these help files are shown.  I install them again.

The m2vcut_help* files were apparently lost at the same time.  I guess I
haven't looked at the help for a while and never noticed.  Reviewers do come in
handy! :-)  (I.e., thanks for the catch!)

> Libexecdir is a no
> go because it should only contain executable called by main applications (that
> sits in bindir). Docdir is not good either because you can omit to install docs
> marked as %doc. The best option is to use usr/share/package_name.

I've moved them to /usr/share/m2vmp2cut, and the relative references in the
scripts are adjusted.

> 6. These macros are not used and must be removed:

Ah, some leftovers.  Removed now.

> 7. Perl packaging guidelines have been changed and the way you omit a perl
> requirement is changed.

That was something I had missed.  I've changed to use %filter_from_requires as
you suggested.  It is considerably neater, too.

> 1. Disttag is missing (this is optional but strongly suggested):

I've never thought of that as "strongly suggested".  Rather as an option you
could use if you need it.  But sure, I can add it if you want.

> 2. Why not requiring xterm instead of /usr/bin/uxterm

I don't remember why I did it, but I see no good reason.  I've changed it.

> 3. Install do not preserve timestamps.

Added -p options to cp.

> 4. Please consider adding man page/man pages.

I made one based on the output of "m2vmp2cut help .".  I've submitted it
upstreams too, to perhaps have it included in the package in the future.

> 5. I think you should use macro in %{macro} format and not in %macro format.

This is a somewhat interesting area.  One can put braces around variable/macro
names in both spec files and in shell scripts in very much the same way.  In
both cases it is always possible, but only occasionally needed.  But while many
people do so all the time in spec files, you almost never see it in shell
scripts.  Except, of course, where necessary.  It is an interesting difference
in common practice.

Personally I feel the braces clutter a bit and prefer to not have them when not
needed.  But it is certainly not a big deal.  So since you want them, I've
added them.

> 1. I have some problems in testing this package... it is not very clear to me
> how to use it.

Maybe the manual page might comes in handy? :-)

The normal work flow is
m2vmp2cut demux file.ts
m2vmp2cut select file.d
m2vmp2cut cut file.d
m2vmp2cut play file.d (and/or) m2vmp2cut move file.d

Instead of moving with m2vmp2cut, you can of course do a plain "mv" to wherever
you want the result.

You need a recording of a DVB transmission to work on.  In case you don't have
any handy, I've put the first ten minutes of an episode of something called
"Chuck" at ftp://ftp.uddeborg.se/pub/m2vmp2cut/chuck.ts

SRPM: ftp://ftp.uddeborg.se/pub/m2vmp2cut/m2vmp2cut-0.79-3.fc14.src.rpm
SPEC: ftp://ftp.uddeborg.se/pub/m2vmp2cut/m2vmp2cut.spec


-- 
Configure bugmail: http://bugzilla.rpmfusion.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.