Copilot commented on code in PR #2214: URL: https://github.com/apache/nifi-minifi-cpp/pull/2214#discussion_r3597053348
########## cmake/GetLlamaCpp.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_LLAMACPP_SOURCE STREQUAL "CONAN") + message("Using Conan to install LlamaCpp") + find_package(llamacpp REQUIRED) +elseif(MINIFI_LLAMACPP_SOURCE STREQUAL "BUILD") + message("Using CMake to build LlamaCpp from source") + include(LlamaCpp) +endif() Review Comment: In CONAN mode this module only does find_package(llamacpp), but the rest of the build (e.g. extensions/llamacpp) links against llama-cpp::llama/::common/::mtmd targets (created in the BUILD path by cmake/LlamaCpp.cmake). Without creating compatible alias targets here, CONAN builds will fail to link due to missing targets. ########## thirdparty/azure-sdk-cpp/all/conanfile.py: ########## @@ -0,0 +1,196 @@ +# 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. + +# Recipe based on https://github.com/conan-io/conan-center-index/blob/master/recipes/azure-sdk-for-cpp/all/conanfile.py + +from conan import ConanFile +from conan.errors import ConanInvalidConfiguration +from conan.tools.build import check_min_cppstd +from conan.tools.cmake import CMakeToolchain, CMake, CMakeDeps, cmake_layout +from conan.tools.files import get, copy, rmdir, replace_in_file +from conan.tools.scm import Version +import glob +import os + +required_conan_version = ">=2.1" + +class AzureSDKForCppConan(ConanFile): + name = "azure-sdk-for-cpp" + description = "Microsoft Azure Storage Client Library for C++" + license = "Apache-2.0" + url = "https://github.com/conan-io/conan-center-index"; + homepage = "https://github.com/Azure/azure-sdk-for-cpp"; + topics = ("azure", "cpp", "cross-platform", "microsoft", "cloud") + package_type = "library" + settings = "os", "arch", "compiler", "build_type" + + options = { + "shared": [True, False], + "fPIC": [True, False], + "transport_winhttp": [True, False], + "transport_curl": [True, False] + } + + default_options = {"shared": False, "fPIC": True} + + default_options = { + "shared": False, + "fPIC": True, + "transport_winhttp": False, + "transport_curl": True + } + + def source(self): + get(self, **self.conan_data["sources"][self.version], strip_root=True) + for cmake_file in glob.glob(os.path.join(self.source_folder, "sdk", "identity", "azure-identity", "**", "CMakeLists.txt"), recursive=True): + replace_in_file(self, cmake_file, + "set(CMAKE_CXX_STANDARD 14)", + "set(CMAKE_CXX_STANDARD 17)", + strict=False) + + def config_options(self): + if self.settings.os == "Windows": + del self.options.fPIC + else: + del self.options.transport_winhttp + + def configure(self): + if self.options.get_safe("shared"): + self.options.rm_safe("fPIC") + + def requirements(self): + self.requires("openssl/3.6.2") + self.requires("libxml2/2.15.3") + + if self.settings.os == "Windows": + # wil is always required on windows since azure-identity can't be disabled via cmake. + # azure-identity and wil are not necessary for storage if skip_test=True, but MS + # doesn't currently support that build option... + self.requires("wil/1.0.260126.7") + + if self.options.transport_curl: + self.requires("libcurl/8.20.0") + + def layout(self): + cmake_layout(self, src_folder="src") + + def validate(self): + if self.settings.compiler.get_safe("cppstd"): + check_min_cppstd(self, 14) + + if self.settings.os == "Windows": + if not self.options.transport_curl and not self.options.get_safe("transport_winhttp"): + raise ConanInvalidConfiguration("On Windows, HTTP Transport options: transport_winhttp or transport_curl must be enabled.") + elif not self.options.transport_curl: + raise ConanInvalidConfiguration("The HTTP Transport option transport_curl must be enabled.") Review Comment: The `elif not self.options.transport_curl:` branch is mis-indented, which will raise an IndentationError (or create an unintended nested block) and break the recipe. ########## thirdparty/llamacpp/config.yml: ########## @@ -0,0 +1,3 @@ +versions: + "b8944": + folder: "all" Review Comment: This new thirdparty config file is missing the standard ASF license header used by the other thirdparty/*/config.yml files added in this PR, which can cause licensing/compliance checks to fail. ########## cmake/GetGrpc.cmake: ########## @@ -0,0 +1,27 @@ +# 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_GRPC_SOURCE STREQUAL "CONAN") + message("Using Conan to install gRPC") + find_package(protobuf CONFIG REQUIRED) + find_package(gRPC CONFIG REQUIRED) + set(PROTOBUF_INCLUDE_DIR "${protobuf_INCLUDE_DIR}" CACHE STRING "" FORCE) + set(GRPC_INCLUDE_DIR "${gRPC_INCLUDE_DIR}" CACHE STRING "" FORCE) +elseif(MINIFI_GRPC_SOURCE STREQUAL "BUILD") + message("Using CMake to build gRPC from source") + include(Grpc) +endif() Review Comment: This assumes `protobuf_INCLUDE_DIR` / `gRPC_INCLUDE_DIR` variables exist after `find_package(...)`, but those variables are not guaranteed to be provided by CMake package configs (and often Conan's CMakeDeps exposes `*_INCLUDE_DIRS` instead). If these are empty, downstream code (e.g. proto generation) may break. ########## cmake/GenerateGrafanaLokiProto.cmake: ########## @@ -0,0 +1,74 @@ +# 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. + +set(LOKI_PROTOBUF_GENERATED_DIR ${CMAKE_BINARY_DIR}/grafana-loki-protobuf-generated) +file(MAKE_DIRECTORY ${LOKI_PROTOBUF_GENERATED_DIR}) + +if(MINIFI_GRPC_SOURCE STREQUAL "CONAN") + find_package(protobuf CONFIG REQUIRED) + find_package(gRPC CONFIG REQUIRED) + + find_program(LOKI_PROTOC_EXECUTABLE + NAMES protoc + PATHS "${protobuf_INCLUDE_DIR}/../bin" + NO_DEFAULT_PATH + REQUIRED) Review Comment: Locating `protoc` via `${protobuf_INCLUDE_DIR}/../bin` is brittle and likely wrong for many package layouts (and `${protobuf_INCLUDE_DIR}` may not even be defined). Since `find_package(protobuf)` provides an imported `protobuf::protoc` target, use that instead. ########## cmake/GetDate.cmake: ########## @@ -0,0 +1,58 @@ +# 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 (WIN32) + include(FetchContent) + + # tzdata and windowsZones.xml from unicode cldr-common are required to be installed for date-tz operation on Windows + FetchContent_Declare(tzdata + URL https://data.iana.org/time-zones/releases/tzdata2026b.tar.gz + URL_HASH SHA256=114543d9f19a6bfeb5bca43686aea173d38755a3db1f2eec112647ae92c6f544 + SYSTEM + ) + FetchContent_GetProperties(tzdata) + if (NOT tzdata_POPULATED) + FetchContent_Populate(tzdata) + endif() + + file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/tzdata) + + file(COPY ${CMAKE_CURRENT_SOURCE_DIR}/thirdparty/cldr-common-38.1/common/supplemental/windowsZones.xml + DESTINATION ${CMAKE_BINARY_DIR}/tzdata) + + file(COPY ${tzdata_SOURCE_DIR}/ + DESTINATION ${CMAKE_BINARY_DIR}/tzdata) + + install(DIRECTORY ${tzdata_SOURCE_DIR}/ + DESTINATION tzdata + COMPONENT bin) + + install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/thirdparty/cldr-common-38.1/common/supplemental/windowsZones.xml + DESTINATION tzdata + COMPONENT bin) Review Comment: Same path issue as above: `${CMAKE_CURRENT_SOURCE_DIR}/thirdparty/...` points under `cmake/`, not the repo root. The install step will fail to find windowsZones.xml. ########## thirdparty/azure-sdk-cpp/all/conanfile.py: ########## @@ -0,0 +1,196 @@ +# 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. + +# Recipe based on https://github.com/conan-io/conan-center-index/blob/master/recipes/azure-sdk-for-cpp/all/conanfile.py + +from conan import ConanFile +from conan.errors import ConanInvalidConfiguration +from conan.tools.build import check_min_cppstd +from conan.tools.cmake import CMakeToolchain, CMake, CMakeDeps, cmake_layout +from conan.tools.files import get, copy, rmdir, replace_in_file +from conan.tools.scm import Version +import glob +import os + +required_conan_version = ">=2.1" + +class AzureSDKForCppConan(ConanFile): + name = "azure-sdk-for-cpp" + description = "Microsoft Azure Storage Client Library for C++" + license = "Apache-2.0" + url = "https://github.com/conan-io/conan-center-index"; + homepage = "https://github.com/Azure/azure-sdk-for-cpp"; + topics = ("azure", "cpp", "cross-platform", "microsoft", "cloud") + package_type = "library" + settings = "os", "arch", "compiler", "build_type" + + options = { + "shared": [True, False], + "fPIC": [True, False], + "transport_winhttp": [True, False], + "transport_curl": [True, False] + } + + default_options = {"shared": False, "fPIC": True} + Review Comment: `default_options` is assigned twice; the first assignment is immediately overwritten and adds noise to the recipe. ########## cmake/GetDate.cmake: ########## @@ -0,0 +1,58 @@ +# 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 (WIN32) + include(FetchContent) + + # tzdata and windowsZones.xml from unicode cldr-common are required to be installed for date-tz operation on Windows + FetchContent_Declare(tzdata + URL https://data.iana.org/time-zones/releases/tzdata2026b.tar.gz + URL_HASH SHA256=114543d9f19a6bfeb5bca43686aea173d38755a3db1f2eec112647ae92c6f544 + SYSTEM + ) + FetchContent_GetProperties(tzdata) + if (NOT tzdata_POPULATED) + FetchContent_Populate(tzdata) + endif() + + file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/tzdata) + + file(COPY ${CMAKE_CURRENT_SOURCE_DIR}/thirdparty/cldr-common-38.1/common/supplemental/windowsZones.xml + DESTINATION ${CMAKE_BINARY_DIR}/tzdata) Review Comment: `CMAKE_CURRENT_SOURCE_DIR` here points to the `cmake/` directory (because this file is included), so the windowsZones.xml path resolves to `cmake/thirdparty/...` which does not exist. This will fail on Windows when staging tzdata. ########## thirdparty/kubernetes-client-c/config.yml: ########## @@ -0,0 +1,3 @@ +versions: + "0.14.0": + folder: "all" Review Comment: This new thirdparty config file is missing the standard ASF license header used by the other thirdparty/*/config.yml files added in this PR, which can cause licensing/compliance checks to fail. ########## cmake/GenerateGrafanaLokiProto.cmake: ########## @@ -0,0 +1,74 @@ +# 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. + +set(LOKI_PROTOBUF_GENERATED_DIR ${CMAKE_BINARY_DIR}/grafana-loki-protobuf-generated) +file(MAKE_DIRECTORY ${LOKI_PROTOBUF_GENERATED_DIR}) + +if(MINIFI_GRPC_SOURCE STREQUAL "CONAN") + find_package(protobuf CONFIG REQUIRED) + find_package(gRPC CONFIG REQUIRED) + + find_program(LOKI_PROTOC_EXECUTABLE + NAMES protoc + PATHS "${protobuf_INCLUDE_DIR}/../bin" + NO_DEFAULT_PATH + REQUIRED) + + add_custom_command( + OUTPUT + ${LOKI_PROTOBUF_GENERATED_DIR}/grafana-loki-push.grpc.pb.cc + ${LOKI_PROTOBUF_GENERATED_DIR}/grafana-loki-push.grpc.pb.h + ${LOKI_PROTOBUF_GENERATED_DIR}/grafana-loki-push.pb.h + ${LOKI_PROTOBUF_GENERATED_DIR}/grafana-loki-push.pb.cc + COMMAND ${LOKI_PROTOC_EXECUTABLE} + ARGS + --plugin=protoc-gen-grpc=$<TARGET_FILE:gRPC::grpc_cpp_plugin> + --proto_path=. + --grpc_out=${LOKI_PROTOBUF_GENERATED_DIR} + --cpp_out=${LOKI_PROTOBUF_GENERATED_DIR} + grafana-loki-push.proto + WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/protos + DEPENDS + ${LOKI_PROTOC_EXECUTABLE} + gRPC::grpc_cpp_plugin + ${CMAKE_CURRENT_SOURCE_DIR}/protos/grafana-loki-push.proto Review Comment: After switching to a target-based protoc path, the custom command should depend on the `protobuf::protoc` target rather than the executable path string, otherwise CMake may not order the build correctly. -- 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]
