Adarsh-Me commented on code in PR #51315: URL: https://github.com/apache/arrow/pull/51315#discussion_r4164613508
########## cpp/cmake_modules/CheckStdChrono.cmake: ########## @@ -0,0 +1,93 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Resolve the ARROW_USE_STD_CHRONO option (AUTO, ON or OFF) into a boolean. +# +# GH-51267 tracks removing the vendored datetime fallback once all supported +# toolchains provide working C++20 chrono timezone support. Until then: +# - AUTO keeps the historical platform default: std::chrono is used on Windows +# toolchains whose standard library provides working C++20 chrono timezone +# support, and the vendored datetime fallback is used everywhere else. +# - ON opts into std::chrono unconditionally, failing the configure step when +# the toolchain does not provide working C++20 chrono timezone support. +# - OFF always uses the vendored datetime fallback. +# +# The resolved value is consumed via arrow/util/config.h (ARROW_USE_STD_CHRONO) +# by arrow/util/chrono_internal.h and to decide whether the vendored datetime +# implementation is built. + +include(CheckCXXSourceCompiles) + +# When Arrow is consumed as a CMake subproject, ARROW_USE_STD_CHRONO is not +# defined; skip detection and let arrow/util/chrono_internal.h fall back to its +# default backend selection (vendored datetime fallback). +if(DEFINED ARROW_USE_STD_CHRONO) +if(NOT "${ARROW_USE_STD_CHRONO}" MATCHES "^(AUTO|ON|OFF)$") + message(FATAL_ERROR "ARROW_USE_STD_CHRONO must be one of AUTO, ON or OFF " + "(got \"${ARROW_USE_STD_CHRONO}\")") +endif() + +set(_ARROW_STD_CHRONO_TEST_SOURCE + " +#include <chrono> +#if !defined(__cpp_lib_chrono) || __cpp_lib_chrono < 201907L +# error \"C++20 chrono timezone support (__cpp_lib_chrono >= 201907L) is unavailable\" +#endif +int main() { return 0; } Review Comment: Fixed and verified against this head: the probe now compiles and links the APIs actually used — <format>, std::chrono::locate_zone, std::vformat_to with make_format_args — and additionally requires __cpp_lib_format (cpp/cmake_modules/CheckStdChrono.cmake:42-71). The same source is mirrored in cpp/meson.build:120-153, so Meson no longer trusts the feature-test macro alone either. ########## cpp/src/arrow/config.cc: ########## @@ -91,7 +103,11 @@ RuntimeInfo GetRuntimeInfo() { Status Initialize(const GlobalOptions& options) noexcept { ARROW_SUPPRESS_DEPRECATION_WARNING if (options.timezone_db_path.has_value()) { -#if !USE_OS_TZDB +#if defined(ARROW_USE_STD_CHRONO) && ARROW_USE_STD_CHRONO + return Status::Invalid( + "Arrow was built with C++20 std::chrono and uses the OS timezone database, " + "so a downloaded database cannot be provided at runtime."); Review Comment: Addressed: Initialize() no longer keys on ARROW_USE_STD_CHRONO alone but on ARROW_HAVE_VENDORED_DATETIME (cpp/src/arrow/config.cc:112-133). std::chrono plus Gandiva still bundles the vendored timezone code and takes the set_install/reload_tzdb branch; only pure std::chrono builds, which carry no vendored code, reject timezone_db_path. ########## cpp/src/arrow/config.cc: ########## @@ -77,11 +83,17 @@ RuntimeInfo GetRuntimeInfo() { MakeSimdLevelString([&](int64_t flags) { return cpu_info->IsSupported(flags); }); info.detected_simd_level = MakeSimdLevelString([&](int64_t flags) { return cpu_info->IsDetected(flags); }); +#if defined(ARROW_USE_STD_CHRONO) && ARROW_USE_STD_CHRONO + // GH-51267: std::chrono builds always use the OS timezone database. + info.using_os_timezone_db = true; + info.timezone_db_path = std::optional<std::string>(); +#else Review Comment: Addressed: cpp/src/arrow/config.cc now includes arrow/util/chrono_internal.h (lines 22-26), so the CMake-subproject case (ARROW_DEFINE_OPTIONS=OFF, macro undefined) resolves through the same header-level backend predicate that RuntimeInfo() and Initialize() use, instead of drifting to the vendored reading. ########## r/R/arrow-package.R: ########## @@ -180,6 +180,13 @@ s3_finalizer <- new.env(parent = emptyenv()) configure_tzdb <- function() { if (requireNamespace("tzdb", quietly = TRUE)) { + if (runtime_info()[[3]] == "true") { + # GH-51267: builds reading the OS timezone database (C++20 std::chrono, + # or the vendored library built against the OS tzdata) cannot use a + # downloaded database, and their timezones already work — skip the + # vendored path instead of surfacing a false startup failure. + return(invisible()) + } Review Comment: Addressed: configure_tzdb() returns before touching the optional package when runtime_info() reports the OS database (r/R/arrow-package.R:181-188), so an OS-backed build without tzdb installed no longer reaches the warning branch. The check now precedes requireNamespace as requested. ########## r/R/arrow-package.R: ########## @@ -180,6 +180,13 @@ s3_finalizer <- new.env(parent = emptyenv()) configure_tzdb <- function() { if (requireNamespace("tzdb", quietly = TRUE)) { + if (runtime_info()[[3]] == "true") { + # GH-51267: builds reading the OS timezone database (C++20 std::chrono, + # or the vendored library built against the OS tzdata) cannot use a + # downloaded database, and their timezones already work — skip the + # vendored path instead of surfacing a false startup failure. + return(invisible()) + } Review Comment: Same fix as the earlier thread on this file: the backend check at r/R/arrow-package.R:181-188 now precedes requireNamespace("tzdb"), so OS-backed builds return early and never emit that startup message. Resolving as a duplicate of that thread. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
