[arrow] Update to 10.0.0 #27687

kou · 2022-11-07T03:25:43Z

Describe the pull request

What does your PR fix?

None
Which triplets are supported/not supported? Have you updated the CI baseline?

Not changed, No
Does your PR follow the maintainer guide?

Yes
If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

kou · 2022-11-07T04:02:47Z

NOTE: I'm one of Apache Arrow developers. I'll upstream the updated patches as much as possible after this is merged. We'll be able to remove most patches with Apache Arrow 11.0.0.

dg0yt · 2022-11-07T06:46:46Z

ports/arrow/portfile.cmake

@@ -42,48 +34,44 @@ if(VCPKG_TARGET_IS_WINDOWS AND NOT VCPKG_TARGET_IS_MINGW)
    list(APPEND FEATURE_OPTIONS "-DARROW_USE_NATIVE_INT128=OFF")
 endif()

+set(MALLOC_OPTIONS)


Note that this commands unsets MALLOC_OPTIONS in the current scope.
To actually start with an emtpy value, this needs to be set(MALLOC_OPTIONS "").

Thanks! I didn't know it.

dg0yt · 2022-11-07T06:47:38Z

ports/arrow/portfile.cmake

+
 if("jemalloc" IN_LIST FEATURES)
-    set(MALLOC_OPTIONS -DARROW_JEMALLOC=ON)
+    list(APPEND MALLOC_OPTIONS -DARROW_JEMALLOC=ON)
 else()
-    set(MALLOC_OPTIONS -DARROW_JEMALLOC=OFF)
+    list(APPEND MALLOC_OPTIONS -DARROW_JEMALLOC=OFF)
 endif()

 if("mimalloc" IN_LIST FEATURES)
-    set(MALLOC_OPTIONS ${MALLOC_OPTIONS} -DARROW_MIMALLOC=ON)
+    list(APPEND MALLOC_OPTIONS -DARROW_MIMALLOC=ON)
 else()
-    set(MALLOC_OPTIONS ${MALLOC_OPTIONS} -DARROW_MIMALLOC=OFF)
+    list(APPEND MALLOC_OPTIONS -DARROW_MIMALLOC=OFF)
 endif()


This shoud be integrated into vcpgk_check_features(...) if possible, or get its own invocation of that command.

Hmm can arrow actually be build with jemalloc and mimalloc? I would assume those are exclusive options and thus forbidden.

Apache Arrow can build with jemalloc and mimalloc. Users can choose their allocation backend at runtime.

I've removed them and used vcpkg_check_features() instead.

dg0yt · 2022-11-07T06:48:53Z

ports/arrow/portfile.cmake

-vcpkg_cmake_config_fixup(CONFIG_PATH lib/cmake/arrow)
+if("parquet" IN_LIST FEATURES)
+    vcpkg_cmake_config_fixup(
+        PACKAGE_NAME Parquet


You can change this to lower-case parquet. It is only the directory name in /share.

OK. vcpkg prefers to lowercase for /share/PACKAGE name, right?

Yes, and there is even a PR to add a check for this.

dg0yt · 2022-11-07T06:49:38Z

ports/arrow/portfile.cmake

+        DO_NOT_DELETE_PARENT_CONFIG_PATH
+    )
+endif()
+vcpkg_cmake_config_fixup(PACKAGE_NAME Arrow CONFIG_PATH lib/cmake/Arrow)


You can change this to lower-case arrow. It is only the directory name in /share, and it will integrate this into the port's existing folder.

dg0yt · 2022-11-07T06:53:28Z

ports/arrow/utf8proc.patch

+  find_package(unofficial-utf8proc ${find_package_args} REQUIRED)
+  if(unofficial-utf8proc_FOUND)
+    set(utf8proc_FOUND TRUE)
+    add_library(utf8proc::utf8proc ALIAS utf8proc)
+    return()
+  endif()


You can trim this to

find_package(utf8proc NAMES unofficial-utf8proc REQUIRED) add_library(utf8proc::utf8proc ALIAS utf8proc) return()

I've removed REQUIRED and added NAMES.

