Skip to content

[curl] Avoid targets in CURL_LIBRARIES - #29061

Merged
BillyONeal merged 2 commits into
microsoft:masterfrom
dg0yt:curl-wrapper
Jan 21, 2023
Merged

[curl] Avoid targets in CURL_LIBRARIES#29061
BillyONeal merged 2 commits into
microsoft:masterfrom
dg0yt:curl-wrapper

Conversation

@dg0yt

@dg0yt dg0yt commented Jan 19, 2023

Copy link
Copy Markdown
Contributor

Fixes #28827.
Needed for #27261.
Validated on x64-osx with vcpkg install curl[openssl,ssh] aws-sdk-cpp.

github-actions[bot]
github-actions Bot previously approved these changes Jan 19, 2023
View reviewed changes
@dg0yt dg0yt mentioned this pull request Jan 19, 2023
Closed
@LilyWangLL LilyWangLL added the category:port-bug The issue is with a library, which is something the port should already support label Jan 19, 2023
@dg0yt

dg0yt commented Jan 19, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Port cmake stumbled over LibArchive_LIBRARIES. This variable being a mix of file paths (for libarchive.a) and targets (for usage requirements), the list was added in proper order to CMakeLib, but ended in another order in the linker command line: The targets were only in the beginning of the line, libarchive.a came later.
I don't want to fully resolve to file paths at the moment. If we need targets for usage requirements, and downstream targets were able to resolve targets, we can simply re-use the LibArchive::LibArchive target when available. This the other alternative extreme wrt the curl fix: Only use targets. In both cases, do not mix library filepaths and library targets.

Failing vs. fixed build, space replace with line breaks:

--- /tmp/a	2023-01-19 22:04:58.856757391 +0100
+++ /tmp/b	2023-01-19 22:05:02.404762855 +0100
@@ -9,6 +9,7 @@
 Source/kwsys/libcmsys.a
 Utilities/std/libcmstd.a
 /home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libexpat.a
+/home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libarchive.a
 /home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libbz2d.a
 /home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libxml2.a
 /home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libz.a
@@ -18,12 +19,10 @@
 /home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libzstd.a
 /home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libcrypto.a
 /home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libz.a
-/home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libarchive.a
 /home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libcurl-d.a
 /home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libssl.a
 /home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libcrypto.a
 /home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libz.a
-/home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libarchive.a
 /home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libcurl-d.a
 /home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libssl.a
 /home/dg0yt/Projekte/vcpkg/vcpkg/installed/x64-linux/debug/lib/libcrypto.a

BillyONeal
BillyONeal approved these changes Jan 20, 2023
View reviewed changes

@BillyONeal BillyONeal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I prefer the only use targets answer but this seems reasonable to me.

Comment thread ports/curl/vcpkg-cmake-wrapper.cmake
string(REGEX REPLACE "([\$]<[^;]*)?OpenSSL::(SSL|Crypto)([^;]*>)?" "${OPENSSL_LIBRARIES}" _curl_link_libraries "${_curl_link_libraries}")
endif()
if(_curl_link_libraries MATCHES "Libssh2::libssh2")
# TODO: move find_dependency(Libssh2 CONFIG) into CURL config

@BillyONeal BillyONeal Jan 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
# TODO: move find_dependency(Libssh2 CONFIG) into CURL config

@dg0yt dg0yt Jan 20, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would prefer to leave the comment.
It is a reminder to do a patch which must be upstreamed.
I just didn't want to add it here because we are also waiting for a port update to 7.87.

@LilyWangLL LilyWangLL added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Jan 20, 2023
@dg0yt

dg0yt commented Jan 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I prefer the only use targets answer but this seems reasonable to me.

I would prefer targets, but they don't work in other ways, such as substituting them into files for pkg-config, or across sub-projects (#28827 (comment)).

@BillyONeal
BillyONeal merged commit b9152d1 into microsoft:master Jan 21, 2023
@BillyONeal

BillyONeal commented Jan 21, 2023

Copy link
Copy Markdown
Member

OK, thanks for the fix!

@dg0yt
dg0yt deleted the curl-wrapper branch January 21, 2023 08:24
GordonSmith added a commit to GordonSmith/vcpkg that referenced this pull request Jan 23, 2023
@GordonSmith
fixL Bump curl and libarchive port versions
bb48801 
Fixes issue with OSX builds on GH Actions

See:
microsoft#28827
microsoft#29061

Signed-off-by: Gordon Smith <GordonJSmith@gmail.com>
GordonSmith added a commit to GordonSmith/vcpkg that referenced this pull request Jan 23, 2023
@GordonSmith
fix: Bump curl and libarchive port versions
717be14 
Fixes issue with OSX builds on GH Actions

See:
microsoft#28827
microsoft#29061

Signed-off-by: Gordon Smith <GordonJSmith@gmail.com>
GordonSmith added a commit to GordonSmith/vcpkg that referenced this pull request Feb 2, 2023
@GordonSmith
fix: APR 1.7.0 dropped from server
301eaf0 
fix:  Race condition during cpp-driver configure / build
fix:  Bump curl and libarchive port versions
fix:  Issue with OSX builds on GH Actions
See:
microsoft#28827
microsoft#29061
chore:  Squash commits for 8.12.0 gold release

Signed-off-by: Gordon Smith <GordonJSmith@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BillyONeal BillyONeal BillyONeal approved these changes
@github-actions github-actions[bot] github-actions[bot] approved these changes
+1 more reviewer
@LilyWangLL LilyWangLL LilyWangLL approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@LilyWangLL LilyWangLL

Labels

category:port-bug The issue is with a library, which is something the port should already support 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.

[aws-sdk-cpp] Build error

3 participants

@dg0yt @BillyONeal @LilyWangLL