ola: Add patch to fix linking issue
PATCH 1/1 ola: Add patch to fix linking issue
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.
--- ...link-ola-network-TCPSocket-ReadDescriptor.patch | 28 ++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 package/ola/Fix-link-ola-network-TCPSocket-ReadDescriptor.patch
package/ola/Fix-link-ola-network-TCPSocket-ReadDescriptor.patch new file +28 -0
@@ -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(); +
Signature · 1 line
2.4.5
PATCH 1/1 ola: Add patch to fix linking issue
Quoted 8 lines · 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::ReadDescriptoris 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.
PATCH 1/1 ola: Add patch to fix linking issue
Quoted 3 lines · Nicolas Cavallari
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.
Quoted 2 lines
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?
PATCH 1/1 ola: Add patch to fix linking issue
In reply to Simon MarchiDear Simon Marchi,
Quoted 12 lines · 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::ReadDescriptoris 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.
Quoted 1 line
Signed-off-by Simon Marchi <[email protected]>
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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH 1/1 ola: Add patch to fix linking issue
Quoted 2 lines · Thomas Petazzoni
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.
PATCH 1/1 ola: Add patch to fix linking issue
Dear Simon Marchi,
Quoted 9 lines · Simon Marchi
Quoted 2 lines · Thomas Petazzoni
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 ?
Quoted 4 lines
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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH 1/1 ola: Add patch to fix linking issue
Quoted 8 lines · Thomas Petazzoni
Dear Simon Marchi,
Quoted 5 lines · Simon Marchi
Quoted 2 lines · Thomas Petazzoni
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?
Quoted 6 lines
Quoted 4 lines
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).
Quoted 7 lines
Quoted 4 lines
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
PATCH 1/1 ola: Add patch to fix linking issue
Dear Simon Marchi,
Quoted 8 lines · Simon Marchi
Quoted 6 lines
Quoted 4 lines
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.
Quoted 3 lines
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.
Quoted 7 lines
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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com