Bug 477750

Summary: Review Request: Ogmtools - Tools for Ogg media streams
Product: [Fedora] Fedora Reporter: Gianluca Sforna <giallu>
Component: Package ReviewAssignee: Matthias Saou <matthias>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: bnocera, fedora-package-review, matthias, notting, promac, ville.skytta
Target Milestone: ---Keywords: Reopened
Target Release: ---Flags: matthias: fedora-review+
kevin: fedora-cvs+
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 1.5-6.fc11 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-05-09 04:18:58 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 496968, 496433    
Attachments:
Description Flags
optflags patch none

Description Gianluca Sforna 2008-12-23 10:26:44 UTC
Spec URL: http://giallu.fedorapeople.org/ogmtools.spec
SRPM URL: http://giallu.fedorapeople.org/ogmtools-1.5-4.fc10.src.rpm
Description: These tools allow information about (ogminfo) or extraction from (ogmdemux) or creation of (ogmmerge) OGG media streams. Note that OGM is used for "OGG media streams".

Comment 1 Gianluca Sforna 2008-12-23 10:54:29 UTC
Please note this used to be out of Fedora due to its libdvdread dependency.

Spec tool adapted from freshrpms one; rpmlint is silent, apart from:

ogmtools.x86_64: W: file-not-utf8 /usr/share/doc/ogmtools-1.5/ChangeLog

Comment 2 Matthias Saou 2009-01-02 18:54:05 UTC
I'll review this one :-)

Comment 3 Matthias Saou 2009-01-02 19:10:11 UTC
Initial comment about the licensing (formal review pending) : The Licensing wiki page states that "A GPL or LGPL licensed package that lacks any statement of what version that it's licensed under in the source code/program output/accompanying docs is technically licensed under *any* version of the GPL or LGPL, not just the version in whatever COPYING file they include.", so since here the sources contain only "Distributed under the GPL", the License: field should be "GPL+" and not "GPLv2".

Note that lots of files state "Based on Xiph.org's 'oggmerge' found in their CVS repository", but that doesn't seem to be a problem, as it has exactly the same plain "Distributed under the GPL" statement. See :
https://trac.xiph.org/browser/trunk/ogg-tools/oggmerge/oggmerge.c

All of the rest does look fine. Just that ChangeLog which could be converted to UTF-8, though I wouldn't consider that a blocker (I would if it was the main README for instance).

Comment 4 Gianluca Sforna 2009-01-06 09:44:32 UTC
ok, do you want an updated spec file before the official review?

Comment 5 Gianluca Sforna 2009-02-19 22:36:23 UTC
ping...

Comment 6 Paulo Roma Cavalcanti 2009-04-09 14:09:43 UTC
Just add something like this, to convert ChangeLog to utf8:

%prep
%setup -q
iconv -f iso8859-1 -t utf8 ChangeLog -o ChangeLog.txt
touch -r ChangeLog ChangeLog.txt
mv ChangeLog.txt ChangeLog

Also, in the changelog of your spec file, I would write only:

* Thu Dec 16 2008 Gianluca Sforna <giallu gmail com> - 1.5-4
- Adapted spec file based on freshrpms latest version.

Comment 7 Matthias Saou 2009-04-09 17:35:33 UTC
* /!\ rpmlint output :
  ogmtools.x86_64: W: file-not-utf8 /usr/share/doc/ogmtools-1.5/ChangeLog
  That will need fixing, like Paulo suggested for instance.
* OK : naming is ok, spec is named ok, confirms to packaging guielines
* /!\ license is GPL+ from what I dug up, please confirm then fix in the License
* OK : sources are pristine
* OK : builds and runs fine (tested on x86_64)
* OK : build requires are ok, though libogg-devel could be excluded since
  libvorbis-devel requires it, but since the package explicitly requires libogg
  to build this is fine
* OK : no libs, not relocatable, no unusual parent dirs
* OK : %files, %clean, %defattr, consistency in macros
* OK : no devel type files, no large docs
* OK : no GUI, no .desktop file

Cosmetic : The header lines aren't aligned with the BuildRequires ones.

Please fix the ChangeLog file, the "License:" field and align the headers in the spec if you don't mind. Once that done, I'll approve the package.

(and sorry for having stalled the review for so long, I really had hope to be able to get it over with much more quickly)

Comment 8 Gianluca Sforna 2009-04-09 21:27:56 UTC
(In reply to comment #7)
> * /!\ rpmlint output :
>   ogmtools.x86_64: W: file-not-utf8 /usr/share/doc/ogmtools-1.5/ChangeLog
>   That will need fixing, like Paulo suggested for instance.
Fixed

> * /!\ license is GPL+ from what I dug up, please confirm then fix in the
> License

Confirmed and fixed

> 
> Cosmetic : The header lines aren't aligned with the BuildRequires ones.
Fixed


> (and sorry for having stalled the review for so long, I really had hope to be
> able to get it over with much more quickly)  
np

Spec file updated if you want to have a look

Comment 9 Matthias Saou 2009-04-10 16:15:16 UTC
Everything looks good now, but I've just double checked the source just in case, and looking closer at the "avilib" directory, there is code taken from transcode, which is "either version 2, or (at your option) any later version.".

I'm no legal expert, but I'm assuming that the "License": field should then be either "GPL+ and GPLv2+" or (maybe simplified to) "GPLv2+".

This license question needs to be sorted out :-/

Comment 10 Gianluca Sforna 2009-04-14 14:28:59 UTC
ah... in that case, "GPLv2+" wins

Comment 11 Gianluca Sforna 2009-04-14 20:36:11 UTC
And I asked spot on IRC:

giallu: spot, is GPLv2+ right for a package with some sources GPL+ and others GPLv2+?
spot: giallu: if they compile together, yes.
giallu: ok thanks :)

Comment 12 Matthias Saou 2009-04-15 08:04:59 UTC
Perfect, then the package is APPROVED.

Comment 13 Gianluca Sforna 2009-04-15 08:43:58 UTC
New Package CVS Request
=======================
Package Name: ogmtools
Short Description: Tools for Ogg media streams
Owners: giallu
Branches: F-10 F-11
InitialCC:

Comment 14 Kevin Fenzi 2009-04-16 04:23:27 UTC
cvs done.

Comment 15 Gianluca Sforna 2009-04-16 07:55:16 UTC
Package imported and built. Thank you very much

Comment 16 Ville Skyttä 2009-04-19 20:04:19 UTC
Build logs show $RPM_OPT_FLAGS are not used at all.

%configure --disable-dependency-tracking could speed up the build a bit and make problems like the above easier to spot.

Comment 17 Gianluca Sforna 2009-04-19 21:30:33 UTC
Thanks for the notice. I assume the two suggestions are unrelated?

Comment 18 Ville Skyttä 2009-04-19 21:43:45 UTC
(In reply to comment #17)
> I assume the two suggestions are unrelated?

Yes and no.  --disable-dependency-tracking doesn't do anything to any compiler flags, but as said in comment 16, it might make such problems easier to spot for people who read build logs (and might also speed up builds slightly, but that's unrelated).

Comment 19 Gianluca Sforna 2009-04-19 23:27:41 UTC
Created attachment 340264 [details]
optflags patch

The build looks good with the attached patch. can you please double check it is the correct thing to do?

Comment 20 Ville Skyttä 2009-04-20 16:37:59 UTC
I didn't test the patch but it looks like something that would work, although I'd personally use $CFLAGS and $CXXFLAGS instead of $RPM_OPT_FLAGS (the %configure macro sets them), and try to push the change upstream (obviously the patched file would be configure.in instead of configure in that case), noting to them that their configure doesn't quite behave how it says and how people expect (CFLAGS and CXXFLAGS are documented as "influential environment variables" in configure --help output).

Comment 21 Gianluca Sforna 2009-04-21 22:38:15 UTC
Ok, I changed the patch per your suggestion and I am rebuilding the package.

I will try to chase upstream, but being last release in 2004 I doubt there will be any reaction...

Comment 22 Fedora Update System 2009-04-22 14:45:15 UTC
ogmtools-1.5-6.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/ogmtools-1.5-6.fc11

Comment 23 Fedora Update System 2009-04-22 20:09:03 UTC
ogmtools-1.5-6.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/ogmtools-1.5-6.fc10

Comment 24 Fedora Update System 2009-04-27 21:28:08 UTC
ogmtools-1.5-6.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 25 Fedora Update System 2009-05-09 04:18:53 UTC
ogmtools-1.5-6.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.