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

[C++] arrow.pc is missing dependencies with Windows static builds #15139

Closed
lukester1975 opened this issue Dec 31, 2022 · 14 comments · Fixed by #33712
Closed

[C++] arrow.pc is missing dependencies with Windows static builds #15139

lukester1975 opened this issue Dec 31, 2022 · 14 comments · Fixed by #33712
Assignees
Milestone

Comments

@lukester1975
Copy link
Contributor

Describe the bug, including details regarding any error messages, version, and platform.

I have been having to manually edit arrow.pc from a vcpkg installation of a static build of arrow (using triplet x64-windows-static-md) for a few reasons. #14869 was a start, but now for the tricky (to me) one - missing dependencies. It doesn't seem to be a vcpkg issue, as non-vcpkg builds also generate "unconsumable" pkg-config files, so I'm posting here. Or maybe it goes all the way up to cmake, I don't know (I'm a cmake avoider...).

The following is using arrow master as of a0d1630.

For example, using VS2022 (17.4.3) (and cmake 3.24.202208181-MSVC_2 from there) configured as:

cmake -G Ninja -DVCPKG_TARGET_TRIPLET=x64-windows-static-md -DARROW_WITH_BROTLI=ON -DARROW_WITH_BZ2=ON 
  -DARROW_WITH_LZ4=ON -DARROW_WITH_SNAPPY=ON -DARROW_WITH_ZLIB=ON -DARROW_WITH_ZSTD=ON 
  -DARROW_BUILD_TESTS=OFF -DCMAKE_INSTALL_PREFIX=/path/to/somewhere -DARROW_DEPENDENCY_SOURCE=VCPKG 
  -DARROW_BUILD_STATIC=ON -DARROW_BUILD_SHARED=OFF -DARROW_BUILD_TESTS=OFF 
  -DARROW_DEPENDENCY_USE_SHARED=OFF -DARROW_WITH_BACKTRACE=OFF 
  -DCMAKE_MSVC_RUNTIME_LIBRARY=MultiThreadedDebugDLL -DCMAKE_BUILD_TYPE=Debug ../..

when built and installed generates arrow.pc:

...
Requires:
Requires.private:
Libs: -L${libdir} -larrow /path/to/arrow.git/cpp/vcpkg_installed/x64-windows-static-md/lib/snappy.lib optimized;/path/to/arrow.git/cpp/vcpkg_installed/x64-windows-static-md/lib/bz2.lib;debug;/path/to/arrow.git/cpp/vcpkg_installed/x64-windows-static-md/debug/lib/bz2d.lib
Libs.private:
Cflags: -I${includedir} -DARROW_STATIC
Cflags.private:

This is essentially the same as an install via vcpkg. There are a few problems here:

  • Most of the compression deps are missing (libbrotlidec libbrotlienc zlib liblz4 libzstd).The cmake invocation generates the following:

    -- pkg-config package for libbrotlidec for static link isn't found
    -- pkg-config package for libbrotlienc for static link isn't found
    -- pkg-config package for zlib for static link isn't found
    ...
    

    so not surprising, I suppose! pc files are there in the vcpkg_installed/x64-windows-static-md/debug/lib/pkgconfig dir, however (and the libs themselves a level up).

  • Libs has bz2 and snappy without -l and with a file extension and path prefix. I don't think this is ideal, as without the -l the file may be treated as a plain object file rather than import library causing issues with certain toolchains. Ultimately this could be worked around in meson, maybe, (or libpkgconf I think it uses) but...

  • bzip2 should ideally be a Requires too - since there are separate release and debug pkgconfig dirs they will cope with the file naming difference.

  • And snappy too, but its .pc file is also broken (missing completely here; Libs: -L${libdir} -l, i.e. not -lsnappy, with a vcpkg install of arrow).
    I haven't investigated that at all yet, so keep in Libs, but without a path.

I.e. a usable arrow.pc would look something like:

...
Requires: bzip2 libzstd liblz4 zlib libbrotlidec libbrotlienc libbrotlicommon
Libs: -L${libdir} -larrow -lsnappy
Cflags: -I"${includedir}" -DARROW_STATIC

