Copilot commented on code in PR #2218: URL: https://github.com/apache/nifi-minifi-cpp/pull/2218#discussion_r3672665734
########## thirdparty/soci/all/patches/disable-sqlwchar-support.patch: ########## @@ -0,0 +1,31 @@ +According to the issue comment https://github.com/SOCI/soci/issues/1306#issuecomment-3351612844 SQLWCHAR being char32_t with libiodbc is unexpected and is not supported by the SOCI library +This patch disables SQLWCHAR using wstrings and instead uses string type as in previous versions of SOCI. + +diff --git a/src/backends/odbc/statement.cpp b/src/backends/odbc/statement.cpp Review Comment: This patch file includes free-form text before the diff header. GNU `patch` commonly fails with “Only garbage was found in the patch input” when the file doesn’t start with a recognizable diff header, which would break both the CMake FetchContent patch step and `apply_conandata_patches()`. Move the explanation into `conandata.yml`’s `patch_description` (or a separate documentation file) and ensure the patch begins with `diff --git` (or `---/+++`) as the first line. ########## cmake/Soci.cmake: ########## @@ -0,0 +1,47 @@ +# 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. + +include(FetchContent) + +include(fmt) + +set(PATCH_FILE1 "${CMAKE_SOURCE_DIR}/thirdparty/soci/all/patches/disable-sqlwchar-support.patch") +set(PATCH_FILE2 "${CMAKE_SOURCE_DIR}/thirdparty/soci/all/patches/odbc-get-parameter-name-bounds-safe.patch") +set(PC ${Bash_EXECUTABLE} -c "set -x &&\ + (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE1}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE1}\\\") &&\ + (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE2}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE2}\\\")") Review Comment: `PATCH_COMMAND "${PC}"` quotes a CMake list, which collapses the command into a single argument (semicolon-delimited) and can prevent the patch step from executing correctly. Use `PATCH_COMMAND ${PC}` (unquoted) so CMake passes the command and its arguments properly. As a follow-up (optional but recommended), consider avoiding a Bash-dependent patch pipeline here (for portability on Windows and minimal build environments) by using a CMake script (`cmake -P`) to apply patches idempotently via `execute_process()`. ########## cmake/GetSOCI.cmake: ########## @@ -0,0 +1,30 @@ +# 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. + +if(MINIFI_SOCI_SOURCE STREQUAL "CONAN") + message("Using Conan to install SOCI") + find_package(SOCI REQUIRED) + # The Conan package exposes per-backend targets; recreate the SOCI::SOCI + # aggregate target the from-source build provides so consumers are unchanged. + if(NOT TARGET SOCI::SOCI) + add_library(SOCI::SOCI INTERFACE IMPORTED) + target_link_libraries(SOCI::SOCI INTERFACE SOCI::soci_core_static SOCI::soci_odbc_static) + endif() +elseif(MINIFI_SOCI_SOURCE STREQUAL "BUILD") + message("Using CMake to build SOCI from source") + include(Soci) +endif() Review Comment: `extensions/sql/CMakeLists.txt` links `minifi-sql` against `SOCI::SOCI`, but in the `BUILD` branch you only `include(Soci)` and don’t ensure an aggregate `SOCI::SOCI` target exists. Unless the FetchContent build defines `SOCI::SOCI` itself, this will fail at configure/generate time with an unknown target. To keep consumers unchanged, create `SOCI::SOCI` in the `BUILD` branch too (or update consumers to link the actual build targets exported by SOCI-from-source consistently across both branches). ########## cmake/Soci.cmake: ########## @@ -0,0 +1,47 @@ +# 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. + +include(FetchContent) + +include(fmt) + +set(PATCH_FILE1 "${CMAKE_SOURCE_DIR}/thirdparty/soci/all/patches/disable-sqlwchar-support.patch") +set(PATCH_FILE2 "${CMAKE_SOURCE_DIR}/thirdparty/soci/all/patches/odbc-get-parameter-name-bounds-safe.patch") +set(PC ${Bash_EXECUTABLE} -c "set -x &&\ + (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE1}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE1}\\\") &&\ + (\\\"${Patch_EXECUTABLE}\\\" -p1 -R -s -f --dry-run -i \\\"${PATCH_FILE2}\\\" || \\\"${Patch_EXECUTABLE}\\\" -p1 -N -i \\\"${PATCH_FILE2}\\\")") + +set(SOCI_TESTS OFF CACHE BOOL "" FORCE) +set(SOCI_SHARED OFF CACHE BOOL "" FORCE) +set(SOCI_ODBC ON CACHE BOOL "" FORCE) +set(SOCI_SQLITE3 OFF CACHE BOOL "" FORCE) +set(SOCI_LTO OFF CACHE BOOL "" FORCE) +set(WITH_BOOST OFF CACHE BOOL "" FORCE) + +FetchContent_Declare( + soci + URL "https://github.com/SOCI/soci/archive/refs/tags/v4.1.4.tar.gz"; + URL_HASH "SHA256=144f017cccc2e2d806badb3313d6ab3c67a1925bccaa747a46fb3907108a615d" + PATCH_COMMAND "${PC}" + SYSTEM +) Review Comment: `PATCH_COMMAND "${PC}"` quotes a CMake list, which collapses the command into a single argument (semicolon-delimited) and can prevent the patch step from executing correctly. Use `PATCH_COMMAND ${PC}` (unquoted) so CMake passes the command and its arguments properly. As a follow-up (optional but recommended), consider avoiding a Bash-dependent patch pipeline here (for portability on Windows and minimal build environments) by using a CMake script (`cmake -P`) to apply patches idempotently via `execute_process()`. ########## thirdparty/iodbc/all/conanfile.py: ########## @@ -0,0 +1,95 @@ +# 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. + +import os + +from conan import ConanFile +from conan.errors import ConanInvalidConfiguration +from conan.tools.files import apply_conandata_patches, copy, export_conandata_patches, get, rm, rmdir +from conan.tools.gnu import Autotools, AutotoolsToolchain +from conan.tools.layout import basic_layout + +required_conan_version = ">=2.0" + + +class IodbcConan(ConanFile): + name = "iodbc" + description = "iODBC is a platform-independent ODBC driver manager (LGPL, with linking exception)" + license = "LGPL-2.0-or-later" + url = "https://github.com/openlink/iODBC"; + homepage = "https://www.iodbc.org/"; + topics = ("odbc", "driver-manager", "database") + package_type = "static-library" + settings = "os", "arch", "compiler", "build_type" + options = { + "fPIC": [True, False], + } + default_options = { + "fPIC": True, + } + + def export_sources(self): + export_conandata_patches(self) + + def layout(self): + basic_layout(self, src_folder="src") + + def validate(self): + if self.settings.os == "Windows": + raise ConanInvalidConfiguration("iodbc does not support Windows; use the system ODBC driver manager instead") + + def build_requirements(self): + self.tool_requires("libtool/2.4.7") + self.tool_requires("autoconf/2.71") + self.tool_requires("automake/1.16.5") + + def source(self): + get(self, **self.conan_data["sources"][self.version], strip_root=True) + + def generate(self): + tc = AutotoolsToolchain(self) + tc.configure_args.append("--with-pic") + tc.configure_args.append("--enable-static") + tc.configure_args.append("--disable-shared") + # Matches the from-source build (see cmake/BundledIodbc.cmake) + tc.extra_cflags.append("-std=gnu17") + tc.generate() Review Comment: The recipe exposes an `fPIC` option but unconditionally passes `--with-pic`, so `-o iodbc/*:fPIC=False` won’t have any effect. Make `--with-pic` conditional on `self.options.fPIC` (and omit/disable it when `False`) so the option behaves as advertised. ########## cmake/GetIODBC.cmake: ########## @@ -0,0 +1,24 @@ +# 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. + +if(MINIFI_IODBC_SOURCE STREQUAL "CONAN") + message("Using Conan to install iODBC") + find_package(ODBC REQUIRED) +elseif(MINIFI_IODBC_SOURCE STREQUAL "BUILD") + message("Using CMake to build iODBC from source") + include(BundledIodbc) +endif() Review Comment: The message says “Using CMake to build iODBC from source”, but `BundledIodbc.cmake` uses `ExternalProject_Add` to drive an autotools build (`autogen.sh` / `configure` / `make`). Updating the message to “build iODBC from source (autotools)” (or similar) would better reflect what’s happening. -- 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]
