|Summary:||Review Request: Ogmtools - Tools for Ogg media streams|
|Product:||[Fedora] Fedora||Reporter:||Gianluca Sforna <giallu>|
|Component:||Package Review||Assignee:||Matthias Saou <matthias>|
|Status:||CLOSED NEXTRELEASE||QA Contact:||Fedora Extras Quality Assurance <extras-qa>|
|Version:||rawhide||CC:||bnocera, fedora-package-review, matthias, notting, promac, ville.skytta|
|Fixed In Version:||1.5-6.fc11||Doc Type:||Bug Fix|
|Doc Text:||Story Points:||---|
|Last Closed:||2009-05-09 04:18:58 UTC||Type:||---|
|oVirt Team:||---||RHEL 7.3 requirements from Atomic Host:|
|Bug Depends On:|
|Bug Blocks:||496968, 496433|
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
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
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.