Fedora Merge Review: opal http://cvs.fedora.redhat.com/viewcvs/devel/opal/ Initial Owner: veillard
Created attachment 312131 [details] initial spec file cleanup Here's an initial .spec cleanup based on the packaging guidelines and rpmlint. rpmlint is still complaining about version obsoletes but I don't think that's an issue, although opal has obsoleted openh323 for quite a while so it might be time to remove that. [perobinson@euuklonw7300b1n devel]$ rpmlint opal.spec opal.spec:14: W: unversioned-explicit-obsoletes openh323 opal.spec:26: W: unversioned-explicit-obsoletes openh323-devel 0 packages and 1 specfiles checked; 0 errors, 2 warnings.
Please note, that this library is shipped with iLBC codec, which legal status is questionable.
RH Legal is not concerned about the opal implementation of the iLBC codec. Lifting FE-Legal.
Sorry to bother you again, but, please, provide more explanations - it still not clear for some people, whether iLBC legal or not. Opal ships this implementation of RFC3951 ( http://tools.ietf.org/html/rfc3951 ), which distributes under very strange license: http://www.ilbcfreeware.org/documentation/gips_iLBClicense.pdf See also this thread: http://thread.gmane.org/gmane.linux.redhat.fedora.devel/90195 We already removed iLBC support from Asterisk - can we re-add this support back?
I'm doing the review of this package. Could you review my hydrogen-drumkits? (Bug 468765)
Here's the full review: * rpmlint complains: opal.src:27: W: unversioned-explicit-obsoletes openh323-devel Will this cause any problem in the future? I would say, let's put a version number just to be safe opal-devel.x86_64: W: no-documentation At least the license file can get into this. opal-devel.x86_64: W: obsolete-not-provided openh323-devel Is openh323 compatible with opal? If yes, you should provide it. * Remove the precompiled binaries during prep. So far I found: ./configure.exe ./samples/opalgw/messages.bin ./plugins/LID/TigerJet/TjIpSys.dll ./plugins/LID/CM_HID/CM_HID.dll ./plugins/LID/VPB/libvpb.lib ./plugins/video/H.263-ffmpeg/ffmpeg/libavcodec.dll ./plugins/video/H.263-ffmpeg/ffmpeg/libavcodec.so ./src/win32/vpbapi.lib: current ar archive Actually the ffmpeg stuff is patent encumbered. You should take that stuff off and provide a "clean" tarball for the SRPM. * Please package the docs directory. I think it makes more sense to put it in the -devel package. * Shall we package samples and plugins (possibly in different subpackages)? Note that some plugins have different licenses. * We prefer %defattr(-,root,root,-) * Please make use of the %{name} macro. * The devel package must require openssl-devel (see iax2/remote.h) * Weird provides: $ rpm -qv --provides opal ()(64bit) <--- This one g726()(64bit ... * Most libraries install into the directory %{_libdir}/%{name} , but not %{_libdir}/%{name}-%{version}. Any reason you picked the latter way? * Latest version is not packaged. opal-3.4.3 is available * Fedora specific flag -O2 is overriden at certain instances by -Os. That needs fixed.
I'm confused about this whole obsoletes/provides issue opal-devel obsoletes openh323-devel but opal does not obsolete openh323 I don't get it. An explanation in the SPEC file as a comment would be useful if you believe this is the right way.
(In reply to comment #7) > I'm confused about this whole obsoletes/provides issue > > opal-devel obsoletes openh323-devel > > but opal does not obsolete openh323 > > I don't get it. An explanation in the SPEC file as a comment would be useful if > you believe this is the right way. there's no need for them any more as the docs only require them to be kept for 2 Fedora releases, openh323 was replaced back in Fedora 6 or 7. I thought I'd already dropped them, apparently missed the -devel.
> * rpmlint complains: > opal.src:27: W: unversioned-explicit-obsoletes openh323-devel > Will this cause any problem in the future? I would say, let's put a version > number just to be safe > opal-devel.x86_64: W: no-documentation > At least the license file can get into this. > opal-devel.x86_64: W: obsolete-not-provided openh323-devel > Is openh323 compatible with opal? If yes, you should provide it. I'll remove it as its long obsolete. > * Remove the precompiled binaries during prep. So far I found: > ./configure.exe > ./samples/opalgw/messages.bin > ./plugins/LID/TigerJet/TjIpSys.dll > ./plugins/LID/CM_HID/CM_HID.dll > ./plugins/LID/VPB/libvpb.lib > ./plugins/video/H.263-ffmpeg/ffmpeg/libavcodec.dll > ./plugins/video/H.263-ffmpeg/ffmpeg/libavcodec.so > ./src/win32/vpbapi.lib: current ar archive > Actually the ffmpeg stuff is patent encumbered. You should take that stuff off > and provide a "clean" tarball for the SRPM. I'll speak to upstream to get this cleaned up. > * Please package the docs directory. I think it makes more sense to put it in > the -devel package. OK > * Shall we package samples and plugins (possibly in different subpackages)? > Note that some plugins have different licenses. The library is little use with out plugins so I don't see the point in splitting it up. > * We prefer %defattr(-,root,root,-) > > * Please make use of the %{name} macro. > > * The devel package must require openssl-devel (see iax2/remote.h) Will fix > * Weird provides: > $ rpm -qv --provides opal > ()(64bit) <--- This one > g726()(64bit > ... See RHBZ 473084 > * Most libraries install into the directory %{_libdir}/%{name} , but not > %{_libdir}/%{name}-%{version}. Any reason you picked the latter way? As per upstream. > * Latest version is not packaged. opal-3.4.3 is available Yes, but the current ekiga release depends on 3.4.2. When the new version of ekiga comes out it will be upgraded too. > * Fedora specific flag -O2 is overriden at certain instances by -Os. That needs > fixed. I'll add it to my upstream list.
(In reply to comment #9) > > * Shall we package samples and plugins (possibly in different subpackages)? > > Note that some plugins have different licenses. > > The library is little use with out plugins so I don't see the point in > splitting it up. Ok, How about the samples? Btw, currently the "MPEG4 Part 2" plugin is disabled for obvious reasons. Shall we include it in a freeworld package at rpmfusion? Is there any benefit in that?
> Ok, How about the samples? I'll have a look > Btw, currently the "MPEG4 Part 2" plugin is disabled for obvious reasons. Shall > we include it in a freeworld package at rpmfusion? Is there any benefit in > that? I think it requires x264 so its a possibility, it provides a video codec.
(In reply to comment #4) > Sorry to bother you again, but, please, provide more explanations - it still > not clear for some people, whether iLBC legal or not. > > Opal ships this implementation of RFC3951 ( http://tools.ietf.org/html/rfc3951 > ), which distributes under very strange license: > > http://www.ilbcfreeware.org/documentation/gips_iLBClicense.pdf > > See also this thread: > > http://thread.gmane.org/gmane.linux.redhat.fedora.devel/90195 > > We already removed iLBC support from Asterisk - can we re-add this support > back? Sorry for the delay. Red Hat Legal got this one wrong (it happens to everyone sometimes). The iLBC codec needs to be removed from the opal tarball. Reblocking FE-Legal.
> > We already removed iLBC support from Asterisk - can we re-add this support > > back? > > Sorry for the delay. Red Hat Legal got this one wrong (it happens to everyone > sometimes). The iLBC codec needs to be removed from the opal tarball. > > Reblocking FE-Legal. I'm reported this upstream on the OPAL bug tracker. I've also had a direct email conversation with the ekiga maintainer and he agrees so is going to discuss this with the OPAL developers so hopefully will have an upstream fix for the ilbc issue shortly.
Any word on this issue?
The upstream develops have indicated that they would accept a patch for something like 'make dist' producing a second tarball called opal-oss or something that has the ilbc code and the random binaries that are needed for the windows build removed. They don't have the time to do it themselves, and I don't really know how to do something like that with autoconf/automake. Details can be found on the SF tracker below. https://sourceforge.net/tracker/?func=detail&atid=989748&aid=2555959&group_id=204472
I've made a modified tarball that removes the ilbc bits, and committed it for F-11 and rawhide. If you push any updates for F-9 or F-10, you need to do the same thing.
Peter, It's been more than 4 months since the initial review and you made 9 builds since. Any chance you will have a look at the issues below this season? (In reply to comment #9) > > > * Remove the precompiled binaries during prep. So far I found: > > ./configure.exe > > ./samples/opalgw/messages.bin > > ./plugins/LID/TigerJet/TjIpSys.dll > > ./plugins/LID/CM_HID/CM_HID.dll > > ./plugins/LID/VPB/libvpb.lib > > ./plugins/video/H.263-ffmpeg/ffmpeg/libavcodec.dll > > ./plugins/video/H.263-ffmpeg/ffmpeg/libavcodec.so > > ./src/win32/vpbapi.lib: current ar archive > > Actually the ffmpeg stuff is patent encumbered. You should take that stuff off > > and provide a "clean" tarball for the SRPM. > > I'll speak to upstream to get this cleaned up. > (In reply to comment #11) > > Ok, How about the samples? > > I'll have a look >
(In reply to comment #17) > Peter, > It's been more than 4 months since the initial review and you made 9 builds > since. Any chance you will have a look at the issues below this season? Sorry, I've been very busy and haven't had a chance to get back to this.
I think most of the issues are now fixed.
Peter, my comment #17 needs to be addressed. Especially, the removal of precompiled binaries is important. You can use something like this in %prep for file in dll so bin lib exe; do find . -name "*.$file" -exec rm -f {} \; ; done
I don't think that in prep will be of any use as its still in the tarball unless its only an issue if we ship/use it in the final rpms (which we don't anyway as the binaries are only used by the windows builds). I don't believe any of the precompiled stuff is used by the linux build at all.
It has been a standard procedure to remove the prebuilt binaries (whether they are used or not). I saw and applied this on so many different packages that I forgot that there is no specific guideline for it except this https://fedoraproject.org/wiki/Packaging:Guidelines#No_inclusion_of_pre-built_binaries_or_libraries Since this guideline does not cover your case, I cannot claim that it is a blocker for the review. But I'd be happy if you reconsider. At the end of the day, it is your responsibility that the prebuilt binaries do not end up in the RPM's. Removing them in %prep may only make your life easier. Working on a "clean" source tree is a good thing, at least IMHO.
There's no prebuild binaries in either the opal or opal-devel package, they're used for the windows build so won't ever be used in the build process.
Okay, as I said it is not a blocker, it was just a suggestion. Anyhow, I'll ask for the 4th time (comments #6, #10, #17 and #24): > > Ok, How about the samples? > > I'll have a look >
> Anyhow, I'll ask for the 4th time (comments #6, #10, #17 and #24): AFAICS the only things I didn't directly address directly are as follows: Comment #6 > At least the license file can get into this. > opal-devel.x86_64: W: obsolete-not-provided openh323-devel Fixed in cvs as of Jan 6th > Is openh323 compatible with opal? If yes, you should provide it. No, its not compatible. > * Please package the docs directory. I think it makes more sense to put it in > the -devel package. Fixed in cvs as of Jan 6th > * Shall we package samples and plugins (possibly in different subpackages)? > Note that some plugins have different licenses. Possibly but there's never been a request for them, alot of the samples don't work well so they end up causing more issues than their contribute. Comment #10 > Ok, How about the samples? See comment above. > Btw, currently the "MPEG4 Part 2" plugin is disabled for obvious reasons. Shall > we include it in a freeworld package at rpmfusion? Is there any benefit in > that? Possibly but out of scope for this. Comment #17 Already addressed in other parts of the bug. Comment #24 Already addressed in other parts of the bug. > Anyhow, I'll ask for the 4th time (comments #6, #10, #17 and #24): AFAICS the only things I didn't directly address directly are as follows: Comment #6 > At least the license file can get into this. > opal-devel.x86_64: W: obsolete-not-provided openh323-devel Fixed in cvs as of Jan 6th > Is openh323 compatible with opal? If yes, you should provide it. No, its not compatible. > * Please package the docs directory. I think it makes more sense to put it in > the -devel package. Fixed in cvs as of Jan 6th > * Shall we package samples and plugins (possibly in different subpackages)? > Note that some plugins have different licenses. Possibly but there's never been a request for them, alot of the samples don't work well so they end up causing more issues than their contribute. Comment #10 > Ok, How about the samples? See comment above. > Btw, currently the "MPEG4 Part 2" plugin is disabled for obvious reasons. Shall > we include it in a freeworld package at rpmfusion? Is there any benefit in > that? Possibly, but out of scope for this. Comment #17 Already addressed in other parts of the bug. Comment #24 Already addressed in other parts of the bug.
Thanks for the report. As it has been a while since I did the review and the package has been updated many times since, I made a quick check about the current status: * There is a new upstream version 3.6.3 now. * rpmlint on the installed package complains: $ rpmlint opal opal.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopal.so.3.6.2 /lib64/libdl.so.2 opal.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopal.so.3.6.2 /usr/lib64/libsasl2.so.2 opal.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopal.so.3.6.2 /usr/lib64/libldap-2.4.so.2 opal.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopal.so.3.6.2 /usr/lib64/liblber-2.4.so.2 opal.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopal.so.3.6.2 /usr/lib64/libldap_r-2.4.so.2 opal.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopal.so.3.6.2 /usr/lib64/libssl.so.8 opal.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopal.so.3.6.2 /lib64/libz.so.1 opal.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopal.so.3.6.2 /lib64/libexpat.so.1 opal.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopal.so.3.6.2 /usr/lib64/libSDL-1.2.so.0 opal.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopal.so.3.6.2 /lib64/libresolv.so.2 opal.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopal.so.3.6.2 /lib64/libgcc_s.so.1 This looks like a pkgconfig issue to me. Could you give a hand? * There is a new guideline now, that says the license file (actually any doc file) should be only in one package. Having it on other subpackages is considered as file duplication. * %configure --prefix=/usr Please use the %{_prefix} macro
(In reply to comment #26) > > * %configure --prefix=/usr > Please use the %{_prefix} macro Actually, just the %configure macro should be enough
> * There is a new upstream version 3.6.3 now. I'm aware of it but I'm waiting for the new version of ekiga that will work with it. > * rpmlint on the installed package complains: > > $ rpmlint opal > opal.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopal.so.3.6.2 > /lib64/libdl.so.2 > > This looks like a pkgconfig issue to me. Could you give a hand? I don't see any of that. I just got an error on space vs tab which is now fixed. [perobinson@neo SPECS]$ rpmlint opal.spec ../RPMS/x86_64/opal* ../SRPMS/opal-3.6.2-2.fc11.src.rpm 4 packages and 1 specfiles checked; 0 errors, 0 warnings. [perobinson@neo SPECS]$ > * There is a new guideline now, that says the license file (actually any doc > file) should be only in one package. Having it on other subpackages is > considered as file duplication. Removed the extra copy of the licence in devel > * %configure --prefix=/usr > Please use the %{_prefix} macro Updated as it seems upstream have finally fixed the build without the need to specify the prefix :-) You can see the update here, I'm not going to push a new build for a few minor changes. They'll go into rawhide when I can push 3.6.3 http://cvs.fedoraproject.org/viewvc/rpms/opal/devel/opal.spec?view=markup
(In reply to comment #28) > > > * rpmlint on the installed package complains: > > > > $ rpmlint opal > > opal.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libopal.so.3.6.2 > > /lib64/libdl.so.2 > > > > This looks like a pkgconfig issue to me. Could you give a hand? > > I don't see any of that. I just got an error on space vs tab which is now > fixed. > > [perobinson@neo SPECS]$ rpmlint opal.spec ../RPMS/x86_64/opal* > ../SRPMS/opal-3.6.2-2.fc11.src.rpm > 4 packages and 1 specfiles checked; 0 errors, 0 warnings. > [perobinson@neo SPECS]$ > Yes, but you can also run rpmlint against installed packages, which may show other flaws if there are any. One of these flaws that doesn't show up when you run rpmlint against the actual rpm files is the unused-direct-shlib-dependency warning. Just simply run $ rpmlint opal and you will see > > * %configure --prefix=/usr > > Please use the %{_prefix} macro > > Updated as it seems upstream have finally fixed the build without the need to > specify the prefix :-) > Nice. Btw, %configure should define the prefix for you among other things. You can run $ rpm -E %configure to see what else it defines. > You can see the update here, I'm not going to push a new build for a few minor > changes. They'll go into rawhide when I can push 3.6.3 > http://cvs.fedoraproject.org/viewvc/rpms/opal/devel/opal.spec?view=markup All good. I'll go over the package one last time and let you know if there's anything else.
> Yes, but you can also run rpmlint against installed packages, which may show > other flaws if there are any. One of these flaws that doesn't show up when you > run rpmlint against the actual rpm files is the unused-direct-shlib-dependency > warning. > > Just simply run > $ rpmlint opal > and you will see And you get those errors for glib2 and others as well. I don't see it as a blocker. > All good. I'll go over the package one last time and let you know if there's > anything else. Thanks
rpmlints are usually blockers. If not, the packager should explain/defend why each rpmlint can be disregarded. In this case, the fix should be straightforward. "Some X package gives those errors too" is not a proper way of defending this. Please file bugs to those packages too. They need to be fixed.
Looking at this ancient bug, I don't think there are any remaining Legal issues, so I'm lifting FE-Legal. I would encourage you to try to revisit this bug for any remaining issues, identify them, or close out this ticket.
(In reply to comment #31) > rpmlints are usually blockers. If not, the packager should explain/defend why > each rpmlint can be disregarded. In this case, the fix should be > straightforward. This is the only outstanding issue on the bug. I've tried the fedora recommended fixes for the unused-direct-shlib-dependency but it breaks the build due to the esoteric build system of opal. Upstream have no interested in fixing them either. I personally don't see that it should be blocking the approval of this and I can't see any other blockers.
(In reply to comment #33) > (In reply to comment #31) > > rpmlints are usually blockers. If not, the packager should explain/defend why > > each rpmlint can be disregarded. In this case, the fix should be > > straightforward. > > This is the only outstanding issue on the bug. I've tried the fedora > recommended fixes for the unused-direct-shlib-dependency but it breaks the > build due to the esoteric build system of opal. Upstream have no interested in > fixing them either. I personally don't see that it should be blocking the > approval of this and I can't see any other blockers. Well, there is also the "pre-built libraries need to be removed in %prep issue". When I brought this up months ago, it was not strictly required. But now we have a guideline which makes this a MUST: http://fedoraproject.org/wiki/Packaging/Guidelines#No_inclusion_of_pre-built_binaries_or_libraries
s/libraries/binaries
> Well, there is also the "pre-built libraries need to be removed in %prep > issue". When I brought this up months ago, it was not strictly required. But > now we have a guideline which makes this a MUST: > > http://fedoraproject.org/wiki/Packaging/Guidelines#No_inclusion_of_pre-built_binaries_or_libraries Well it mentions the removal in prep. None are shipped in the binary rpms so i don't see what difference it makes. Its still the same outcome but I am beyond caring so I will update the spec.
Updated and built in rawhide. Diff is http://cvs.fedoraproject.org/viewvc/rpms/opal/devel/opal.spec?r1=1.56&r2=1.57
Fair enough. Thanks. -------------------------------------------- This Merge Review (opal) is APPROVED by oget -------------------------------------------- Closing the bug