-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[msquic, msh3] update msquic to 3.4.7, update msh3 to 0.7.0 #42547
Conversation
msquic port installation complain about absolute paths that I don't know where it coming from:
|
It looks like they're somehow vendoring a different OpenSSL despite
|
msquic using quictls from the beginning of this port. # Create imported target OpenSSLQuic
add_library(OpenSSLQuic INTERFACE IMPORTED)
set_target_properties(OpenSSLQuic PROPERTIES
INTERFACE_LINK_LIBRARIES "\$<\$<NOT:\$<CONFIG:DEBUG>>:\$<\$<CONFIG:Debug>:C:/src/vcpkg/buildtrees/msquic/x64-windows-rel/_deps/opensslquic-build/openssl3/debug/lib/libssl.lib>>;\$<\$<CONFIG:DEBUG>:\$<\$<CONFIG:Debug>:C:/src/vcpkg/buildtrees/msquic/x64-windows-dbg/_deps/opensslquic-build/openssl3/debug/lib/libssl.lib>>;\$<\$<NOT:\$<CONFIG:DEBUG>>:\$<\$<CONFIG:Debug>:C:/src/vcpkg/buildtrees/msquic/x64-windows-rel/_deps/opensslquic-build/openssl3/debug/lib/libcrypto.lib>>;\$<\$<CONFIG:DEBUG>:\$<\$<CONFIG:Debug>:C:/src/vcpkg/buildtrees/msquic/x64-windows-dbg/_deps/opensslquic-build/openssl3/debug/lib/libcrypto.lib>>;\$<\$<NOT:\$<CONFIG:DEBUG>>:\$<\$<NOT:\$<CONFIG:Debug>>:C:/src/vcpkg/buildtrees/msquic/x64-windows-rel/_deps/opensslquic-build/openssl3/release/lib/libssl.lib>>;\$<\$<CONFIG:DEBUG>:\$<\$<NOT:\$<CONFIG:Debug>>:C:/src/vcpkg/buildtrees/msquic/x64-windows-dbg/_deps/opensslquic-build/openssl3/release/lib/libssl.lib>>;\$<\$<NOT:\$<CONFIG:DEBUG>>:\$<\$<NOT:\$<CONFIG:Debug>>:C:/src/vcpkg/buildtrees/msquic/x64-windows-rel/_deps/opensslquic-build/openssl3/release/lib/libcrypto.lib>>;\$<\$<CONFIG:DEBUG>:\$<\$<NOT:\$<CONFIG:Debug>>:C:/src/vcpkg/buildtrees/msquic/x64-windows-dbg/_deps/opensslquic-build/openssl3/release/lib/libcrypto.lib>>"
) |
@dg0yt do you have an idea how to solve it? |
I believe the problem is that it is being fetchcontent'd; vcpkg_cmake_config_fixup doesn't know how to fix it up when it's being consumed from |
bad20f3
to
dcc56a2
Compare
5f16404
to
ef95015
Compare
@BillyONeal @WangWeiLin-MV |
d971fbd
to
ccba8d5
Compare
@WangWeiLin-MV I answered @dg0yt review comments. This PR is ready for review. |
ccba8d5
to
75f7b1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The introduction of the misquic::
CMake namespace is merged upstream, but it is not yet part of a release and thus should not be yet accepted in the vcpkg registry.
AFAIU upstream was unaware of this change breaking downstream projects (e.g. requiring patching in port msh3 here.), and vcpkg should be concerned for the same reason.
I suggested a solution but it is not yet integrated.
microsoft/msquic#4701 (comment)
I will remove the namespace from this PR, and next version we can add it back. |
1d81177
to
65575fa
Compare
@WangWeiLin-MV The PR is ready for review. |
The change of namespace not effect msh3 repo, because msh3 use msquic as internal lib, and have access to internal targets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The serious concerns are resolved.
Thanks.
@MonicaLiu0311 Can you review my PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The port usage tests fail with error C1083: Cannot open include file: 'msquic.h': No such file or directory
cmake_minimum_required(VERSION 3.25.1)
project(Proj LANGUAGES CXX)
add_executable(main c.cpp)
target_compile_features(main PRIVATE cxx_std_23)
find_package(msquic CONFIG REQUIRED)
target_link_libraries(main PRIVATE msquic)
I guess you test msquic. @dg0yt Can you take a look too? |
I don't have the knowledge to solve it. |
Did upstream merge the Put the usage in a test port, and you catch breaking changes early, and you save time. |
target_include_directories(msquic PUBLIC | ||
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../inc> | ||
- $<INSTALL_INTERFACE:${include_dest}>) | ||
+ $<INSTALL_INTERFACE:include>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test case build pass when manual specify target_include_directories
. Probably due to it missing the include path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these lines is now on one other place:
https://github.com/microsoft/msquic/blob/main/src/inc/CMakeLists.txt#L14
I can patch upstream and wait for it release.
65575fa
to
eea0aff
Compare
@WangWeiLin-MV I added a new patch that export the include to relative folder. Can you test it again? |
Test failed on configure steps.
Please test the case above. |
eea0aff
to
cef1bd8
Compare
I don't have the cpp file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point is that the interface include dir must be attached to msquic
directly (like in the original patch).
All the other targets in msquic are linked as PRIVATE at msquic build-time, i.e. they are LINK_ONLY transitive dependencies for static library linkage, but they do not forward interface include directories to the user who is linking to msquic
.
target_include_directories(inc INTERFACE | ||
$<BUILD_INTERFACE:${QUIC_INCLUDE_DIR}> | ||
- $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>) | ||
+ $<INSTALL_INTERFACE:include>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is neither necessary nor useful: inc
is a private link lib of msquic
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I remove the lines in the patch in the upstream, I change this line, because this is the source of the include folder.
microsoft/msquic#4575
This is my attempt to deal with the test fail here: #42547 (comment)
If this is not good change, as you say, please provide the solution.
I don't have enough knowledge to solve it.
If you don't have time to help (that is totally fine), I will close this PR and open an issue to update both msquic and msh3 and when you have time, please take a look on it and update it yourself.
Thank you for your comments and your help. I appreciate your knowledge. 🙏🏻
@WangWeiLin-MV Can you verify if the new change is working for your test? Thank you. |
Usage test passed with x64-windows triplet. |
The usage test failed under Linux with the following error.
|
Thank you for your time and effort for review. I don't have the knowledge to solve this issue. |
./vcpkg x-add-version --all
and committing the result.