-
Notifications
You must be signed in to change notification settings - Fork 280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ld-2.24.90 does not support -l:/full/path/to/library.so #694
Comments
Using This was working well until now. What system are you experiencing this? Using |
On Thursday, at 10:13, Dirk Thomas wrote: You mean -l:/path/to/lib? (-l/path/to/lib has never worked). AFAIK, pkg-config does not reorder libraries (this would be a bug). But yes, I | This was working well until now. What system are you experiencing this? ld-2.24.90, from ubuntu-14.10 % ld --version |
Yes, It sadly does reorder libraries - please see more info in #300. If that effects Utopic it will be "problematic" to build any dry packages which depend on pkg-config. |
On Thursday, at 11:13, Dirk Thomas wrote: I do not see any reordering issue in #300 ? Maybe what you mean is that pkg-config does not properly handle items that are | If that effects Utopic it will be "problematic" to build any dry packages which Yes, hence this PR :) I think all this comes from a more general issue that is a misunderstanding of The Libs: is not about listing a library's dependencies. Libs: is just meant to The only scenario where Libs: should contain something more is when the foo Also, there is the Libs.private: which is meant for static linking, because So, if the goal is to find the most simple fix, then adding just a "-L/" in |
On Thursday, at 21:05, Anthony Mallet wrote: More on this ... because I would really be surprised that pkg-config reorders % cat foo.pc Libs: -L/foo /usr/lib/libfoo.so -lbar /usr/lib/libfoo.so -lbar % env PKG_CONFIG_PATH=. pkg-config --libs foo This looks correct to me. |
See also the issue reported to the binutils people here If somebody wants to help convincing them ... :) |
Wow... |
Reviving an old thread, but I'm facing the same issue on Arch with binutils 2.25.0. One way to solve the issue is to only add -l:library.so instead of -l:/full/path/to/library.so to the generated pkg-config files. I haven't tested whether this works on the supported Ubuntu platforms, but for someone looking for a quick solution, here is the hack:
|
Just using the library name without the absolute path won't work since a) it does not set the library dirs anywhere and b) even if it would prepend a |
Well, according to the ld man page, it says
so it should only search for the file in the library path. And as per https://sourceware.org/bugzilla/show_bug.cgi?id=17532#c3, the use of full path in the argument is |
Using numerous Especially when intentionally overlaying workspaces (http://www.ros.org/reps/rep-0128.html#overlays). E.g. when a workspace B overlays workspace A that means that any shared library from workspace B should be preferred over any from workspace A. if every package simply exports library dirs and library names the order in which you find and use them results in a non-deterministic order of I could not imagine a workaround for the ROS buildsystem which does not involve a significant refactoring of all ROS packages if the linker would suddenly not support absolute paths anymore. Also we are building ROS Jade packages for Ubuntu Vivid for several months now and I have not seen a single failure related to this. So to me it seems that its ld version still supports that use case. |
@dirk-thomas It only occurs when building rosbuild packages, catkin packages build fine so you might not have noticed it. I just tried building a rosbuild package, with a trivial executable as the target, depending on roscpp (with jade on Ubuntu 15.04) and it gave me errors in the linking stage
|
I didn't notice that you referred to
Removing the colon is therefore not an option (at least that was the behavior at that time). And dropping the path and using only the filename will result in the ordering problem described. |
Sadly then seems like rosbuild package building would be broken from Utopic onwards... |
I am open to suggestions if you can imagine a way to address the problem. |
It could sound a bit stupid but: what about adding the absolute path to the .pc file without the
We can discuss (and probably lost the discussion) if we are not breaking the semantic of the Libs: clause of pkg-config, which specifies:
I've found that Ogre and OpenCV are doing this kind of tricks in OGRE-PCZ.pc (together with rpath). In the case of OpenCV I think that the .pc file kind of buggy: |
I was able to create a catkin_workspace with catkin modified with this patch and ros_comm. All seems to be working. Absolute paths are passing to the compiler without the
I was not able to find any singe rosbuild package. @kartikmohta could please point me to your rosbuild package or test the patch by yourself? |
I think we first should reproduce the original problem (the reordering) which was the reason why this was changed in the first place (#300). |
I'm looking into this, too. As @tho- asked, can somebody provide a |
Removing the |
On Monday, at 14:55, gerkey wrote: To strictly comply with pkg-config, I think (to be checked again) that Libs: I think that the reordering issues come from libtool, which is too ... clever [I know that libtool is not used by ros packages, but it happens that I do use The reordering, when using ony -L/-l and no absolute paths, is only an issue if % ls A/ i.e trying to pick-up foo from A and bar from B where both A and B both contain My personal opinion on this is that trying to solve this is bound to fail, no For instance, if it happens that foo depends on bar, then the libfoo.so So, I would personnaly say that the reordering issue is ... not an issue :) |
Well, to be fair, it didn't fail when
That's great if it works for you, but for most of our users who don't have an intimate knowledge of linkers, it still leaves them with a subtly broken library exporting mechanism when used with
@gerkey I have an idea of how to setup a simple(ish) set of workspaces which reproduces the issue, but they'll be sort of contrived. If you'd like to get together in the near future maybe we can work through it on a white board. Unfortunately I don't remember the exact scenario where this caused us a problem before, so finding an existing package with this issue may not be the easiest way to reproduce the problem. |
@wjwwood You need to link against two libraries which work with one order but not the other. Usually you do that by having one library require symbols from the other library. While the symbols required for the executable will always be found - independent of the library order - the symbols used in one library provided by the other will only work for one of the the cases. Based on that you can construct the two workspaces. |
@tho- I appreciate that using something other than
|
Hypothesis: For libs, When we specify in So, a potential fix is to modify Before I implement this fix, any comments or alternative suggestions? |
To clarify, my proposal is:
|
@gerkey That sounds reasonable, and I trust your diagnosis of the issue, but do we have a test case yet that will tell us if it is resolved? Put another way, how will we know it works after your change? I started working on a test setup for this yesterday, but I didn't finish it yet. It seems likely that what you've described may be the case. When we were fixing the compatibility between catkin and rosbuild when I first joined WG, I remember that we always approached the issues with the mind set that we would not change rosbuild if at all possible. So we may have originally arrived at this solution we have by avoiding the consideration that we could change rosbuild to fix the issue. |
It turns out that I couldn't fix the behavior in rosbuild, because rospack doesn't offer a subcommand to give the entire libs string. The fix, which is pretty minor, needs to go in rospack: ros/rospack#48. I'm working on a test case for it. |
While the patch from ros/ros#87 fixes building with If someone would like to work on a PR for that it would be highly appreciated. |
ROS Jade creates library link commands of the form "-l:/full/path/to/lib.so" in the generated pkg-config files. This used to work with older ld version, but does not longer with recent versions. Therefore, adopt a workaround discussed for rosbuild by removing "-l:", as ld allows to simply name shared libraries on the command line. Confer: ros/catkin#694 ros/ros#87
I will set the milestone to |
We ran into this problem again while writing some examples of building against ROS packages directly with pkg-config. So far, we've been working around it with the |
I was also just surprised by this. We are using pkgconfig to depend on some ROS packages in a qmake project. The following works fine on trusty / indigo, but fails on xenial / kinetic:
We might have to resort to manually calling |
@NikolausDemmel I agree with you that having to work around this by modifying the output of pkg-config is pretty bad. I suggested an alternative above which wouldn't require such hacks but @gerkey decided to implement the workaround in Imo it would be better to actually fix what |
Thanks for the update Dirk.
I would love to, but I am not sure when I will have the time (which usually means never, at least until priorities shift, unfortunately...). We are still mostly using trusty / indigo, but I recently needed to release one package for xenial/kinetic for one xenial machine... For that we have simply modified the pc file after install as a quick and dirty workaround. |
I just ran into this problem again while trying to link to some ROS libraries directly via pkg-config from a non-ROS project. Can we assume that all platforms running Kinetic now have If so, couldn't we just merge @kartikmohta 's proposal (slightly modified) into the diff --git a/cmake/catkin_package.cmake b/cmake/catkin_package.cmake
index 07613ce..f2a13ad 100644
--- a/cmake/catkin_package.cmake
+++ b/cmake/catkin_package.cmake
@@ -272,7 +272,8 @@ function(_catkin_package)
if(IS_ABSOLUTE ${library})
get_filename_component(suffix ${library} EXT)
if(NOT "${suffix}" STREQUAL "${CMAKE_STATIC_LIBRARY_SUFFIX}")
- set(library "-l:${library}")
+ get_filename_component(libname ${library} NAME)
+ set(library "-l${libname}")
endif()
else()
set(library "-l${library}") Look at it this way: On systems with the new version of I haven't tested the proposal above yet; my quick and dirty workaround was to simply replace all occurrences of |
Or, alternatively, follow @gerkey 's example and keep the absolute paths without any diff --git a/cmake/catkin_package.cmake b/cmake/catkin_package.cmake
index 07613ce..f2a13ad 100644
--- a/cmake/catkin_package.cmake
+++ b/cmake/catkin_package.cmake
@@ -272,7 +272,8 @@ function(_catkin_package)
if(IS_ABSOLUTE ${library})
get_filename_component(suffix ${library} EXT)
if(NOT "${suffix}" STREQUAL "${CMAKE_STATIC_LIBRARY_SUFFIX}")
- set(library "-l:${library}")
+ set(library "${library}")
endif()
else()
set(library "-l${library}") |
It could very well be that newer platforms won't be affected anymore and it would be safe to use either of the proposed changes. This will require some significant testing though (including reproduce the original problem and than confirm that it has been resolved in newer versions across all targeted platforms). I will likely not have the bandwidth to do that so if anybody would like to spend that effort that would be great. |
Ok, I've set up a small repo to demonstrate the original problem and to confirm that it affects all supported platforms from Xenial onwards: https://github.com/mintar/catkin_pkgconfig_tests Quick summary: The catkin-generated
sudo find /opt/ros/$ROS_DISTRO -name '*.pc' -exec sed -i -e 's/-l://g' {} \; If you have any suggestions what else I should test, please let me know or submit a PR to my test repo. Is there a way to keep the current behavior for Trusty and implement a quick fix like the one from my previous comment for all other platforms? Remember, it's already broken on those platforms, so doing anything is better than doing nothing and hoping a future version of ld will restore the |
The interesting question is on which platforms the
A potential fix could only be applied to the kinetic-devel branch and would not affect Indigo and Jade. |
Ok, the answer to that question is easy: none of the platforms except perhaps Merging the fix into kinetic-devel cannot hurt and will fix at least 99% of the use cases (right now, 100% are broken). If we later find that some reordering takes place, we'll have a concrete example that needs a more elaborate fix. As @gerkey wrote 2 years ago, he hasn't seen any reordering in pkg-config 0.28 (Utopic and later), so we should be good. Note that we don't change Trusty, where the current version is tested and tried and we know it works. We change the other platforms, where we know that the current version is broken. |
That sounds indeed easy. If we only target |
I've just tested this and it works for my use case! |
Thank you for your effort on this long standing ticket. |
Thanks @mintar and @dirk-thomas ! |
Any ETA when this will land in kinetic? If I see it correctly, there was a tag containing this fix created in July ( |
The new version was released only into Lunar during the last release round. This will certainly be released into Kinetic when the next round of ROS core packages are released into Kinetic. I would estimate that this happens in the next 2-3 weeks - but don't quote me on this 😉 |
pkg-config files are generated with items like -l:/full/path/to/library.so.
(See https://github.com/ros/catkin/blob/indigo-devel/cmake/catkin_package.cmake#L264 )
This does not seem to work with ld-2.24.90. Only -l:library.so works (also, -L/ -l:/full/path/ seems to work, but it's not clear if this is an intended behaviour or not). The ld manual only talks about -l:library.so.
I could not find yet it this is a regression in ld or a deliberate change.
One option could be to change catkin so that it generates proper -L/full/path -lrary in the .pc to link with /full/path/library.so.
Another option is to add an explicit -L/ in front of the -l: items.
Any comment?
The text was updated successfully, but these errors were encountered: