Fix #1446 #2253

Ericson2314 · 2024-06-26T03:37:31Z

Adapted from https://github.com/dg0yt/vcpkg/blob/issue-template/ports/libarchive/pkgconfig-modules.patch

Partially fixes #1446

kientzle · 2024-07-05T01:02:33Z

This looks interesting, but I'm not familiar enough with Pkgconfig to judge whether this is a good approach.

dg0yt · 2024-07-05T03:48:04Z

You seem to have started from the outdated https://github.com/dg0yt/vcpkg/blob/issue-template/ports/libarchive/pkgconfig-modules.patch.
You should check the latest version of the patch,
https://github.com/microsoft/vcpkg/blob/master/ports/libarchive/fix-deps.patch
It has improvements with regard to library and pc module names. For example, the spelling is lower case bcrypt for mingw on linux.

Some pc files can be missing on windows if not using vcpkg.

Ericson2314 · 2024-07-09T14:10:38Z

I split out the autoconf part in #2268 so they can be reviewed separately.

@dg0yt I thought I did work from the latest version? I skipped the other changes, such as more FIND_PACKAGE because I thought they would best be done in another PR. Those are perfectly good changes, but that fix other things than #1446. I would be happy to review porting the rest of them next.

dg0yt · 2024-07-10T06:48:56Z

I thought I did work from the latest version?

I don't know. I know that the reference you made points to an unrelated and old branch. And my comment even highlighted one of the issues of the old branch.

Ericson2314 · 2024-07-10T17:27:25Z

Sorry, I failed to find the upper-case Bcrypt before, but I see it now.

Adapted from https://github.com/dg0yt/vcpkg/blob/issue-template/ports/libarchive/pkgconfig-modules.patch

https://github.com/microsoft/vcpkg/blob/master/ports/libarchive/fix-deps.patch

Ericson2314 · 2024-07-10T17:41:36Z

@dg0yt OK I put most of the things from https://github.com/microsoft/vcpkg/blob/master/ports/libarchive/fix-deps.patch in a second commit, so we can review carefully what goes where.

See my comments below on things differing between https://github.com/microsoft/vcpkg/blob/master/ports/libarchive/fix-deps.patch and what I got that are worthy of further attention.

Ericson2314 · 2024-07-10T17:42:46Z

CMakeLists.txt

+  LIST(APPEND ADDITIONAL_LIBS LibLZMA::LibLZMA)
+  LIST(APPEND LIBARCHIVE_REQUIRES_PRIVATE liblzma)
  CMAKE_PUSH_CHECK_STATE()
  SET(CMAKE_REQUIRED_INCLUDES ${LIBLZMA_INCLUDE_DIR})
  SET(CMAKE_REQUIRED_LIBRARIES ${LIBLZMA_LIBRARIES})
  INCLUDE_DIRECTORIES(${LIBLZMA_INCLUDE_DIRS})
  LIST(APPEND ADDITIONAL_LIBS ${LIBLZMA_LIBRARIES})


What's the relationship between these two ADDITIONAL_LIBS? @dg0yt