Now I'm sure there are a gazillion of other cases to cope with where doing this would be wrong, but at least in the "get everything via vcpkg" case this arrow.pc works for me (and similarly editied arrow.pc from a vcpkg install of arrow).

As an aside, the same set up of options (except no CMAKE_MSVC_RUNTIME_LIBRARY and x64-linux triplet) on Fedora 37 (cmake 3.25.1) generates:

...
Requires: snappy libbrotlidec libbrotlienc zlib liblz4 libzstd
Requires.private:
Libs: -L${libdir} -larrow optimized;/path/to/arrow.git/cpp/vcpkg_installed/x64-linux/lib/libbz2.a;debug;/path/to/arrow.git/cpp/vcpkg_installed/x64-linux/debug/lib/libbz2d.a -larrow_bundled_dependencies
Libs.private:
Cflags: -I${includedir} -DARROW_STATIC
Cflags.private:

This is more reasonable, though I personally think bzip2 is handled wrong (for the same reasons as above), even if it "works". The debug/optimized stuff is still problematic but vcpkg are adding support for that there (microsoft/vcpkg#23898), so will start to work at some point - but obviously potentially not if consuming the pc file via another route.

I have spent some time hacking ThirdpartyToolchain.cmake to see if I could make the pc files more sane, but unsuccessfully. The best I could do was getting bzip2 "better" with:

if(ARROW_WITH_BZ2)
  resolve_dependency(BZip2
    PC_PACKAGE_NAMES
    bzip2)

  # resolve_dependency(BZip2)

  # if(${BZip2_SOURCE} STREQUAL "SYSTEM")
  #   string(APPEND ARROW_PC_LIBS_PRIVATE " ${BZIP2_LIBRARIES}")
  # endif()

  # if(NOT TARGET BZip2::BZip2)
  #   add_library(BZip2::BZip2 UNKNOWN IMPORTED)
  #   set_target_properties(BZip2::BZip2
  #                         PROPERTIES IMPORTED_LOCATION "${BZIP2_LIBRARIES}"
  #                                    INTERFACE_INCLUDE_DIRECTORIES "${BZIP2_INCLUDE_DIR}")
  # endif()
endif()

but what that breaks I've no idea for other cases. As I say, not a cmake person and would like to keep it that way 😀 Happy to try any suggestions, though.

Thanks!

Component(s)

C++

@kou
Copy link
Member

kou commented Dec 31, 2022

Could you show the full output of cmake?

Could you show the output of pkg-config --list-all --debug?

@kou kou self-assigned this Dec 31, 2022
@kou
Copy link
Member

kou commented Dec 31, 2022

Could you try the following for bzip2?

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index d0c8c600d5..c93c6a534d 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -2546,9 +2546,17 @@ macro(build_bzip2)
 endmacro()
 
 if(ARROW_WITH_BZ2)
-  resolve_dependency(BZip2)
-  if(${BZip2_SOURCE} STREQUAL "SYSTEM")
-    string(APPEND ARROW_PC_LIBS_PRIVATE " ${BZIP2_LIBRARIES}")
+  resolve_dependency(BZip2 PC_PACKAGE_NAMES bzip2)
+  if(${BZip2_SOURCE} STREQUAL "SYSTEM" AND NOT bzip2_PC_FOUND)
+    if(BZIP2_LIBRARY_RELEASE)
+      string(APPEND ARROW_PC_LIBS_PRIVATE " ${BZIP2_LIBRARY_RELEASE}")
+    elseif(BZIP2_LIBRARY_DEBUG)
+      string(APPEND ARROW_PC_LIBS_PRIVATE " ${BZIP2_LIBRARY_DEBUG}")
+    elseif(BZIP2_LIBRARY)
+      string(APPEND ARROW_PC_LIBS_PRIVATE " ${BZIP2_LIBRARY}")
+    else()
+      string(APPEND ARROW_PC_LIBS_PRIVATE " ${BZIP2_LIBRARIES}")
+    endif()
   endif()
 
   if(NOT TARGET BZip2::BZip2)

@lukester1975
Copy link
Contributor Author

Could you show the full output of cmake?

Could you show the output of pkg-config --list-all --debug?

cmake-cfg.txt

I don't have a PKG_CONFIG_PATH set, so essentially nothing:

pkg-config --list-all --debug
Error printing enabled by default due to use of output options besides --exists or --atleast/exact/max-version. Value of --silence-errors: 0
Error printing enabled
Adding virtual 'pkg-config' package to list of known packages
Cannot open directory 'C:/Program Files/Git/usr/lib/pkgconfig' in package search path: No such file or directory
Cannot open directory 'C:/Program Files/Git/usr/share/pkgconfig' in package search path: No such file or directory
Cannot open directory 'C:/Program Files/Git/lib/pkgconfig' in package search path: No such file or directory
Cannot open directory 'C:/ProgramData/chocolatey/lib/pkgconfiglite/tools/pkg-config-lite-0.28-1/lib/pkgconfig' in package search path: No such file or directory
Cannot open directory 'C:/ProgramData/chocolatey/lib/pkgconfiglite/tools/pkg-config-lite-0.28-1/share/pkgconfig' in package search path: No such file or directory

(As an aside, I wondered if the Fedora build generates a sane looking pc file because it's finding the compression deps using system-wide pkg-config. However, I tried unsetting PKG_CONFIG_PATH and the generated arrow.pc file still looks good...)

Regarding the patch for bzip2, that doesn't work quite right. BZIP2_LIBRARY_RELEASE, BZIP2_LIBRARY_DEBUG and BZIP2_LIBRARY are all set, so it's just a function of ordering as to what gets generated (so I ended up with bz2 rather than bz2d for a debug build).

FWIW, this largely fixes a cmake build (again I'm sure it's bad for other reasons, so just for info):

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index d0c8c600d..8354f4912 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -291,7 +291,6 @@ macro(resolve_dependency DEPENDENCY_NAME)
     foreach(ARG_PC_PACKAGE_NAME ${ARG_PC_PACKAGE_NAMES})
       pkg_check_modules(${ARG_PC_PACKAGE_NAME}_PC
                         ${ARG_PC_PACKAGE_NAME}
-                        NO_CMAKE_PATH
                         NO_CMAKE_ENVIRONMENT_PATH
                         QUIET)
       if(${${ARG_PC_PACKAGE_NAME}_PC_FOUND})
@@ -2546,8 +2545,8 @@ macro(build_bzip2)
 endmacro()
 
 if(ARROW_WITH_BZ2)
-  resolve_dependency(BZip2)
-  if(${BZip2_SOURCE} STREQUAL "SYSTEM")
+  resolve_dependency(BZip2 PC_PACKAGE_NAMES bzip2)
+  if(${BZip2_SOURCE} STREQUAL "SYSTEM" AND NOT bzip2_PC_FOUND)
     string(APPEND ARROW_PC_LIBS_PRIVATE " ${BZIP2_LIBRARIES}")
   endif()

Generates a resaonable arrow.pc:

Requires: libbrotlidec libbrotlienc zlib liblz4 libzstd bzip2
Requires.private:
Libs: -L${libdir} -larrow D:/dev/checkouts/external/arrow.git/cpp/vcpkg_installed/x64-windows-static-md/lib/snappy.lib
Libs.private:
Cflags: -I${includedir} -DARROW_STATIC
Cflags.private:

(snappy weirdness would presumably go away if a snappy.pc actually existed)

I tried adding this as a patch to vcpkg's arrow portfile but no joy (still missing compression deps). I notice it uses -DARROW_DEPENDENCY_SOURCE=SYSTEM -DARROW_PACKAGE_KIND=vcpkg there rather than -DARROW_DEPENDENCY_SOURCE=vcpkg which I haven't investigated but presumably explains why this patch doesn't work there, somehow...

Thanks

@lukester1975
Copy link
Contributor Author

Fixing snappy at source is a no-go: google/snappy#86

Hopefully vcpkg will fix theirs: microsoft/vcpkg#28675

@kou
Copy link
Member

kou commented Jan 2, 2023

Could you try the following patch with PKG_CONFIG_PATH=D:/dev/checkouts/external/arrow.git/cpp/vcpkg_installed/x64-windows-static-md/lib/pkgconfig?

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 3eda538fb2..95d425d95d 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -2571,7 +2571,28 @@ endmacro()
 if(ARROW_WITH_BZ2)
   resolve_dependency(BZip2)
   if(${BZip2_SOURCE} STREQUAL "SYSTEM")
-    string(APPEND ARROW_PC_LIBS_PRIVATE " ${BZIP2_LIBRARIES}")
+    set(ARROW_BZIP2_CONFIGS "")
+    if(BZIP2_LIBRARY_RELEASE)
+      string(APPEND ARROW_PC_LIBS_PRIVATE " release[$<$<CONFIG:RELEASE>:${BZIP2_LIBRARY_RELEASE}>]")
+      list(APPEND ARROW_BZIP2_CONFIGS "$<CONFIG:RELEASE>")
+    endif()
+    if(BZIP2_LIBRARY_DEBUG)
+      string(APPEND ARROW_PC_LIBS_PRIVATE " debug[$<$<CONFIG:DEBUG:${BZIP2_LIBRARY_DEBUG}>]")
+      list(APPEND ARROW_BZIP2_CONFIGS "$<CONFIG:DEBUG>")
+    endif()
+    string(APPEND ARROW_PC_LIBS_PRIVATE " other[$<$<NOT:$<OR:")
+    if(CMAKE_VERSION VERSION_LESS "3.12")
+      string(REPLACE ";" "," ARROW_BZIP2_CONFIGS_CSV "${ARROW_BZIP2_CONFIGS}")
+    else()
+      list(JOIN ARROW_BZIP2_CONFIGS "," ARROW_BZIP2_CONFIGS_CSV)
+    endif()
+    string(APPEND ARROW_PC_LIBS_PRIVATE "${ARROW_BZIP2_CONFIGS_CSV}>>:")
+    if(BZIP2_LIBRARY)
+      string(APPEND ARROW_PC_LIBS_PRIVATE "${BZIP2_LIBRARY}")
+    else()
+      string(APPEND ARROW_PC_LIBS_PRIVATE "${BZIP2_LIBRARIES}")
+    endif()
+    string(APPEND ARROW_PC_LIBS_PRIVATE ">]")
   endif()
 
   if(NOT TARGET BZip2::BZip2)

BTW, how did you install pkg-config? vcpkg? Chocolatey? If you use Chocolatey not vcpkg, could you try pkgconf vcpkg port instead?

@lukester1975
Copy link
Contributor Author

  • Yes, pkgconfig via Choco, though I see exactly the same behaviour with vcpkg's.
  • PKG_CONFIG_PATH=/path/to/arrow/cpp/vcpkg_installed/<triplet>/[debug]/lib/pkgconfig works in that Requires: libbrotlidec libbrotlienc zlib liblz4 libzstd gets generated. I assume the intention is not to have to do that, though? Pretty unpleasant (especially given the debug/release specific location)!
  • R.e. Linux - I was wrong, it is behaving the same, as it looks like PKG_CONFIG_PATH="" cmake ... isn't sufficient to ignore the system paths. So it was generating a sensible pc file by accident when arrow's deps are available system wide.
  • Patch won't configure, looks like a missing > - I assume it needs to be
    string(APPEND ARROW_PC_LIBS_PRIVATE " debug[$<$<CONFIG:DEBUG>:${BZIP2_LIBRARY_DEBUG}>]")
  • With that it generates: Libs: -L${libdir} -larrow D:/dev/checkouts/external/arrow.git/cpp/vcpkg_installed/x64-windows-static-md/lib/snappy.lib release[] debug[D:/dev/checkouts/external/arrow.git/cpp/vcpkg_installed/x64-windows-static-md/debug/lib/bz2d.lib] other[]
  • Not sure if that's what's intended, but cursorily it doesn't look like [vcpkg_fixup_pkgconfig] Check for more problems, add unit test microsoft/vcpkg#23898 would fixup that syntax, though maybe vcpkg is a different case altgother given -DARROW_DEPENDENCY_SOURCE=SYSTEM -DARROW_PACKAGE_KIND=vcpkg...

Thanks

@kou
Copy link
Member

kou commented Jan 4, 2023

I assume the intention is not to have to do that, though?

It's expected. I thought that Requires: libbrotlidec libbrotlienc zlib liblz4 libzstd will be generated with PKG_CONFIG_PATH=....

Ah, wait. could you show Get-ChildItem -Recurse D:/dev/checkouts/external/arrow.git/cpp/vcpkg_installed/x64-windows-static-md/?

You can disable system pkg-config search path by PKG_CONFIG_LIBDIR= pkg-config --list-all.

Sorry. XXX[...] is just for debug on my environment...

Could you try the following?

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index 3eda538fb2..0efaaa0c46 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -2571,7 +2571,28 @@ endmacro()
 if(ARROW_WITH_BZ2)
   resolve_dependency(BZip2)
   if(${BZip2_SOURCE} STREQUAL "SYSTEM")
-    string(APPEND ARROW_PC_LIBS_PRIVATE " ${BZIP2_LIBRARIES}")
+    set(ARROW_BZIP2_CONFIGS "")
+    if(BZIP2_LIBRARY_RELEASE)
+      string(APPEND ARROW_PC_LIBS_PRIVATE " $<$<CONFIG:RELEASE>:${BZIP2_LIBRARY_RELEASE}>>")
+      list(APPEND ARROW_BZIP2_CONFIGS "$<CONFIG:RELEASE>")
+    endif()
+    if(BZIP2_LIBRARY_DEBUG)
+      string(APPEND ARROW_PC_LIBS_PRIVATE " $<$<CONFIG:DEBUG:${BZIP2_LIBRARY_DEBUG}>>")
+      list(APPEND ARROW_BZIP2_CONFIGS "$<CONFIG:DEBUG>")
+    endif()
+    string(APPEND ARROW_PC_LIBS_PRIVATE " $<$<NOT:$<OR:")
+    if(CMAKE_VERSION VERSION_LESS "3.12")
+      string(REPLACE ";" "," ARROW_BZIP2_CONFIGS_CSV "${ARROW_BZIP2_CONFIGS}")
+    else()
+      list(JOIN ARROW_BZIP2_CONFIGS "," ARROW_BZIP2_CONFIGS_CSV)
+    endif()
+    string(APPEND ARROW_PC_LIBS_PRIVATE "${ARROW_BZIP2_CONFIGS_CSV}>>:")
+    if(BZIP2_LIBRARY)
+      string(APPEND ARROW_PC_LIBS_PRIVATE "${BZIP2_LIBRARY}")
+    else()
+      string(APPEND ARROW_PC_LIBS_PRIVATE "${BZIP2_LIBRARIES}")
+    endif()
+    string(APPEND ARROW_PC_LIBS_PRIVATE ">")
   endif()
 
   if(NOT TARGET BZip2::BZip2)

It will generate Libs: -L${libdir} -larrow D:/dev/checkouts/external/arrow.git/cpp/vcpkg_installed/x64-windows-static-md/lib/snappy.lib D:/dev/checkouts/external/arrow.git/cpp/vcpkg_installed/x64-windows-static-md/debug/lib/bz2d.lib. Does this work? Do we need to change *.lib to something?

@lukester1975
Copy link
Contributor Author

Apologies for the delayed response, fighting a bout of the you-know-what.

I assume the intention is not to have to do that, though?

It's expected. I thought that Requires: libbrotlidec libbrotlienc zlib liblz4 libzstd will be generated with PKG_CONFIG_PATH=....

Hmm, OK. Shame! Should the pkg-config package for ... for static link isn't founds become hard errors? It doesn't seem very nice for config / build / install to suceed without PKG_CONFIG_PATH but for the generated pc files to be missing dependencies. I suppose the instructions could do with an update to explain the PKG_CONFIG_PATH requirement.

Ah, wait. could you show Get-ChildItem -Recurse D:/dev/checkouts/external/arrow.git/cpp/vcpkg_installed/x64-windows-static-md/?

Sure, it's a bit of a beast of a listing given the includes, so zipped. I wasn't sure from what state you wanted the listing, so it is after a vcpkg install & cmake configure (without the PKG_CONFIG_PATH).

Could you try the following?

[snip]

It will generate Libs: -L${libdir} -larrow D:/dev/checkouts/external/arrow.git/cpp/vcpkg_installed/x64-windows-static-md/lib/snappy.lib D:/dev/checkouts/external/arrow.git/cpp/vcpkg_installed/x64-windows-static-md/debug/lib/bz2d.lib. Does this work? Do we need to change *.lib to something?

Patch as below (needed a tiny tweak) did indeed generate that Libs line. I think it's OK, though given the bzip2 pc files from vcpkg are OK it would seem cleaner to be to just Requires bzip2, but I guess this is coping with non-vcpkg bzip2 too and you'd rather not special case, or something?

(I honestly can't remember when or how I tripped over lib files being linked as plain object files causing problems, so happy to ignore that until it happens again ... sorry for the noise.)

Thanks!

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index d0c8c600d..d1323b575 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -2548,7 +2548,28 @@ endmacro()
 if(ARROW_WITH_BZ2)
   resolve_dependency(BZip2)
   if(${BZip2_SOURCE} STREQUAL "SYSTEM")
-    string(APPEND ARROW_PC_LIBS_PRIVATE " ${BZIP2_LIBRARIES}")
+    set(ARROW_BZIP2_CONFIGS "")
+    if(BZIP2_LIBRARY_RELEASE)
+      string(APPEND ARROW_PC_LIBS_PRIVATE " $<$<CONFIG:RELEASE>:${BZIP2_LIBRARY_RELEASE}>")
+      list(APPEND ARROW_BZIP2_CONFIGS "$<CONFIG:RELEASE>")
+    endif()
+    if(BZIP2_LIBRARY_DEBUG)
+      string(APPEND ARROW_PC_LIBS_PRIVATE " $<$<CONFIG:DEBUG>:${BZIP2_LIBRARY_DEBUG}>")
+      list(APPEND ARROW_BZIP2_CONFIGS "$<CONFIG:DEBUG>")
+    endif()
+    string(APPEND ARROW_PC_LIBS_PRIVATE " $<$<NOT:$<OR:")
+    if(CMAKE_VERSION VERSION_LESS "3.12")
+      string(REPLACE ";" "," ARROW_BZIP2_CONFIGS_CSV "${ARROW_BZIP2_CONFIGS}")
+    else()
+      list(JOIN ARROW_BZIP2_CONFIGS "," ARROW_BZIP2_CONFIGS_CSV)
+    endif()
+    string(APPEND ARROW_PC_LIBS_PRIVATE "${ARROW_BZIP2_CONFIGS_CSV}>>:")
+    if(BZIP2_LIBRARY)
+      string(APPEND ARROW_PC_LIBS_PRIVATE "${BZIP2_LIBRARY}")
+    else()
+      string(APPEND ARROW_PC_LIBS_PRIVATE "${BZIP2_LIBRARIES}")
+    endif()
+    string(APPEND ARROW_PC_LIBS_PRIVATE ">")
   endif()

   if(NOT TARGET BZip2::BZip2)

kou added a commit to kou/arrow that referenced this issue Jan 17, 2023
… arrow.pc

If bzip2 is installed for debug build and release build on Windows,
find_package(BZip2) returns both static library paths of them. We
should use one of them in arrow.pc.

The bzip2's official build system doesn't provide bzip2.pc but vcpkg
provides bzip2.pc. We can use it instead of detecting bzip2's static
library path by ourselves.
@kou
Copy link
Member

kou commented Jan 17, 2023

Thanks for confirming the patch!
#33712 fixes this.

kou added a commit to kou/arrow that referenced this issue Jan 17, 2023
… arrow.pc

If bzip2 is installed for debug build and release build on Windows,
find_package(BZip2) returns both static library paths of them. We
should use one of them in arrow.pc.

The bzip2's official build system doesn't provide bzip2.pc but vcpkg
provides bzip2.pc. We can use it instead of detecting bzip2's static
library path by ourselves.
@lukester1975
Copy link
Contributor Author

OK. FWIW, I added this patch to vcpkg (along with microsoft/vcpkg#23898) and tried an install of arrow. It generates:

Libs: "-L${libdir}" -larrow "${prefix}/lib/snappy.lib" "$<$<CONFIG:RELEASE>:${prefix}/lib/bz2.lib>" "$<$<CONFIG:DEBUG>:${prefix}/lib/bz2d.lib>" "$<$<NOT:$<OR:$<CONFIG:RELEASE>,$<CONFIG:DEBUG>>>:optimized" "${prefix}/lib/bz2.lib" "${prefix}/lib/bz2d.lib>"

which doesn't look great.

As a quick hack I added z_vcpkg_setup_pkgconfig_path(BASE_DIRS "${CURRENT_INSTALLED_DIR}/debug") to the portfile before the vcpkg_cmake_configure and end up with:

Libs: "-L${libdir}" -larrow
Requires: snappy libbrotlidec libbrotlienc zlib liblz4 libzstd bzip2
Cflags: "-I${includedir}" -DARROW_STATIC

which looks spot on to me.

I'm not sure where to go with this now - a new issue in the vcpkg repo? Whilst vcpkg_configure_cmake.cmake has no calls to z_vcpkg_setup_pkgconfig_path (whereas vcpkg_configure_make.cmake, vcpkg_configure_meson.cmake etc. do) I assume that's not an oversight...

kou added a commit that referenced this issue Jan 18, 2023
….pc (#33712)

### Rationale for this change

If bzip2 is installed for debug build and release build on Windows, find_package(BZip2)
returns both static library paths of them. 

### What changes are included in this PR?

We should use one of detected library paths in arrow.pc.

The bzip2's official build system doesn't provide bzip2.pc but vcpkg provides bzip2.pc. We can use it instead of detecting bzip2's static library path by ourselves.

### Are these changes tested?

Yes. We test these changes in #15139 manually.

### Are there any user-facing changes?

No.
* Closes: #15139

Authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
@kou kou added this to the 12.0.0 milestone Jan 18, 2023
@kou
Copy link
Member

kou commented Jan 18, 2023

Oh, sorry. I merged the pull request before I see your last comment.

Libs: "-L${libdir}" -larrow "${prefix}/lib/snappy.lib" "$<$<CONFIG:RELEASE>:${prefix}/lib/bz2.lib>" "$<$<CONFIG:DEBUG>:${prefix}/lib/bz2d.lib>" "$<$<NOT:$<OR:$<CONFIG:RELEASE>,$<CONFIG:DEBUG>>>:optimized" "${prefix}/lib/bz2.lib" "${prefix}/lib/bz2d.lib>"

It seems that CMake's generate expression isn't evaluated.
Implementation: https://github.com/apache/arrow/blob/master/cpp/cmake_modules/BuildUtils.cmake#L947-L951

What is the difference between #15139 (comment) and #15139 (comment) ?

@lukester1975
Copy link
Contributor Author

It seems that CMake's generate expression isn't evaluated. Implementation: https://github.com/apache/arrow/blob/master/cpp/cmake_modules/BuildUtils.cmake#L947-L951

What is the difference between #15139 (comment) and #15139 (comment) ?

Sorry, not sure I really understand the question but the first comment was regarding configuring and building arrow from the arrow source (which looks ok now), second was from vcpkg install with a simple vcpkg.json with just arrow:

{
  "$schema": "https://raw.githubusercontent.com/microsoft/vcpkg/master/scripts/vcpkg.schema.json",
  "name": "vcpkg-arrow-test",
  "version": "0.0.0",
  "dependencies": [
    {
      "name": "arrow",
      "default-features": false
    }
  ]
}

Setting PKG_CONFIG_PATH before vcpkg install didn't help so I looked in to the vcpkg source to see what might work and z_vcpkg_setup_pkgconfig_path seemed to fit the bill, though I'm sure isn't correct (aside from anything else, my hack doesn't distinguish debug/release pkgconfig dirs)...

Thanks

@kou
Copy link
Member

kou commented Jan 21, 2023

Hmm. It seems that it's related to vcpkg. Could you open an issue on vcpkg?

@lukester1975
Copy link
Contributor Author

Sure, that's what I was asking. I'll try!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants