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

[Release][Packaging] Address notes in vcpkg x64-windows-static build #45297

Open
3 tasks
Tracked by #45303
amoeba opened this issue Jan 17, 2025 · 6 comments
Open
3 tasks
Tracked by #45303

[Release][Packaging] Address notes in vcpkg x64-windows-static build #45297

amoeba opened this issue Jan 17, 2025 · 6 comments

Comments

@amoeba
Copy link
Member

amoeba commented Jan 17, 2025

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

During the vcpkg port update for Arrow 19.0.0, an issue with the x64-windows-static build was caught. From microsoft/vcpkg#43303 (comment),

Installing 98/98 arrow[acero,compute,core,csv,dataset,example,filesystem,flight,flightsql,gcs,json,mimalloc,orc,parquet,s3]:[email protected]...
Building arrow[acero,compute,core,csv,dataset,example,filesystem,flight,flightsql,gcs,json,mimalloc,orc,parquet,s3]:[email protected]...

...

-- Performing post-build validation
E:\arrow-19.0.0\ports\arrow\portfile.cmake: warning: binaries built by this port link with C RunTimes ("CRTs") inconsistent with those requested by the triplet and deployment structure. If the triplet is intended to only use the release CRT, you should add set(VCPKG_POLICY_ONLY_RELEASE_CRT enabled) to the triplet .cmake file. To suppress this check entirely, add set(VCPKG_POLICY_SKIP_CRT_LINKAGE_CHECK enabled) to the triplet .cmake if this is triplet-wide, or to portfile.cmake if this is specific to the port. You can inspect the binaries with: dumpbin.exe /directives mylibfile.lib
E:\arrow-19.0.0\packages\arrow_x64-windows-static: note: the binaries are relative to ${CURRENT_PACKAGES_DIR} here
note: The following binaries should link with only: Static Debug (/MTd)
note: debug/lib/arrow_bundled_dependencies.lib links with: Dynamic Debug (/MDd)
note: The following binaries should link with only: Static Release (/MT)
note: lib/arrow_bundled_dependencies.lib links with: Dynamic Release (/MD)
E:\arrow-19.0.0\ports\arrow\portfile.cmake: warning: Found 1 post-build check problem(s). These are usually caused by bugs in portfile.cmake or the upstream build system. Please correct these before submitting this port to the curated registry.

...

We'll have to address these to update vcpkg for 19.0.0.

cc @kou @assignUser @raulcd

Tasks:

  • Add a temporary patch to our vcpkg port
  • Refactor this part of the build to FetchContent in Arrow >=20.0.0
  • Remove the patch from the vcpkg port once the once the FetchContent refactor is done

Component(s)

Release, Packaging

@amoeba
Copy link
Member Author

amoeba commented Jan 18, 2025

Adding this patch to our vcpkg port made the post-build validation warning go away,

diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake
index abfe6d274f..8bacfe89af 100644
--- a/cpp/cmake_modules/ThirdpartyToolchain.cmake
+++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake
@@ -886,9 +886,17 @@ foreach(CONFIG DEBUG MINSIZEREL RELEASE RELWITHDEBINFO)
   set(EP_CXX_FLAGS_${CONFIG} "${CMAKE_CXX_FLAGS_${CONFIG}}")
   set(EP_C_FLAGS_${CONFIG} "${CMAKE_C_FLAGS_${CONFIG}}")
   if(CONFIG STREQUAL DEBUG)
-    set(EP_MSVC_RUNTIME_LIBRARY MultiThreadedDebugDLL)
+    if(BUILD_SHARED_LIBS)
+      set(EP_MSVC_RUNTIME_LIBRARY MultiThreadedDebugDLL)
+     else()
+      set(EP_MSVC_RUNTIME_LIBRARY MultiThreadedDebug)
+    endif()
   else()
-    set(EP_MSVC_RUNTIME_LIBRARY MultiThreadedDLL)
+    if(BUILD_SHARED_LIBS)
+      set(EP_MSVC_RUNTIME_LIBRARY MultiThreadedDLL)
+    else()
+      set(EP_MSVC_RUNTIME_LIBRARY MultiThreaded)
+    endif()
   endif()
   string(APPEND EP_CXX_FLAGS_${CONFIG}
          " ${CMAKE_CXX_COMPILE_OPTIONS_MSVC_RUNTIME_LIBRARY_${EP_MSVC_RUNTIME_LIBRARY}}")

I'm going to test other triplets now too. I'm not sure if the patch is perfect yet.

@amoeba
Copy link
Member Author

amoeba commented Jan 18, 2025

Two other thoughts:

  1. Do we need/want to merge this into Arrow?
  2. We could make the above patch more specific, by testing if(VCPKG_CRT_LINKAGE STREQUAL "static") directly rather than BUILD_SHARED_LIBS.

@kou
Copy link
Member

kou commented Jan 20, 2025

It'll work but migrating to FetchContent from ExternalProject is better.
How about using the change in vcpkg for now and migrating to FetchCotent before Apache Arrow C++ 20.0.0?

@amoeba
Copy link
Member Author

amoeba commented Jan 20, 2025

Thanks for taking a look @kou. Using this patch for Arrow 19.0.0 sounds good.

Should we make a tracking issue for the FetchContent migration? I think there are now at least two related issues (this and #45195).

For now I'll integrate this patch into the vcpkg port and update the issue body here.

@kou
Copy link
Member

kou commented Jan 20, 2025

Should we make a tracking issue for the FetchContent migration? I think there are now at least two related issues (this and #45195).

Yes. It'll be helpful.

FYI: One more related issue: #45063

@amoeba
Copy link
Member Author

amoeba commented Jan 20, 2025

Thanks! Done: #45303. I'm not sure I understand all that past discussion or the task as well as others might so please feel free to add anything.

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

No branches or pull requests

2 participants