Skip to content

Fix option --libs output library path#2402

Closed
JackBoosY wants to merge 2 commits intowxWidgets:masterfrom
JackBoosY:dev/jack/fix-libs-prefix
Closed

Fix option --libs output library path#2402
JackBoosY wants to merge 2 commits intowxWidgets:masterfrom
JackBoosY:dev/jack/fix-libs-prefix

Conversation

@JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Jun 23, 2021

The dep does not consider the cmake generator expression and the absolute path.
We may meet some different situation:

  1. dep was already handled like -pthread:
    dep_name should be ${dep}.
  2. dep is system library like dl / m:
    dep_name should be -l${dep}
  3. dep is $<$<CONFIG:DEBUG>:LIB_PATH> or $<$<NOT:$<CONFIG:DEBUG>>:LIB_PATH> that from LIB-config.cmake:
    dep_name should be ${LIB_PATH}.
  4. dep is an absolute path like /usr/home/lib.a or /usr/home/lib.so etc:
    dep_name should be ${dep}.

Related: microsoft/vcpkg#18580

@vadz
Copy link
Contributor

vadz commented Jun 24, 2021

@MaartenBent could you please review this? TIA!

@vadz vadz added the CMake Anything related to CMake build system label Jun 24, 2021
Copy link
Contributor

@MaartenBent MaartenBent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks determining the external libraries, see my inline comments.

string(REGEX REPLACE "^.+>:(.+)>$" "\\1" dep_name ${dep})
if (NOT dep_name)
set(dep_name ${dep})
endif()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove get_filename_component(dep_name ${dep} NAME)? We need this to get the library name.
With this change it will be /Applications/Xcode.app/Contents/..../MacOSX11.3.sdk/usr/lib/libz.tbd instead of -lz, or /usr/lib/x86_64-linux-gnu/libX11.so instead of -lX11.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some port use *Config.cmake or *-config.cmake, the INTERFACE_LINK_LIBRARIES may contains cmake generator expression like $<$<NOT:$<CONFIG:DEBUG>>:LIB_PATH>.
If we use get_filename_component(dep_name ${dep} NAME), the value of dep_name is NAME>.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But can you use the regex to extract the library from the generator expression, and then use get_filename_component on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why you use the file name directly here.
If the dependency library is not in system like /usr/lib, the compiler will fail to search this library.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try to generate the same output as configure does when generating wx-config. and it only has the library names.
If it is in a different path, maybe we have to specify -Lpath/to/lib before the library. Or require the user to have it in PATH.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we must choose one way to solve this issue.

Co-authored-by: Maarten <MaartenBent@users.noreply.github.com>
@talregev
Copy link

@JackBoosY @MaartenBent Any news?

@MaartenBent
Copy link
Contributor

I can't really work on this, since on the build systems I can test this on (Windows MSVC & MinGW, Ubuntu, macOS) I do not get paths with generator expressions.
Maybe you can give a few examples of such paths?

What about the following solution?
If dep contains a generator expression, we extract both the path and library name. In pseudo code:

if (dep contains generator expression)
    dep_path = regex to extract path
    dep_name = regex to extract name
else()
    get_filename_component(dep_name ${dep} NAME)
endif()

And before adding the library name, we add the library path:

if (dep_path)
    wx_string_append(${var} "-L${dep_path} ")
endif()
if(dep_name STREQUAL "libc.so")
...

This should hopefully fix your problem. Some other additions, but probably not needed immediately, is keeping track of the added dep_path so they are added only once. And maybe extracting the path in the no-generator-expression case too.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 14, 2021

Thanks for working on this. I hope this gets resolved soon so we can use vcpkg for Tenacity.

@talregev
Copy link

talregev commented Jul 14, 2021

Also for our side can get resolve:
use vcpkg in our linux manager
BOINC/boinc#4385

Copy link
Contributor

@MaartenBent MaartenBent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After rethinking this, I suppose it could be merged as it is now.
It will still have some libraries with full path and extension even when it is not required (like I mentioned earlier #2402 (comment)), but I suppose it is more important to keep the path in case it is not in a default location.

And if needed, we could add something in the future that strips default locations like /usr/lib or the macOS sdk location.

@vadz
Copy link
Contributor

vadz commented Jul 15, 2021

Thanks for the changes and for the review, merging it now with just a minor fix for a typo in a comment.

@vadz vadz closed this in c284c88 Jul 15, 2021
@JackBoosY JackBoosY deleted the dev/jack/fix-libs-prefix branch July 16, 2021 02:04
AliKet pushed a commit to AliKet/wxWidgets that referenced this pull request Jul 27, 2021
Don't prepend "-l" if a dependency is not a library name.

Closes wxWidgets#2402
csomor pushed a commit to csomor/wxWidgets that referenced this pull request Aug 26, 2021
Don't prepend "-l" if a dependency is not a library name.

Closes wxWidgets#2402
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CMake Anything related to CMake build system

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants