Bug 477750 - Review Request: Ogmtools - Tools for Ogg media streams
Review Request: Ogmtools - Tools for Ogg media streams
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Matthias Saou
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks: RussianFedoraRemix DebugInfo
  Show dependency treegraph
 
Reported: 2008-12-23 05:26 EST by Gianluca Sforna
Modified: 2009-05-09 00:18 EDT (History)
6 users (show)

See Also:
Fixed In Version: 1.5-6.fc11
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-05-09 00:18:58 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
matthias: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)
optflags patch (729 bytes, patch)
2009-04-19 19:27 EDT, Gianluca Sforna
no flags Details | Diff

  None (edit)
Description Gianluca Sforna 2008-12-23 05:26:44 EST
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 05:54:29 EST
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 13:54:05 EST
I'll review this one :-)
Comment 3 Matthias Saou 2009-01-02 14:10:11 EST
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 04:44:32 EST
ok, do you want an updated spec file before the official review?
Comment 5 Gianluca Sforna 2009-02-19 17:36:23 EST
ping...
Comment 6 Paulo Roma Cavalcanti 2009-04-09 10:09:43 EDT
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 13:35:33 EDT
* /!\ 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 17:27:56 EDT
(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 12:15:16 EDT
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 10:28:59 EDT
ah... in that case, "GPLv2+" wins
Comment 11 Gianluca Sforna 2009-04-14 16:36:11 EDT
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 04:04:59 EDT
Perfect, then the package is APPROVED.
Comment 13 Gianluca Sforna 2009-04-15 04:43:58 EDT
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 00:23:27 EDT
cvs done.
Comment 15 Gianluca Sforna 2009-04-16 03:55:16 EDT
Package imported and built. Thank you very much
Comment 16 Ville Skyttä 2009-04-19 16:04:19 EDT
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 17:30:33 EDT
Thanks for the notice. I assume the two suggestions are unrelated?
Comment 18 Ville Skyttä 2009-04-19 17:43:45 EDT
(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 19:27:41 EDT
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 12:37:59 EDT
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 18:38:15 EDT
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 10:45:15 EDT
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 16:09:03 EDT
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 17:28:08 EDT
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 00:18:53 EDT
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.

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