The duplicate link may also be missing necessary -L flags,
so it causes linker errors when libvncserver is installed
in unusual places (while ${LIBVNCSERVER_LIBRARIES} DTRT).
Details
Details
Diff Detail
Diff Detail
- Repository
- R437 Desktop Sharing
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Adding as reviewers people I could find that committed to krfb in the last month, since there's no .arcconfig or krfb group.
Comment Actions
Added apol as linking expert. When he explained me how linking in cmake works few months ago, I had the impression that the new way would be the one "vncserver", but I miss a bit of context.
Comment Actions
It would be if "vncserver" were an imported target, but AFAICT it's just a left over line from the times when libvncserver was bundled with krfb. So +1 from me, but I don't think committing one small patch gives me much authority ;-)
Comment Actions
I am guessing @apol was referring to an imported target then. If you simply list a "name" cmake will run the compiler with -lname. If name is an (imported) target it will first be properly resolved and end up being -l/usr/lib/yaydyda.so, ${FOO_LIBRARIES} is the poor man's version of that which will still resolve but not have the other advantages of an imported target (automatic include flags, flags etc.). So, what currently happens is -lvncserver -l/usr/lib/libvncserver.so, the proposed change changes it to only use the explicitly defined one -l/usr/lib/libvncserver.so.
Up until https://git.reviewboard.kde.org/r/119548/ vncserver actually was a native target, the change however neglected to drop the then-removed target so we ended up with the double linking.
Comment Actions
Thanks for the explanation! Just for the sake of completeness, I guess that in order to use an "import target," someone should patch the build system of libvncserver to export the proper information, (and maybe that would require cmake), is it correct?
Comment Actions
Doing this would at least require writing a cmake config, which doesn't require cmake on the library side (OTOH it is mostly replicating pkgconfig in cmake syntax, so people tend to opt for pkgconfig because of portability). Generally speaking, imported targets would come from a cmake config generated by the library if the library is using cmake, that's how it works for frameworks for example. There's a nice write-up on stack overflow on the matter of FindFoo vs. FooConfig http://stackoverflow.com/questions/35815850/getting-imported-targets-through-find-package
That said, one could probably also do it in a finder scirpt manually https://cmake.org/Wiki/CMake/Tutorials/Exporting_and_Importing_Targets (bearing in mind that a finder still has to export all the variables it usually has to set, so the imported target is extra work on top)
Comment Actions
Although we *could* add imported-target support to cmake/modules/FindLibVNCServer.cmake in the krfb sources, that's a bigger change for little benefit (while there, we'd re-write that find-module to match variable-case with modulename-case, among other things), so I'm just going to land this change unmodified.