From 63bb903b7ddc7730beaa3d092dc5808632cd4b08 Mon Sep 17 00:00:00 2001 From: David Li Date: Thu, 9 Jan 2025 01:14:05 -0500 Subject: [PATCH] test(c): don't use sketchy cast to test backwards compatibility (#2425) - Backport nanoarrow patch to satisfy newer Clang - Add test using GCC 15 - Update tests using sketchy casts to satisfy these compilers - Refactor the clang/gcc Docker jobs Fixes #2424. --------- Co-authored-by: Sutou Kouhei --- .github/workflows/nightly-verify.yml | 5 +++ c/driver/flightsql/sqlite_flightsql_test.cc | 16 ++++---- c/driver/postgresql/postgresql_test.cc | 45 +++++++++++---------- c/validation/adbc_validation_statement.cc | 24 ++++++----- c/vendor/nanoarrow/nanoarrow.hpp | 7 ++++ ci/docker/cpp-clang-latest.dockerfile | 12 +++--- ci/docker/cpp-gcc-latest.dockerfile | 34 ++++++++++++++++ docker-compose.yml | 15 ++++++- 8 files changed, 112 insertions(+), 46 deletions(-) create mode 100644 ci/docker/cpp-gcc-latest.dockerfile diff --git a/.github/workflows/nightly-verify.yml b/.github/workflows/nightly-verify.yml index 3d27ef1646..2d4652585d 100644 --- a/.github/workflows/nightly-verify.yml +++ b/.github/workflows/nightly-verify.yml @@ -190,6 +190,11 @@ jobs: pushd arrow-adbc docker compose run --rm cpp-clang-latest + - name: cpp-gcc-latest + run: | + pushd arrow-adbc + docker compose run --rm cpp-gcc-latest + - name: python-debug run: | pushd arrow-adbc diff --git a/c/driver/flightsql/sqlite_flightsql_test.cc b/c/driver/flightsql/sqlite_flightsql_test.cc index 4797d58e77..752ce8154f 100644 --- a/c/driver/flightsql/sqlite_flightsql_test.cc +++ b/c/driver/flightsql/sqlite_flightsql_test.cc @@ -235,18 +235,20 @@ TEST_F(SqliteFlightSqlTest, TestGarbageInput) { ASSERT_THAT(AdbcDatabaseRelease(&database, &error), IsOkStatus(&error)); } +int Canary(const struct AdbcError*) { return 0; } + TEST_F(SqliteFlightSqlTest, AdbcDriverBackwardsCompatibility) { - // XXX: sketchy cast - auto* driver = static_cast(malloc(ADBC_DRIVER_1_0_0_SIZE)); - std::memset(driver, 0, ADBC_DRIVER_1_0_0_SIZE); + struct AdbcDriver driver; + std::memset(&driver, 0, ADBC_DRIVER_1_1_0_SIZE); + driver.ErrorGetDetailCount = Canary; - ASSERT_THAT(::FlightSQLDriverInit(ADBC_VERSION_1_0_0, driver, &error), + ASSERT_THAT(::FlightSQLDriverInit(ADBC_VERSION_1_0_0, &driver, &error), IsOkStatus(&error)); - ASSERT_THAT(::FlightSQLDriverInit(424242, driver, &error), - adbc_validation::IsStatus(ADBC_STATUS_NOT_IMPLEMENTED, &error)); + ASSERT_EQ(Canary, driver.ErrorGetDetailCount); - free(driver); + ASSERT_THAT(::FlightSQLDriverInit(424242, &driver, &error), + adbc_validation::IsStatus(ADBC_STATUS_NOT_IMPLEMENTED, &error)); } class SqliteFlightSqlConnectionTest : public ::testing::Test, diff --git a/c/driver/postgresql/postgresql_test.cc b/c/driver/postgresql/postgresql_test.cc index be32bd893b..b76ba1cf7d 100644 --- a/c/driver/postgresql/postgresql_test.cc +++ b/c/driver/postgresql/postgresql_test.cc @@ -223,18 +223,20 @@ class PostgresDatabaseTest : public ::testing::Test, }; ADBCV_TEST_DATABASE(PostgresDatabaseTest) +int Canary(const struct AdbcError*) { return 0; } + TEST_F(PostgresDatabaseTest, AdbcDriverBackwardsCompatibility) { - // XXX: sketchy cast - auto* driver = static_cast(malloc(ADBC_DRIVER_1_0_0_SIZE)); - std::memset(driver, 0, ADBC_DRIVER_1_0_0_SIZE); + struct AdbcDriver driver; + std::memset(&driver, 0, ADBC_DRIVER_1_1_0_SIZE); + driver.ErrorGetDetailCount = Canary; - ASSERT_THAT(::PostgresqlDriverInit(ADBC_VERSION_1_0_0, driver, &error), + ASSERT_THAT(::PostgresqlDriverInit(ADBC_VERSION_1_0_0, &driver, &error), IsOkStatus(&error)); - ASSERT_THAT(::PostgresqlDriverInit(424242, driver, &error), - IsStatus(ADBC_STATUS_NOT_IMPLEMENTED, &error)); + ASSERT_EQ(Canary, driver.ErrorGetDetailCount); - free(driver); + ASSERT_THAT(::PostgresqlDriverInit(424242, &driver, &error), + IsStatus(ADBC_STATUS_NOT_IMPLEMENTED, &error)); } class PostgresConnectionTest : public ::testing::Test, @@ -1552,24 +1554,25 @@ TEST_F(PostgresStatementTest, BatchSizeHint) { // Test that an ADBC 1.0.0-sized error still works TEST_F(PostgresStatementTest, AdbcErrorBackwardsCompatibility) { - // XXX: sketchy cast - auto* error = static_cast(malloc(ADBC_ERROR_1_0_0_SIZE)); - std::memset(error, 0, ADBC_ERROR_1_0_0_SIZE); + struct AdbcError error; + std::memset(&error, 0, ADBC_ERROR_1_1_0_SIZE); + struct AdbcDriver canary; + error.private_data = &canary; + error.private_driver = &canary; - ASSERT_THAT(AdbcStatementNew(&connection, &statement, error), IsOkStatus(error)); + ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error)); ASSERT_THAT( - AdbcStatementSetSqlQuery(&statement, "SELECT * FROM thistabledoesnotexist", error), - IsOkStatus(error)); + AdbcStatementSetSqlQuery(&statement, "SELECT * FROM thistabledoesnotexist", &error), + IsOkStatus(&error)); adbc_validation::StreamReader reader; ASSERT_THAT(AdbcStatementExecuteQuery(&statement, &reader.stream.value, - &reader.rows_affected, error), - IsStatus(ADBC_STATUS_NOT_FOUND, error)); - - ASSERT_EQ("42P01", std::string_view(error->sqlstate, 5)); - ASSERT_EQ(0, AdbcErrorGetDetailCount(error)); - - error->release(error); - free(error); + &reader.rows_affected, &error), + IsStatus(ADBC_STATUS_NOT_FOUND, &error)); + ASSERT_EQ("42P01", std::string_view(error.sqlstate, 5)); + ASSERT_EQ(0, AdbcErrorGetDetailCount(&error)); + ASSERT_EQ(&canary, error.private_data); + ASSERT_EQ(&canary, error.private_driver); + error.release(&error); } TEST_F(PostgresStatementTest, Cancel) { diff --git a/c/validation/adbc_validation_statement.cc b/c/validation/adbc_validation_statement.cc index cd388623ba..8c6dd94846 100644 --- a/c/validation/adbc_validation_statement.cc +++ b/c/validation/adbc_validation_statement.cc @@ -2817,21 +2817,23 @@ struct ADBC_EXPORT AdbcError100 { // Test that an ADBC 1.0.0-sized error still works void StatementTest::TestErrorCompatibility() { static_assert(sizeof(AdbcError100) == ADBC_ERROR_1_0_0_SIZE, "Wrong size"); - // XXX: sketchy cast - auto* error = reinterpret_cast(malloc(ADBC_ERROR_1_0_0_SIZE)); - std::memset(error, 0, ADBC_ERROR_1_0_0_SIZE); + struct AdbcError error; + std::memset(&error, 0, ADBC_ERROR_1_1_0_SIZE); + struct AdbcDriver canary; + error.private_data = &canary; + error.private_driver = &canary; - ASSERT_THAT(AdbcStatementNew(&connection, &statement, error), IsOkStatus(error)); + ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error)); ASSERT_THAT( - AdbcStatementSetSqlQuery(&statement, "SELECT * FROM thistabledoesnotexist", error), - IsOkStatus(error)); + AdbcStatementSetSqlQuery(&statement, "SELECT * FROM thistabledoesnotexist", &error), + IsOkStatus(&error)); adbc_validation::StreamReader reader; ASSERT_THAT(AdbcStatementExecuteQuery(&statement, &reader.stream.value, - &reader.rows_affected, error), - ::testing::Not(IsOkStatus(error))); - auto* old_error = reinterpret_cast(error); - old_error->release(old_error); - free(error); + &reader.rows_affected, &error), + ::testing::Not(IsOkStatus(&error))); + ASSERT_EQ(&canary, error.private_data); + ASSERT_EQ(&canary, error.private_driver); + error.release(&error); } void StatementTest::TestResultInvalidation() { diff --git a/c/vendor/nanoarrow/nanoarrow.hpp b/c/vendor/nanoarrow/nanoarrow.hpp index 16c2e55b9f..f2eade3d8e 100644 --- a/c/vendor/nanoarrow/nanoarrow.hpp +++ b/c/vendor/nanoarrow/nanoarrow.hpp @@ -92,9 +92,16 @@ namespace literals { /// @{ /// \brief User literal operator allowing ArrowStringView construction like "str"_asv +#if !defined(__clang__) && (defined(__GNUC__) && __GNUC__ < 6) inline ArrowStringView operator"" _asv(const char* data, std::size_t size_bytes) { return {data, static_cast(size_bytes)}; } +#else +inline ArrowStringView operator""_asv(const char* data, std::size_t size_bytes) { + return {data, static_cast(size_bytes)}; +} +#endif +// N.B. older GCC requires the space above, newer Clang forbids the space // @} diff --git a/ci/docker/cpp-clang-latest.dockerfile b/ci/docker/cpp-clang-latest.dockerfile index ff6e161e37..a1b76a552e 100644 --- a/ci/docker/cpp-clang-latest.dockerfile +++ b/ci/docker/cpp-clang-latest.dockerfile @@ -15,9 +15,8 @@ # specific language governing permissions and limitations # under the License. -ARG VCPKG - FROM debian:12 +ARG GO RUN export DEBIAN_FRONTEND=noninteractive && \ apt-get update -y && \ @@ -34,6 +33,9 @@ RUN export DEBIAN_FRONTEND=noninteractive && \ RUN export DEBIAN_FRONTEND=noninteractive && \ apt-get install -y cmake git libpq-dev libsqlite3-dev pkg-config -RUN curl -L -o go.tar.gz https://go.dev/dl/go1.22.5.linux-amd64.tar.gz && \ - tar -C /opt -xvf go.tar.gz && \ - echo 'export PATH=$PATH:/opt/go/bin' | tee -a ~/.bashrc +RUN curl -L -o go.tar.gz https://go.dev/dl/go${GO}.linux-amd64.tar.gz && \ + tar -C /opt -xvf go.tar.gz + +ENV PATH=/opt/go/bin:$PATH \ + CC=/usr/bin/clang \ + CXX=/usr/bin/clang++ diff --git a/ci/docker/cpp-gcc-latest.dockerfile b/ci/docker/cpp-gcc-latest.dockerfile new file mode 100644 index 0000000000..38a7781ef9 --- /dev/null +++ b/ci/docker/cpp-gcc-latest.dockerfile @@ -0,0 +1,34 @@ +# 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. + +FROM amd64/debian:experimental +ARG GCC +ARG GO + +ENV DEBIAN_FRONTEND noninteractive + +RUN apt-get update -y && \ + apt-get install -y -q cmake curl git gnupg libpq-dev libsqlite3-dev pkg-config && \ + apt-get install -y -q -t experimental g++-${GCC} gcc-${GCC} && \ + apt-get clean + +RUN curl -L -o go.tar.gz https://go.dev/dl/go${GO}.linux-amd64.tar.gz && \ + tar -C /opt -xvf go.tar.gz + +ENV PATH=/opt/go/bin:$PATH \ + CC=/usr/bin/gcc-${GCC} \ + CXX=/usr/bin/g++-${GCC} diff --git a/docker-compose.yml b/docker-compose.yml index bf961cdb84..1872a2cfc1 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -38,10 +38,21 @@ services: context: . dockerfile: ci/docker/cpp-clang-latest.dockerfile args: - VCPKG: ${VCPKG} + GO: ${GO} + volumes: + - .:/adbc:delegated + command: "bash -c 'git config --global --add safe.directory /adbc && /adbc/ci/scripts/cpp_build.sh /adbc /adbc/build/clang-latest && env BUILD_ALL=0 BUILD_DRIVER_MANAGER=1 BUILD_DRIVER_SQLITE=1 /adbc/ci/scripts/cpp_test.sh /adbc/build/clang-latest'" + + cpp-gcc-latest: + build: + context: . + dockerfile: ci/docker/cpp-gcc-latest.dockerfile + args: + GCC: 15 + GO: ${GO} volumes: - .:/adbc:delegated - command: "bash -c 'export PATH=$PATH:/opt/go/bin CC=$(which clang) CXX=$(which clang++) && git config --global --add safe.directory /adbc && /adbc/ci/scripts/cpp_build.sh /adbc /adbc/build && env BUILD_ALL=0 BUILD_DRIVER_MANAGER=1 BUILD_DRIVER_SQLITE=1 /adbc/ci/scripts/cpp_test.sh /adbc/build'" + command: "bash -c 'git config --global --add safe.directory /adbc && /adbc/ci/scripts/cpp_build.sh /adbc /adbc/build/gcc-latest && env BUILD_ALL=0 BUILD_DRIVER_MANAGER=1 BUILD_DRIVER_SQLITE=1 /adbc/ci/scripts/cpp_test.sh /adbc/build/gcc-latest'" ############################ Documentation ###################################