Skip to content

[ls-qpack] Add cmake export#40652

Merged
vicroms merged 2 commits intomicrosoft:masterfrom
talregev:TalR/ls-qpack-cmake
Aug 29, 2024
Merged

[ls-qpack] Add cmake export#40652
vicroms merged 2 commits intomicrosoft:masterfrom
talregev:TalR/ls-qpack-cmake

Conversation

@talregev
Copy link
Copy Markdown
Contributor

@talregev talregev commented Aug 26, 2024 •
edited
Loading

  • 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.

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 26, 2024

I try to add export cmake and it create an error that I don't know how to solve. Help will be appreciate.

CMake Error in CMakeLists.txt:
  Target "ls-qpack" INTERFACE_INCLUDE_DIRECTORIES property contains path:

    "C:/src/vcpkg/buildtrees/ls-qpack/src/v2.5.4-e6ccc134ae.clean/."

  which is prefixed in the source directory.


CMake Error in CMakeLists.txt:
  Target "ls-qpack" INTERFACE_INCLUDE_DIRECTORIES property contains path:

    "C:/src/vcpkg/buildtrees/ls-qpack/src/v2.5.4-e6ccc134ae.clean/wincompat"

  which is prefixed in the source directory.

@talregev talregev force-pushed the TalR/ls-qpack-cmake branch from 13cdf46 to bb67777 Compare August 26, 2024 19:29
@talregev talregev changed the title [ls-qpack][help needed] Add cmake export [ls-qpack] Add cmake export Aug 26, 2024
@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 26, 2024

I solve the error

@talregev talregev marked this pull request as ready for review August 26, 2024 19:36
@MonicaLiu0311 MonicaLiu0311 self-assigned this Aug 27, 2024
@MonicaLiu0311 MonicaLiu0311 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Aug 27, 2024
dg0yt
dg0yt reviewed Aug 27, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

I can't check the details now, but I would expect that the exported config needs a find_dependency for xxhash at least for static linkage - that's also what we learned from the msh PR.

@MonicaLiu0311
Copy link
Copy Markdown
Contributor

MonicaLiu0311 commented Aug 27, 2024

usage:

ls-qpack provides CMake targets:

  # this is heuristically generated, and may not be correct
  find_package(unofficial-ls-qpack CONFIG REQUIRED)
  target_link_libraries(main PRIVATE unofficial::ls-qpack::ls-qpack)

When testing usage, the following error occurs:

1> [CMake] -- Configuring done (3.6s)
1> [CMake] CMake Error at E:/ls-qpack/installed/x64-windows/share/unofficial-ls-qpack/unofficial-ls-qpack-config.cmake:60 (set_target_properties):
1> [CMake]   The link interface of target "unofficial::ls-qpack::ls-qpack" contains:
1> [CMake] 
1> [CMake]     PkgConfig::XXH
1> [CMake] 
1> [CMake]   but the target was not found.  Possible reasons include:
1> [CMake] 
1> [CMake]     * There is a typo in the target name.
1> [CMake]     * A find_package call is missing for an IMPORTED target.
1> [CMake]     * An ALIAS target is missing.
1> [CMake] 
1> [CMake] Call Stack (most recent call first):
1> [CMake]   E:/ls-qpack/scripts/buildsystems/vcpkg.cmake:859 (_find_package)
1> [CMake]   CMakeProject/CMakeLists.txt:14 (find_package)
test.cpp 
#include <iostream>
#include "lsqpack.h"

using namespace std;


int main()

{

cout << "Hello CMake." << endl;

return 0;

}

CMakeLists.txt 
cmake_minimum_required (VERSION 3.8)

set(CMAKE_TOOLCHAIN_FILE "E:/ls-qpack/vcpkg/scripts/buildsystems/vcpkg.cmake")


project ("test")


add_library (main "test.cpp")


find_package(unofficial-ls-qpack CONFIG REQUIRED)

target_link_libraries(main PRIVATE unofficial::ls-qpack::ls-qpack)

@MonicaLiu0311 MonicaLiu0311 marked this pull request as draft August 27, 2024 08:18
@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 27, 2024

@dg0yt Do you have suggestion here?

@talregev talregev force-pushed the TalR/ls-qpack-cmake branch from bb67777 to fa3e025 Compare August 27, 2024 15:13
@talregev talregev marked this pull request as ready for review August 27, 2024 15:14
@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 27, 2024

@MonicaLiu0311 I change the way we discover xxhash.
Can you test it again?

@MonicaLiu0311
Copy link
Copy Markdown
Contributor

MonicaLiu0311 commented Aug 28, 2024

It's also missing this:
image

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 28, 2024

@MonicaLiu0311
Should I create a usage file?

ls-qpack provides CMake targets:

  include(CMakeFindDependencyMacro)
  find_dependency(xxhash CONFIG)
  find_package(unofficial-ls-qpack CONFIG REQUIRED)
  target_link_libraries(main PRIVATE unofficial::ls-qpack::ls-qpack)

@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 28, 2024

@MonicaLiu0311 I added usage file. Can you verify?

MonicaLiu0311
MonicaLiu0311 reviewed Aug 28, 2024
View reviewed changes
MonicaLiu0311
MonicaLiu0311 reviewed Aug 28, 2024
View reviewed changes
@talregev talregev force-pushed the TalR/ls-qpack-cmake branch from 29cf6ed to 9c84351 Compare August 28, 2024 10:13
@talregev talregev requested a review from MonicaLiu0311 August 28, 2024 10:15
MonicaLiu0311
MonicaLiu0311 previously approved these changes Aug 28, 2024
View reviewed changes
@MonicaLiu0311
Copy link
Copy Markdown
Contributor

MonicaLiu0311 commented Aug 28, 2024

The usage test passed on x64-windows (header files found):

ls-qpack provides CMake targets:

  find_package(unofficial-ls-qpack CONFIG REQUIRED)
  target_link_libraries(main PRIVATE unofficial::ls-qpack::ls-qpack)

@MonicaLiu0311 MonicaLiu0311 added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Aug 28, 2024
dg0yt
dg0yt suggested changes Aug 28, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

This port lacks vcpkg_cmake_config_fixup. It is mandatory for multi-config installation.

@talregev talregev force-pushed the TalR/ls-qpack-cmake branch from 9c84351 to 18676bb Compare August 28, 2024 15:30
@talregev
Copy link
Copy Markdown
Contributor Author

talregev commented Aug 28, 2024

This port lacks vcpkg_cmake_config_fixup. It is mandatory for multi-config installation.

Done.

@talregev talregev requested a review from MonicaLiu0311 August 28, 2024 15:30
dg0yt
dg0yt suggested changes Aug 28, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM. 👍

@talregev talregev force-pushed the TalR/ls-qpack-cmake branch from 18676bb to f85588f Compare August 28, 2024 15:56
@vicroms vicroms merged commit 89685cc into microsoft:master Aug 29, 2024
@talregev talregev deleted the TalR/ls-qpack-cmake branch August 29, 2024 07:11
@talregev talregev mentioned this pull request Aug 31, 2024
Merged
@anders-wind anders-wind mentioned this pull request Sep 2, 2024
Merged
7 tasks
@talregev talregev mentioned this pull request Dec 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@dg0yt dg0yt dg0yt requested changes

@MonicaLiu0311 MonicaLiu0311 MonicaLiu0311 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@MonicaLiu0311 MonicaLiu0311

Labels

category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@talregev @MonicaLiu0311 @dg0yt @vicroms