Bug 477750

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

I'll review this one :-)

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).

ok, do you want an updated spec file before the official review?

ping...

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.

* /!\ 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)

(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

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 :-/

ah... in that case, "GPLv2+" wins

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 :)

Perfect, then the package is APPROVED.

New Package CVS Request
=======================
Package Name: ogmtools
Short Description: Tools for Ogg media streams
Owners: giallu
Branches: F-10 F-11
InitialCC:

cvs done.

Package imported and built. Thank you very much

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.

Thanks for the notice. I assume the two suggestions are unrelated?

(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).

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?

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).

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...

ogmtools-1.5-6.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/ogmtools-1.5-6.fc11

ogmtools-1.5-6.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/ogmtools-1.5-6.fc10

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.

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.