Skip to content
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

[libusb] Compatibility to projects with imported targets #18442

Closed
SunBlack opened this issue Jun 14, 2021 · 12 comments
Closed

[libusb] Compatibility to projects with imported targets #18442

SunBlack opened this issue Jun 14, 2021 · 12 comments
Assignees
Labels
category:question This issue is a question

Comments

@SunBlack
Copy link
Contributor

Currently within PointCloudLibrary we have the problem (PointCloudLibrary/pcl#4653) that we have a Findlibusb.cmake that creates an imported target libusb::libusb, but when you build the PCl with vcpkg the script is not called so the imported target is not created.

The question now is how to work around this problem, and there are several ways:

  1. Fix within the PCL: create the libusb::libusb target in a separate wrapper file, since the PCL's Findlibusb.cmake is never called.
  2. Add creation of libusb::libusb here
  3. Add call to _find_package(${ARGS}) at the beginning of vcpkg-cmake-wrapper.cmake - doesn't really work because the PCL's find script doesn't know the path "${_VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET} since find_library is not overridden like find_package

Is there a recommended way to handle this issue?

@JackBoosY
Copy link
Contributor

First, Findlibusb.cmake is not official.
The reason vcpkg will not call this script is that vcpkg always calls the wrapper first (if it exists).

In the PCL upstream, you can add this cmake directly and change the cmake code to use it. This will not affect the PCL in vcpkg, because we will patch it.
In vcpkg, we will choose whether to keep it in the next PCL update based on many factors.

@JackBoosY JackBoosY added the category:question This issue is a question label Jun 15, 2021
@SunBlack
Copy link
Contributor Author

The question about how to work around the problem was not related to the PCL port of vcpkg, but how in general to deal with problems of this kind when using vcpkg as a toolchain (e.g. when using cmake ../pcl --CMAKE_TOOLCHAIN_FILE=D:\vcpkg\scripts\buildsystems\vcpkg.cmake to build the dev version of the PCL) and having conflicts with the overwritten find scripts. Should we fix it in the projects or add missing cmake targets in vcpkg for example?

@JackBoosY
Copy link
Contributor

My suggestion is to add an option WITH_VCPKG_DEPENDENCY to the upstream cmake like other libraries that use vcpkg to build dependencies, and choose different methods of finding and using libusb according to this option.

@larshg
Copy link
Contributor

larshg commented Jun 17, 2021

My suggestion is to add an option WITH_VCPKG_DEPENDENCY to the upstream cmake like other libraries that use vcpkg to build dependencies, and choose different methods of finding and using libusb according to this option.

This seems like a cumbersome way to handle this. We could end up having special cases for all kinds of package managers?

If a PR for suggestion 2 is openened, wouldn't be the better solution? It would give more ways to use LIBUSB in downstream projects. Either by directly using the INCLUDE / LIBRARIES variables or by using the target defined, ie. libusb::libusb?

@JackBoosY
Copy link
Contributor

This can be accepted if the upstream libusb adds this method or cmake provides this method. Otherwise, other ports may also provide the same method and cannot achieve consistency.

@dg0yt
Copy link
Contributor

dg0yt commented Jun 18, 2021

I think this particular conflict is the usage of find_package with custom find modules. IMO this is more a legacy CMake approach which could cause a lot of trouble when integrating components which each had their own variant of FindXy. My POV on modern CMake is:

  • Use find_package for CMake standard find modules, dependency-provided cmake config files. This is where vcpkg or other environments inject customization.
  • Use plain include for cmake code where you rely on your particular implementation. No external interference.

This style makes it clear whether you use external find support or internal build system features.

This is not meant offensive - I just want to present an alternative which is not listed in the original post.

@SunBlack
Copy link
Contributor Author

Using plan include is bad style too. When you have e.g.:

# Main CMakeLists.txt
if(WIN32)
	set(Win32_WinExtras WinExtras)
endif()
find_package(MyDependency 5.14 COMPONENTS Gui Test ${Win32_WinExtras} REQUIRED)

it would really decrease readability when you have to write instead

# Main CMakeLists.txt
set(MyDependency_Components Gui Test)
set(MyDependency_MinVersion 5.14)
if(WIN32)
	list(APPEND MyDependency_Components WinExtras)
endif()
include(find_package_mydependency.cmake)

# Main find_package_mydependency.cmake
find_package(MyDependency ${MyDependency_MinVersion} COMPONENTS ${MyDependency_Components} REQUIRED) # call to find_package vcpkg or native module
...
# additional logic which is usually within own find script

There are some reasons why using find_package with own find scripts is better than using include:

  • Just using include without creating a method within this include: Readability decreased as you need the knowledge which variables this include is processing
  • Using include to define an own method like find_MyDependency: You need to duplicate the implementation of find_package yourself. So you have to implement signature parsing (cmake_parse_arguments), version comparism, REQUIRED vs OPTIONAL, ...

This leads to massively more code just to allow package managers.

@dg0yt
Copy link
Contributor

dg0yt commented Jun 18, 2021

The example sounds like Qt, but for that, my recommendation was "Use find_package for ... dependency-provided cmake config files". Anyway, I do accept that there are good reasons for using custom find modules.

@dg0yt
Copy link
Contributor

dg0yt commented Jun 18, 2021

@SunBlack wrote:

  1. Add creation of libusb::libusb [to vcpkg-cmake-wrapper]

@JackBoosY wrote:

This can be accepted if the upstream libusb adds this method or cmake provides this method. Otherwise, other ports may also provide the same method and cannot achieve consistency.

To be honest, I find this policy a little bit inconsistent on the vcpkg side. When it is not acceptable to create non-standard targets, how can it be acceptable to create non-standard cache variables and interfer with lookup of custom find modules?
When consistency is the goal, wrappers make sense for standard CMake find modules, but are risky for other purposes, unless scoped in some kind of vcpkg namespace.
From this POV, "Findlibusb.cmake is not official" implies that vcpkg must not install a wrapper for "libusb".

@SunBlack
Copy link
Contributor Author

The example sounds like Qt

I took indeed the example from Qt as I didn't had another example in our code where we have switch between different components (just replaced Qt by MyDependency to avoid the discussion that Qt isn't an library where you need an own find script) ;-).

From this POV, "Findlibusb.cmake is not official" implies that vcpkg must not install a wrapper for "libusb".

Yep, that is the point. As libusb has no find script itself it is not clear, which properties/variables are allowed to defined or not.

@JackBoosY
Copy link
Contributor

From this POV, "Findlibusb.cmake is not official" implies that vcpkg must not install a wrapper for "libusb".

According to our policy, we should not add the code for finding libusb to the wrapper, but export unofficial targets or adapt the FindLibusb.cmake provided by cmake.

@JackBoosY
Copy link
Contributor

Generally, many ports contain internal Find modules, and when building these ports using vcpkg, these modules will be used by default unless there are some path problems.
So if something goes wrong, we need to add a patch to fix it.

daschuer added a commit to daschuer/vcpkg that referenced this issue Aug 14, 2021
This allows using the port with imported targes form the mixxx module folder
Fixes: microsoft#18442
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:question This issue is a question
Projects
None yet
Development

No branches or pull requests

4 participants