[C++][Packaging] Figure out why Arrow C++ fails to build under vcpkg on Android

[amoeba]

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

The PR to update vcpk's version of Arrow C++ for 18.1.0 is failing but only for Android, see microsoft/vcpkg#42357 (comment). The relevant log output is:

FAILED: src/arrow/CMakeFiles/arrow_vendored.dir/vendored/datetime.cpp.o 
/android-ndk-r27c/toolchains/llvm/prebuilt/linux-x86_64/bin/clang++ --target=aarch64-none-linux-android21 --sysroot=/android-ndk-r27c/toolchains/llvm/prebuilt/linux-x86_64/sysroot -DARROW_HAVE_NEON -DARROW_WITH_TIMING_TESTS -DURI_STATIC_BUILD -I/mnt/vcpkg-ci/b/arrow/arm64-android-dbg/src -I/mnt/vcpkg-ci/b/arrow/src/e-arrow-18-d474976a0f.clean/cpp/src -I/mnt/vcpkg-ci/b/arrow/src/e-arrow-18-d474976a0f.clean/cpp/src/generated -g -DANDROID -fdata-sections -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security -frtti -fexceptions  -fPIC   -Qunused-arguments -fcolor-diagnostics  -Wall -Wno-unknown-warning-option -Wno-pass-failed -march=armv8-a  -fno-limit-debug-info    -O0 -ggdb  -std=c++17 -fPIC -MD -MT src/arrow/CMakeFiles/arrow_vendored.dir/vendored/datetime.cpp.o -MF src/arrow/CMakeFiles/arrow_vendored.dir/vendored/datetime.cpp.o.d -o src/arrow/CMakeFiles/arrow_vendored.dir/vendored/datetime.cpp.o -c /mnt/vcpkg-ci/b/arrow/src/e-arrow-18-d474976a0f.clean/cpp/src/arrow/vendored/datetime.cpp
In file included from /mnt/vcpkg-ci/b/arrow/src/e-arrow-18-d474976a0f.clean/cpp/src/arrow/vendored/datetime.cpp:19:
/mnt/vcpkg-ci/b/arrow/src/e-arrow-18-d474976a0f.clean/cpp/src/arrow/vendored/datetime/tz.cpp:608:55: error: use of undeclared identifier 'init_tzdb'; did you mean 'get_tzdb'?
  608 |     tzdb_list::undocumented_helper::push_front(tz_db, init_tzdb().release());
      |                                                       ^~~~~~~~~
      |                                                       get_tzdb
/mnt/vcpkg-ci/b/arrow/src/e-arrow-18-d474976a0f.clean/cpp/src/arrow/vendored/datetime/tz.h:1221:22: note: 'get_tzdb' declared here
 1221 | DATE_API const tzdb& get_tzdb();
      |                      ^
In file included from /mnt/vcpkg-ci/b/arrow/src/e-arrow-18-d474976a0f.clean/cpp/src/arrow/vendored/datetime.cpp:19:
/mnt/vcpkg-ci/b/arrow/src/e-arrow-18-d474976a0f.clean/cpp/src/arrow/vendored/datetime/tz.cpp:608:67: error: no member named 'release' in 'arrow_vendored::date::tzdb'
  608 |     tzdb_list::undocumented_helper::push_front(tz_db, init_tzdb().release());
      |                                                       ~~~~~~~~~~~ ^
/mnt/vcpkg-ci/b/arrow/src/e-arrow-18-d474976a0f.clean/cpp/src/arrow/vendored/datetime/tz.cpp:3039:18: error: 'parse_from_android_tzdata' is a private member of 'arrow_vendored::date::time_zone'
 3039 |         timezone.parse_from_android_tzdata(in, hdr.data_offset + index_entry.start);
      |                  ^
/mnt/vcpkg-ci/b/arrow/src/e-arrow-18-d474976a0f.clean/cpp/src/arrow/vendored/datetime/tz.h:861:10: note: declared private here
  861 |     void parse_from_android_tzdata(std::ifstream& inf, const std::size_t off);
      |          ^
3 errors generated.

I'm guessing this has something to do with 67850be which was cherry-picked onto 18.1.0.

Component(s)

C++, Packaging

[amoeba]

@assignUser @kou @raulcd can any of you take a look? If the fix needs to be done on the Arrow side, would it be acceptable to just not publish 18.1.0 to vcpkg, fix this for 19.0.0, and update vcpkg with 19.0.0?

[dg0yt]

Note that vcpkg CI builds Android with API level 21. At that level, C runtime is very incomplete. It might be enough to document the minimum requirement.

[kou]

Could you try this?

diff --git a/cpp/src/arrow/vendored/datetime/tz.h b/cpp/src/arrow/vendored/datetime/tz.h
index 61ab3df106..d456d6765f 100644
--- a/cpp/src/arrow/vendored/datetime/tz.h
+++ b/cpp/src/arrow/vendored/datetime/tz.h
@@ -858,7 +858,9 @@ private:
     load_data(std::istream& inf, std::int32_t tzh_leapcnt, std::int32_t tzh_timecnt,
                                  std::int32_t tzh_typecnt, std::int32_t tzh_charcnt);
 # if defined(ANDROID) || defined(__ANDROID__)
+public:
     void parse_from_android_tzdata(std::ifstream& inf, const std::size_t off);
+private:
 # endif // defined(ANDROID) || defined(__ANDROID__)
 #else  // !USE_OS_TZDB
     DATE_API sys_info   get_info_impl(sys_seconds tp, int tz_int) const;
diff --git a/cpp/src/arrow/vendored/datetime/visibility.h b/cpp/src/arrow/vendored/datetime/visibility.h
index 780c00d70b..a9514edba7 100644
--- a/cpp/src/arrow/vendored/datetime/visibility.h
+++ b/cpp/src/arrow/vendored/datetime/visibility.h
@@ -21,6 +21,10 @@
 #  define USE_OS_TZDB 1
 #endif
 
+#if defined(ANDROID) || defined(__ANDROID__)
+#  define BUILD_TZ_LIB
+#endif
+
 #if defined(ARROW_STATIC)
 // intentially empty
 #elif defined(ARROW_EXPORTING)

If this doesn't work, we need to skip 18.1.0 on vcpkg and report it to upstream https://github.com/HowardHinnant/date .
It seems that upstream doesn't work with #define USE_OS_TZDB 1 on Android.

[dg0yt]

https://github.com/HowardHinnant/date

And this library has a port in vcpkg. (May not solve the android issue, but must be devendored at least if not encapsulating symbol names.)

[kou]

Thanks.
If we require C++20, we can drop https://github.com/HowardHinnant/date because C++20 has the feature provided by the library...

[amoeba]

Thanks @kou, I added the patch to the vcpkg port PR, see microsoft/vcpkg#42357. Note I also renamed the patches to match vcpkg convention. I'll keep an eye on CI.

[kou]

Thanks.
It seems that the patch fixed the build errors.

We need to add a CI for Android before we apply this patch.
See also: #43390

[assignUser]

We need to add a CI for Android before we apply this patch.

Do we really? This would mean adding android to our kind-of-officially supported list of configurations that we test (we are missing an offical one still...). I am not sure that is really justified. After all this was not a user report but rather vcpkg adding CI for a new triplet (this happend last year apparently). I think it's fine to keep the patch in VCPKG for now?

We should of course have some form of offical support policy that would make this decision easier :D