(same comment as the brotli changes #27687 (comment))

dg0yt · 2022-11-07T06:56:05Z

ports/arrow/utf8proc.patch

+   return()
+ endif()
+
+if(ARROW_PACAKGE_KIND STREQUAL vcpkg)


Without quotes, this will break when a variable named vcpkg is defined.

dg0yt · 2022-11-07T06:56:31Z

ports/arrow/brotli.patch

+   return()
+ endif()
+
+if(ARROW_PACAKGE_KIND STREQUAL vcpkg)


Without quotes, this will break when a variable named vcpkg is defined.

Good catch!

dg0yt · 2022-11-07T06:58:58Z

ports/arrow/brotli.patch

+  find_package(unofficial-brotli ${find_package_args} REQUIRED)
+  if(unofficial-brotli_FOUND)
+    set(Brotli_FOUND TRUE)
+    add_library(Brotli::brotlicommon ALIAS unofficial::brotli::brotlicommon)
+    add_library(Brotli::brotlienc ALIAS unofficial::brotli::brotlienc)
+    add_library(Brotli::brotlidec ALIAS unofficial::brotli::brotlidec)
+    return()
+  endif()


You can trim this to

find_package(Brotli NAMES unofficial-brotli REQUIRED) add_library(Brotli::brotlicommon ALIAS unofficial::brotli::brotlicommon) add_library(Brotli::brotlienc ALIAS unofficial::brotli::brotlienc) add_library(Brotli::brotlidec ALIAS unofficial::brotli::brotlidec) return()

Ah, I should have removed REQUIRED.
Wow, I have never used NAMES for find_package()!

Use INTERFACE instead of ALIAS.
Also return() in Find<X>.cmake?
also you need to define everything from find_package_handle_standard_args:

find_package_handle_standard_args( Brotli REQUIRED_VARS BROTLI_COMMON_LIBRARY BROTLI_ENC_LIBRARY BROTLI_DEC_LIBRARY BROTLI_INCLUDE_DIR)

In fact, REQUIRED and return() need extra care.

In Find modules, one must not use return(). It will exit the scope where find_package is called, not just the Find module. (That's the reason why we cannot use find_dependency in Find modules.)

In Find modules, one should pass on REQUIRED only if it was passed in. (In Config files, it can be delegated to find_dependency.)

Use INTERFACE instead of ALIAS.

Why? Is it for old CMake?

Use INTERFACE instead of ALIAS. Also return() in Find<X>.cmake? also you need to define everything from find_package_handle_standard_args:

Why do we need to use find_package_handle_standard_args() here? It seems that find_package(Brotli NAMES unofficial-brotli) does needed things because this file is called by find_package(Brotli). (Same package name is used.)

In Find modules, one must not use return(). It will exit the scope where find_package is called, not just the Find module.

Oh, I didn't know it.

I tried the return() behavior with Apache Arrow but it seems that it works as expected for Apache Arrow (exited only from a Find module).
In Apache Arrow, this file (FileBrotli.cmake) is called from resolve_dependency() macro https://github.com/apache/arrow/blob/apache-arrow-10.0.0/cpp/cmake_modules/ThirdpartyToolchain.cmake#L261 .
If I call return() from FindBrotli.cmake, the next line https://github.com/apache/arrow/blob/apache-arrow-10.0.0/cpp/cmake_modules/ThirdpartyToolchain.cmake#L262 is also evaluated instead of returning from resolve_dependency().

So it seems that we can use return() here.

Use INTERFACE instead of ALIAS.

Why? Is it for old CMake?

It may not be a problem here, but...
It does behave differently at least on CMake Config export: It will export the original target name (here unofficial::...), not the alias name. This may cause problems when the exported CMake config is used, depending on the completeness of find_dependency stuff.

(Here, both target names are namespaced, so CMake knows they are targets. But we have seen this with internal names like libfoo, and then CMake has no choice but must pass this blindly to the linker, which may succeed on Windows (libfoo.lib) but fail on linux (libfoo.so).)

Thanks for the explanation. I understand.
I confirmed that we can use ALIAS In Apache Arrow use case. Can we use ALIAS here? (Can we merge this pull request with the current changes?) Or should we use INTERFACE for better manner?

github-actions

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

error: checked-in files for arrow have changed but the version was not updated
version: 10.0.0
old SHA: 11ae7347b3d708803a9562ef48f1734796bb56f6
new SHA: ab325846893f2dc3382b7b403bf195645ab93a39
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

JonLiu1993 · 2022-11-08T06:35:28Z

Feature plasma,s3,parquet,orc,json,jemalloc,flight,filesystem,example,dataset,csv tested successfully in the x64-linux triplet

kou · 2022-11-08T07:59:28Z

Thanks for confirming these features!

… vcpkg vcpkg provides CMake packages for Brotli and utf8proc with "unofficial-" prefix. So we need to care about them separately. See also: microsoft/vcpkg#27687

spectre-ns · 2022-11-11T00:15:33Z

I'm awaiting this for #26501

JavierMatosD · 2022-11-11T22:36:20Z

ports/arrow/brotli.patch

+  if(Brotli_FIND_QUIETLY)
+    list(APPEND find_package_args QUIET)
+  endif()


Next time, would you mind making this REQUIRED? :)
Other than that, this looks good to me!
Thank you!

This spot is in a find module forwarding to vcpkg's unofficial config. IMO REQUIRED is only possible if the original call was REQUIRED and the absence of vcpkg config shall raise an immediate error despite other search options.

OK. I'll add the following:

if(Brotli_FIND_REQUIRED) list(APPEND find_package_args REQUIRED) endif()

… vcpkg (#14609) vcpkg provides CMake packages for Brotli and utf8proc with "unofficial-" prefix. So we need to care about them separately. See also: microsoft/vcpkg#27687 Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

kou added 2 commits November 7, 2022 12:11

[arrow] Update Apache Arrow to 10.0.0

e030116

Update versions

3d839c7

github-actions bot previously approved these changes Nov 7, 2022

View reviewed changes

kou marked this pull request as ready for review November 7, 2022 04:01

Cheney-W assigned JonLiu1993 Nov 7, 2022

JonLiu1993 added the category:port-update The issue is with a library, which is requesting update new revision label Nov 7, 2022

JonLiu1993 previously approved these changes Nov 7, 2022

View reviewed changes

dg0yt reviewed Nov 7, 2022

View reviewed changes

JonLiu1993 added the requires:author-response label Nov 7, 2022

kou added 3 commits November 7, 2022 17:42

Simplify and make more robust

7cf37c3

Use vcpkg_check_features()

94574f2

Use lower case for CMake package name

d9f7db8

kou dismissed stale reviews from JonLiu1993 and github-actions[bot] via d9f7db8 November 7, 2022 08:43

github-actions bot requested changes Nov 7, 2022

View reviewed changes

JonLiu1993 removed the requires:author-response label Nov 7, 2022

Update version

32be039

github-actions bot approved these changes Nov 7, 2022

View reviewed changes

JonLiu1993 added the requires:author-response label Nov 7, 2022

bernhardmgruber mentioned this pull request Nov 7, 2022

[xsimd] Update to version 9.0.1 #26501

Merged

kou mentioned this pull request Nov 8, 2022

ARROW-18287: [C++][CMake] Add support for Brotli/utf8proc provided by vcpkg apache/arrow#14609

Merged

JonLiu1993 removed the requires:author-response label Nov 9, 2022

JonLiu1993 approved these changes Nov 10, 2022

View reviewed changes

JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Nov 11, 2022

JavierMatosD approved these changes Nov 11, 2022

View reviewed changes

JavierMatosD merged commit 59b58fc into microsoft:master Nov 11, 2022

kou deleted the arrow-10.0.0 branch November 12, 2022 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[arrow] Update to 10.0.0 #27687

[arrow] Update to 10.0.0 #27687

kou commented Nov 7, 2022

kou commented Nov 7, 2022

dg0yt Nov 7, 2022

kou Nov 7, 2022

dg0yt Nov 7, 2022

Neumann-A Nov 7, 2022

kou Nov 7, 2022

dg0yt Nov 7, 2022

kou Nov 7, 2022

dg0yt Nov 7, 2022

kou Nov 8, 2022

dg0yt Nov 7, 2022

kou Nov 7, 2022

dg0yt Nov 7, 2022

kou Nov 7, 2022

Neumann-A Nov 7, 2022

dg0yt Nov 7, 2022

kou Nov 7, 2022

dg0yt Nov 7, 2022

kou Nov 7, 2022

dg0yt Nov 7, 2022

kou Nov 7, 2022

Neumann-A Nov 7, 2022

dg0yt Nov 7, 2022

kou Nov 8, 2022

dg0yt Nov 8, 2022

kou Nov 8, 2022

github-actions bot left a comment

JonLiu1993 commented Nov 8, 2022

kou commented Nov 8, 2022

spectre-ns commented Nov 11, 2022

JavierMatosD Nov 11, 2022

dg0yt Nov 12, 2022

kou Nov 12, 2022

[arrow] Update to 10.0.0 #27687

[arrow] Update to 10.0.0 #27687

Conversation

kou commented Nov 7, 2022

What does your PR fix?

Which triplets are supported/not supported? Have you updated the CI baseline?

Does your PR follow the maintainer guide?

If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

kou commented Nov 7, 2022

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

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

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

github-actions bot left a comment

Choose a reason for hiding this comment

JonLiu1993 commented Nov 8, 2022

kou commented Nov 8, 2022

spectre-ns commented Nov 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

If you have added/updated a port: Have you run `./vcpkg x-add-version --all` and committed the result?