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

[treehopper] Fix dependence libusb #35480

Merged
merged 19 commits into from
Dec 22, 2023

Conversation

FrankXie05
Copy link
Contributor

Fix #35435 (comment)
Note: Fix correct libusb usage.
Error:

/treehopper/src/1.11.3-f321b6be5f.clean/C++/API/inc/LibUsbConnection.h:9:10: fatal error: 'libusb-1.0/libusb.h' file not found
#include <libusb-1.0/libusb.h>
         ^~~~~~~~~~~~~~~~~~~~~
1 warning and 1 error generated.

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@FrankXie05 FrankXie05 added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Dec 4, 2023
target_link_libraries(treehopper pthread ${CORE_FOUNDATION} ${IOKIT})
elseif(UNIX)
- target_link_libraries(treehopper usb-1.0 pthread)
+ find_package(libusb CONFIG REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
+ find_package(libusb CONFIG REQUIRED)
+ find_package(libusb REQUIRED)

There is no config, so don't hard-code it. (There is not even a find module. Just a vcpkg wrapper.)

+++ b/C++/API/CMakeLists.txt
@@ -37,7 +37,9 @@ if(APPLE)
find_library(IOKIT IOKit)
target_link_libraries(treehopper pthread ${CORE_FOUNDATION} ${IOKIT})
Copy link
Contributor

Choose a reason for hiding this comment

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

FTR IMO this should also be patched, to decouple cached artifacts from local SDK paths. Untested:

Suggested change
target_link_libraries(treehopper pthread ${CORE_FOUNDATION} ${IOKIT})
target_link_libraries(treehopper pthread "-framework CoreFoundation" "-framework IOKit")

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 think we need to change the behavior of upstream:

find_library(IOKIT IOKit)

@dg0yt
Copy link
Contributor

dg0yt commented Dec 5, 2023

Use libusb pkgconfig, not vcpkg's cmake package hack.

@MonicaLiu0311 MonicaLiu0311 mentioned this pull request Dec 5, 2023
7 tasks
@FrankXie05 FrankXie05 reopened this Dec 15, 2023
@FrankXie05 FrankXie05 marked this pull request as ready for review December 18, 2023 02:46
Cheney-W
Cheney-W previously approved these changes Dec 20, 2023
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Dec 20, 2023
@BillyONeal
Copy link
Member

Use libusb pkgconfig, not vcpkg's cmake package hack.

I'm pretty sure in general that we currently prefer the unofficial cmake package because we don't have a great answer for what happens for pkgconfig on Windows, as downstream consumers of us don't usually have a copy of pkgconfig that their CMake is going to find.

That also said, that doesn't apply in this case because the libusb backend is never selected on Windows.

@Neumann-A
Copy link
Contributor

because we don't have a great answer for what happens for pkgconfig on Windows, as downstream consumers of us don't usually have a copy of pkgconfig that their CMake is going to find.

Just depend on port pkgconf on windows and cmake will find that without problems.

@BillyONeal
Copy link
Member

Just depend on port pkgconf on windows and cmake will find that without problems.

Not in downstream's CMake

BillyONeal
BillyONeal previously approved these changes Dec 20, 2023
@Neumann-A
Copy link
Contributor

Not in downstream's CMake

It does. Did you forgot about:

option(VCPKG_SETUP_CMAKE_PROGRAM_PATH "Enable the setup of CMAKE_PROGRAM_PATH to vcpkg paths" ON)
set(VCPKG_CAN_USE_HOST_TOOLS OFF)
if(DEFINED VCPKG_HOST_TRIPLET AND NOT VCPKG_HOST_TRIPLET STREQUAL "")
set(VCPKG_CAN_USE_HOST_TOOLS ON)
endif()
cmake_dependent_option(VCPKG_USE_HOST_TOOLS "Setup CMAKE_PROGRAM_PATH to use host tools" ON "VCPKG_CAN_USE_HOST_TOOLS" OFF)
unset(VCPKG_CAN_USE_HOST_TOOLS)
if(VCPKG_SETUP_CMAKE_PROGRAM_PATH)
set(tools_base_path "${VCPKG_INSTALLED_DIR}/${VCPKG_TARGET_TRIPLET}/tools")
if(VCPKG_USE_HOST_TOOLS)
set(tools_base_path "${VCPKG_INSTALLED_DIR}/${VCPKG_HOST_TRIPLET}/tools")
endif()
list(APPEND CMAKE_PROGRAM_PATH "${tools_base_path}")
file(GLOB Z_VCPKG_TOOLS_DIRS LIST_DIRECTORIES true "${tools_base_path}/*")
file(GLOB Z_VCPKG_TOOLS_FILES LIST_DIRECTORIES false "${tools_base_path}/*")
file(GLOB Z_VCPKG_TOOLS_DIRS_BIN LIST_DIRECTORIES true "${tools_base_path}/*/bin")
file(GLOB Z_VCPKG_TOOLS_FILES_BIN LIST_DIRECTORIES false "${tools_base_path}/*/bin")
list(REMOVE_ITEM Z_VCPKG_TOOLS_DIRS ${Z_VCPKG_TOOLS_FILES} "") # need at least one item for REMOVE_ITEM if CMake <= 3.19
list(REMOVE_ITEM Z_VCPKG_TOOLS_DIRS_BIN ${Z_VCPKG_TOOLS_FILES_BIN} "")
string(REPLACE "/bin" "" Z_VCPKG_TOOLS_DIRS_TO_REMOVE "${Z_VCPKG_TOOLS_DIRS_BIN}")
list(REMOVE_ITEM Z_VCPKG_TOOLS_DIRS ${Z_VCPKG_TOOLS_DIRS_TO_REMOVE} "")
list(APPEND Z_VCPKG_TOOLS_DIRS ${Z_VCPKG_TOOLS_DIRS_BIN})
foreach(Z_VCPKG_TOOLS_DIR IN LISTS Z_VCPKG_TOOLS_DIRS)
list(APPEND CMAKE_PROGRAM_PATH "${Z_VCPKG_TOOLS_DIR}")
endforeach()
unset(Z_VCPKG_TOOLS_DIR)
unset(Z_VCPKG_TOOLS_DIRS)
unset(Z_VCPKG_TOOLS_FILES)
unset(Z_VCPKG_TOOLS_DIRS_BIN)
unset(Z_VCPKG_TOOLS_FILES_BIN)
unset(Z_VCPKG_TOOLS_DIRS_TO_REMOVE)
unset(tools_base_path)
endif()

Since CMAKE_PROGRAM_PATH is modified find_program will actually find pkgconf if it is installed by vcpkg

@BillyONeal
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BillyONeal
Copy link
Member

LLVM failure was a random ICE we aren't normally seeing in CI, so let's hope simply rerunning fixes it

@dg0yt
Copy link
Contributor

dg0yt commented Dec 21, 2023

Just depend on port pkgconf on windows and cmake will find that without problems.

"Without problems" is not true.
Our cmake maintainer functions do not set VCPKG_HOST_TRIPLET. So the host pkgconf is not found without extra options.
Related discussion is (at least) in #25529.

@BillyONeal BillyONeal merged commit f304349 into microsoft:master Dec 22, 2023
15 checks passed
@FrankXie05 FrankXie05 deleted the dev/Frank/treehopper_libusb branch December 22, 2023 01:59
Osyotr pushed a commit to Osyotr/vcpkg that referenced this pull request Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants