[Buildroot] [PATCH] ola: Add patch to fix linking issue

[Buildroot] [PATCH] ola: Add patch to fix linking issue

[Simon Marchi]

This patch fixes this autobuild failure:

  http://autobuild.buildroot.net/results/e14/e14e1700d4fe359c56be57587bdb509e002e5753/build-end.log

I am not 100% sure of what happens (I am not a C++ expert at all), but
here it is anyway.  I assume that when the definition of
TCPSocket::ReadDescriptor is embedded in the class definition, no actual
implementation is generated (all usages of it are inline).  When object
libolaopenpixelcontrol.so tries to reference it somehow, it results in
an undefined reference.  Moving the definition in the cpp retains it. If
you have a clear explanation of what happens, I'd be happy to read it.

Signed-off-by: Simon Marchi <simon.marchi@polymtl.ca>
---
 ...link-ola-network-TCPSocket-ReadDescriptor.patch | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 package/ola/Fix-link-ola-network-TCPSocket-ReadDescriptor.patch

diff --git a/package/ola/Fix-link-ola-network-TCPSocket-ReadDescriptor.patch b/package/ola/Fix-link-ola-network-TCPSocket-ReadDescriptor.patch
new file mode 100644
index 0000000..8c9385b
--- /dev/null
+++ b/package/ola/Fix-link-ola-network-TCPSocket-ReadDescriptor.patch
@@ -0,0 +1,28 @@
+Index: ola-0.9.6/common/network/TCPSocket.cpp
+===================================================================
+--- ola-0.9.6.orig/common/network/TCPSocket.cpp
++++ ola-0.9.6/common/network/TCPSocket.cpp
+@@ -163,6 +163,10 @@ TCPSocket* TCPSocket::Connect(const Sock
+   return socket;
+ }
+ 
++ola::io::DescriptorHandle TCPSocket::ReadDescriptor() const
++{
++  return m_handle;
++}
+ 
+ // TCPAcceptingSocket
+ // ------------------------------------------------
+Index: ola-0.9.6/include/ola/network/TCPSocket.h
+===================================================================
+--- ola-0.9.6.orig/include/ola/network/TCPSocket.h
++++ ola-0.9.6/include/ola/network/TCPSocket.h
+@@ -47,7 +47,7 @@ class TCPSocket: public ola::io::Connect
+ 
+   ~TCPSocket() { Close(); }
+ 
+-  ola::io::DescriptorHandle ReadDescriptor() const { return m_handle; }
++  ola::io::DescriptorHandle ReadDescriptor() const;
+   ola::io::DescriptorHandle WriteDescriptor() const { return m_handle; }
+   bool Close();
+ 
-- 
2.4.5

[Buildroot] [PATCH] ola: Add patch to fix linking issue

[Nicolas Cavallari]

On 05/08/2015 06:19, Simon Marchi wrote:
> This patch fixes this autobuild failure:
> 
>   http://autobuild.buildroot.net/results/e14/e14e1700d4fe359c56be57587bdb509e002e5753/build-end.log
> 
> I am not 100% sure of what happens (I am not a C++ expert at all), but
> here it is anyway.  I assume that when the definition of
> TCPSocket::ReadDescriptor is embedded in the class definition, no actual
> implementation is generated (all usages of it are inline).

No, the actual implementation is still generated.  However, the build
system pass the -fvisibility-inlines-hidden option, so it is not
exported ...

This bug should be present in the whole library, i'm surprised that
only break in this case.

[Buildroot] [PATCH] ola: Add patch to fix linking issue

[Simon Marchi]

On 5 August 2015 at 03:58, Nicolas Cavallari
<Nicolas.Cavallari@green-communications.fr> wrote:
> No, the actual implementation is still generated.  However, the build
> system pass the -fvisibility-inlines-hidden option, so it is not
> exported ...

Ahh thanks for the tip.  I should have looked at the compiler flags in
the first place, this one would have been obvious.

> This bug should be present in the whole library, i'm surprised that
> only break in this case.

My guess is it's the only method that has this problem that is called
from a shared library.

In any case, do you think that making the implementation non-inline is
the right fix here?

[Buildroot] [PATCH] ola: Add patch to fix linking issue

[Thomas Petazzoni]

Dear Simon Marchi,

On Wed,  5 Aug 2015 00:19:40 -0400, Simon Marchi wrote:
> This patch fixes this autobuild failure:
> 
>   http://autobuild.buildroot.net/results/e14/e14e1700d4fe359c56be57587bdb509e002e5753/build-end.log
> 
> I am not 100% sure of what happens (I am not a C++ expert at all), but
> here it is anyway.  I assume that when the definition of
> TCPSocket::ReadDescriptor is embedded in the class definition, no actual
> implementation is generated (all usages of it are inline).  When object
> libolaopenpixelcontrol.so tries to reference it somehow, it results in
> an undefined reference.  Moving the definition in the cpp retains it. If
> you have a clear explanation of what happens, I'd be happy to read it.
> 
> Signed-off-by: Simon Marchi <simon.marchi@polymtl.ca>

Could you report the problem to the upstream ola project and see what
they say? Also, they have a 0.9.7 release, maybe you want to try it?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] ola: Add patch to fix linking issue

[Simon Marchi]

On 8 August 2015 at 06:44, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Could you report the problem to the upstream ola project and see what
> they say? Also, they have a 0.9.7 release, maybe you want to try it?

I discussed it a bit on their IRC channel, it seems like the right fix
will be to move the implementations to the .cpp files.

I filed a bug: https://github.com/OpenLightingProject/ola/issues/880

The 0.9.7 release does not change anything about that problem, but we
could still bump the version. I am preparing a patch to cleanup the
plugin management of the package, if it hasn't been bump by the time I
send my patch, I'll do it then.

[Buildroot] [PATCH] ola: Add patch to fix linking issue

[Thomas Petazzoni]

Dear Simon Marchi,

On Sun, 9 Aug 2015 23:11:03 -0400, Simon Marchi wrote:
> On 8 August 2015 at 06:44, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> > Could you report the problem to the upstream ola project and see what
> > they say? Also, they have a 0.9.7 release, maybe you want to try it?
> 
> I discussed it a bit on their IRC channel, it seems like the right fix
> will be to move the implementations to the .cpp files.
> 
> I filed a bug: https://github.com/OpenLightingProject/ola/issues/880

Shouldn't we simply remove -fvisibility-inlines-hidden ?

> The 0.9.7 release does not change anything about that problem, but we
> could still bump the version. I am preparing a patch to cleanup the
> plugin management of the package, if it hasn't been bump by the time I
> send my patch, I'll do it then.

Sure, but that's going to be material for after 2015.08. What I was
looking for right now for 2015.08 is a fix for the build issue.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH] ola: Add patch to fix linking issue

[Simon Marchi]

On 10 August 2015 at 08:44, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Simon Marchi,
>
> On Sun, 9 Aug 2015 23:11:03 -0400, Simon Marchi wrote:
>> On 8 August 2015 at 06:44, Thomas Petazzoni
>> <thomas.petazzoni@free-electrons.com> wrote:
>> > Could you report the problem to the upstream ola project and see what
>> > they say? Also, they have a 0.9.7 release, maybe you want to try it?
>>
>> I discussed it a bit on their IRC channel, it seems like the right fix
>> will be to move the implementations to the .cpp files.
>>
>> I filed a bug: https://github.com/OpenLightingProject/ola/issues/880
>
> Shouldn't we simply remove -fvisibility-inlines-hidden ?

Do you mean remove it as a patch in the buildroot package, or upstream?

I asked in the bug report if removing the switch upstream would be a
possibility, how much impact it has on runtime performance (startup
time).

>> The 0.9.7 release does not change anything about that problem, but we
>> could still bump the version. I am preparing a patch to cleanup the
>> plugin management of the package, if it hasn't been bump by the time I
>> send my patch, I'll do it then.
>
> Sure, but that's going to be material for after 2015.08. What I was
> looking for right now for 2015.08 is a fix for the build issue.

As an immediate fix, the patch I sent works. Although it only moves
the problematic method. It would be possible for another method to
break with another combination of compiler/platform.

Removing the switch for the buildroot package would probably work as
well (I am testing right now) and would prevent other similar breaks.
Would you like me to send a patch immediately that does that instead?

Thanks,

Simon

[Buildroot] [PATCH] ola: Add patch to fix linking issue

[Thomas Petazzoni]

Dear Simon Marchi,

On Mon, 10 Aug 2015 13:45:31 -0400, Simon Marchi wrote:

> >> I discussed it a bit on their IRC channel, it seems like the right fix
> >> will be to move the implementations to the .cpp files.
> >>
> >> I filed a bug: https://github.com/OpenLightingProject/ola/issues/880
> >
> > Shouldn't we simply remove -fvisibility-inlines-hidden ?
> 
> Do you mean remove it as a patch in the buildroot package, or upstream?

I was thinking in the Buildroot package for now, while the discussion
with upstream is on-going.

> I asked in the bug report if removing the switch upstream would be a
> possibility, how much impact it has on runtime performance (startup
> time).

Yep, seen that, that's good.

> As an immediate fix, the patch I sent works. Although it only moves
> the problematic method. It would be possible for another method to
> break with another combination of compiler/platform.
> 
> Removing the switch for the buildroot package would probably work as
> well (I am testing right now) and would prevent other similar breaks.
> Would you like me to send a patch immediately that does that instead?

To be honest, I really don't know what is the best between changing the
method implementation, or removing the switch. Just let me know which
you think is the best workaround for now, until upstream solves the
problem.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com