The relationship is that vcpkg never uses the second ADDITIONAL_LIBS. You omitted the third inserted line (which doesn't make sense here).

+elseif(0)

With this trick, the vcpkg patch is intentionally kept short.

For upstreaming the change, you wouldn't insert the code at the beginning but indeed update the code around the original ADDITIONAL_LIBS line.

Ericson2314 · 2024-07-10T17:43:38Z

CMakeLists.txt

@@ -628,13 +631,16 @@ ENDIF(ENABLE_LZ4)
 IF(LZ4_FOUND)
  SET(HAVE_LIBLZ4 1)
  SET(HAVE_LZ4_H 1)
+  SET(HAVE_LZ4HC_H 1)


This seems separate?

Ericson2314 · 2024-07-10T17:44:19Z

CMakeLists.txt

+  LIST(APPEND LIBARCHIVE_REQUIRES_PRIVATE liblz4)
+elseif(0)
  CMAKE_PUSH_CHECK_STATE()	# Save the state of the variables
  SET(CMAKE_REQUIRED_INCLUDES ${LZ4_INCLUDE_DIR})
  CHECK_INCLUDE_FILES("lz4hc.h" HAVE_LZ4HC_H)
  CMAKE_POP_CHECK_STATE()	# Restore the state of the variables
  INCLUDE_DIRECTORIES(${LZ4_INCLUDE_DIR})
  LIST(APPEND ADDITIONAL_LIBS ${LZ4_LIBRARY})
-  LIST(APPEND LIBARCHIVE_REQUIRES_PRIVATE liblz4)


Does no pkg-config in the FIND_PACKAGE not found case make sense?

Ericson2314 · 2024-07-10T17:44:40Z

CMakeLists.txt

+  LIST(APPEND LIBARCHIVE_REQUIRES_PRIVATE libzstd)
+elseif(0)
  INCLUDE_DIRECTORIES(${ZSTD_INCLUDE_DIR})
  LIST(APPEND ADDITIONAL_LIBS ${ZSTD_LIBRARY})
-  LIST(APPEND LIBARCHIVE_REQUIRES_PRIVATE libzstd)


Does no pkg-config in the FIND_PACKAGE not found case make sense?

Ericson2314 · 2024-07-10T17:45:02Z

CMakeLists.txt

@@ -977,7 +992,7 @@ main(int argc, char **argv)
        IF ("${IMPLEMENTATION}" MATCHES "^OPENSSL$" AND OPENSSL_FOUND)
          INCLUDE_DIRECTORIES(${OPENSSL_INCLUDE_DIR})
 	  LIST(APPEND ADDITIONAL_LIBS ${OPENSSL_LIBRARIES})
-	  LIST(REMOVE_DUPLICATES ADDITIONAL_LIBS)
+	  #LIST(REMOVE_DUPLICATES ADDITIONAL_LIBS)


Do we want to get rid of this?

REMOVE_DUPLICATES has the potential to break static linking with "traditional linkers". Independent libs must come at the end of the list, but that command leaves only the first occurence.
In addition, it will break multi-config keywords (optimized, debug).

Ericson2314 · 2024-07-10T17:45:12Z

CMakeLists.txt

@@ -851,8 +867,7 @@ IF(ENABLE_OPENSSL AND NOT CMAKE_SYSTEM_NAME MATCHES "Darwin")
  FIND_PACKAGE(OpenSSL)
  IF(OPENSSL_FOUND)
    SET(HAVE_LIBCRYPTO 1)
-    INCLUDE_DIRECTORIES(${OPENSSL_INCLUDE_DIR})


Do we want to get rid of this?

For building libarchive, OpenSSL::Crypto' will make CMake pass include dirs and link libs to the targets that link to it. So there is no reason to use both variables and targets. For exported CMake config, using targets implies using find_dependency` (for static linkage).

dg0yt

You cannot copy the vcpkg patch literally. It is for a certain controlled environment.
You really have to check package by package. And the libarchive must check how modern the systems are required to be when relying on system packages.

dg0yt · 2024-07-10T19:49:12Z

CMakeLists.txt

+  FIND_PACKAGE(ZSTD NAMES zstd CONFIG REQUIRED)
+elseif(0)


Not suitable here.
User may not have zstd CMake config.

dg0yt · 2024-07-10T19:52:50Z

CMakeLists.txt

@@ -851,8 +867,7 @@ IF(ENABLE_OPENSSL AND NOT CMAKE_SYSTEM_NAME MATCHES "Darwin")
  FIND_PACKAGE(OpenSSL)
  IF(OPENSSL_FOUND)
    SET(HAVE_LIBCRYPTO 1)
-    INCLUDE_DIRECTORIES(${OPENSSL_INCLUDE_DIR})


For building libarchive, OpenSSL::Crypto' will make CMake pass include dirs and link libs to the targets that link to it. So there is no reason to use both variables and targets. For exported CMake config, using targets implies using find_dependency` (for static linkage).

dg0yt · 2024-07-10T19:55:21Z

CMakeLists.txt

@@ -977,7 +992,7 @@ main(int argc, char **argv)
        IF ("${IMPLEMENTATION}" MATCHES "^OPENSSL$" AND OPENSSL_FOUND)
          INCLUDE_DIRECTORIES(${OPENSSL_INCLUDE_DIR})
 	  LIST(APPEND ADDITIONAL_LIBS ${OPENSSL_LIBRARIES})
-	  LIST(REMOVE_DUPLICATES ADDITIONAL_LIBS)
+	  #LIST(REMOVE_DUPLICATES ADDITIONAL_LIBS)


REMOVE_DUPLICATES has the potential to break static linking with "traditional linkers". Independent libs must come at the end of the list, but that command leaves only the first occurence.
In addition, it will break multi-config keywords (optimized, debug).

Ericson2314 changed the title ~~Fix #1446 in CMake build~~ Fix #1446 Jun 27, 2024

Ericson2314 force-pushed the fix-1446 branch from b040036 to f6f34d3 Compare June 27, 2024 20:06

kientzle mentioned this pull request Jul 5, 2024

[libarchive,lz4] Fix pc file, modernize portfile microsoft/vcpkg#20146

Merged

4 tasks

Ericson2314 mentioned this pull request Jul 9, 2024

Fix #1446 in Autoconf build #2268

Draft

Ericson2314 force-pushed the fix-1446 branch from f6f34d3 to 76e99cd Compare July 9, 2024 14:09

Ericson2314 added 2 commits July 10, 2024 13:35

Fix libarchive#1446 in CMake build

0ebe7cc

Adapted from https://github.com/dg0yt/vcpkg/blob/issue-template/ports/libarchive/pkgconfig-modules.patch

Sync additional changes from vcpkg

975f271

https://github.com/microsoft/vcpkg/blob/master/ports/libarchive/fix-deps.patch

Ericson2314 force-pushed the fix-1446 branch from 76e99cd to 975f271 Compare July 10, 2024 17:41

Ericson2314 commented Jul 10, 2024

View reviewed changes

dg0yt reviewed Jul 10, 2024

View reviewed changes

Ericson2314 marked this pull request as draft July 11, 2024 02:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1446 #2253

Fix #1446 #2253

Ericson2314 commented Jun 26, 2024 •

edited

Loading

kientzle commented Jul 5, 2024

dg0yt commented Jul 5, 2024 •

edited

Loading

Ericson2314 commented Jul 9, 2024 •

edited

Loading

dg0yt commented Jul 10, 2024

Ericson2314 commented Jul 10, 2024

Ericson2314 commented Jul 10, 2024 •

edited

Loading

Ericson2314 Jul 10, 2024 •

edited

Loading

dg0yt Jul 10, 2024

Ericson2314 Jul 10, 2024

Ericson2314 Jul 10, 2024

Ericson2314 Jul 10, 2024

Ericson2314 Jul 10, 2024

dg0yt Jul 10, 2024

Ericson2314 Jul 10, 2024

dg0yt Jul 10, 2024

dg0yt left a comment

dg0yt Jul 10, 2024

dg0yt Jul 10, 2024

dg0yt Jul 10, 2024

Fix #1446 #2253

Are you sure you want to change the base?

Fix #1446 #2253

Conversation

Ericson2314 commented Jun 26, 2024 • edited Loading

kientzle commented Jul 5, 2024

dg0yt commented Jul 5, 2024 • edited Loading

Ericson2314 commented Jul 9, 2024 • edited Loading

dg0yt commented Jul 10, 2024

Ericson2314 commented Jul 10, 2024

Ericson2314 commented Jul 10, 2024 • edited Loading

Ericson2314 Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dg0yt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Jun 26, 2024 •

edited

Loading

dg0yt commented Jul 5, 2024 •

edited

Loading

Ericson2314 commented Jul 9, 2024 •

edited

Loading

Ericson2314 commented Jul 10, 2024 •

edited

Loading

Ericson2314 Jul 10, 2024 •

edited

Loading