Copilot commented on code in PR #2212:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2212#discussion_r3541922356
##########
CMakeLists.txt:
##########
@@ -314,13 +319,17 @@ endif()
if (STRICT_GSL_CHECKS STREQUAL "DEBUG_ONLY")
list(APPEND GslDefinitions
$<$<NOT:$<CONFIG:Debug>>:${GslDefinitionsNonStrict}>)
endif()
-target_compile_definitions(gsl-lite INTERFACE ${GslDefinitions})
-# date
-include(Date)
+get_target_property(_gsl_lite_real_target gsl-lite::gsl-lite ALIASED_TARGET)
+if (NOT _gsl_lite_real_target)
+ set(_gsl_lite_real_target gsl-lite::gsl-lite)
+endif()
+target_compile_definitions(${_gsl_lite_real_target} INTERFACE
${GslDefinitions})
Review Comment:
This assumes the target `gsl-lite::gsl-lite` already exists, but in BUILD
mode the wrapper `cmake/GslLite.cmake` only runs
`FetchContent_MakeAvailable(gsl-lite)` and does not guarantee a namespaced
target. If `gsl-lite::gsl-lite` is not created by the upstream project,
`get_target_property()`/`target_compile_definitions()` will fail at configure
time.
##########
cmake/GetRangeV3.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_RANGEV3_SOURCE STREQUAL "CONAN")
+ message("Using Conan to install range-v3")
+ find_package(range-v3 REQUIRED)
+elseif(MINIFI_RANGEV3_SOURCE STREQUAL "BUILD")
+ message("Using CMake to build range-v3 from source")
+ include(RangeV3)
+endif()
Review Comment:
In BUILD mode this includes `cmake/RangeV3.cmake`, but that file currently
FetchContents **magic_enum** (see `cmake/RangeV3.cmake:20-25`), not range-v3.
This breaks mixed-source setups (e.g., `MINIFI_RANGEV3_SOURCE=BUILD` with
`MINIFI_MAGIC_ENUM_SOURCE=CONAN`) because range-v3 will not be built when
expected.
##########
cmake/GetMagicEnum.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_MAGIC_ENUM_SOURCE STREQUAL "CONAN")
+ message("Using Conan to install magic_enum")
+ find_package(magic_enum REQUIRED)
+elseif(MINIFI_MAGIC_ENUM_SOURCE STREQUAL "BUILD")
+ message("Using CMake to build magic_enum from source")
+ include(MagicEnum)
+endif()
Review Comment:
In BUILD mode this includes `cmake/MagicEnum.cmake`, but that file currently
FetchContents **range-v3** (see `cmake/MagicEnum.cmake:24-31`), not magic_enum.
This breaks mixed-source setups (e.g., `MINIFI_MAGIC_ENUM_SOURCE=BUILD` with
`MINIFI_RANGEV3_SOURCE=CONAN`) because magic_enum will not be provided at all.
##########
bootstrap/cli.py:
##########
@@ -28,24 +30,88 @@ def install_dependencies(minifi_options: MinifiOptions,
package_manager: Package
return res
+def export_custom_conan_recipes(minifi_options: MinifiOptions,
package_manager: PackageManager) -> bool:
+ thirdparty_dir = minifi_options.source_dir / "thirdparty"
+ for root, _, files in os.walk(thirdparty_dir):
+ for file in files:
+ if file == "conanfile.py":
+ config_yaml = os.path.join(Path(root).parent, "config.yml")
+
+ with open(config_yaml) as f:
+ data = yaml.safe_load(f)
+
+ version = next(iter(data["versions"]))
+
+ print(f"Exporting the custom Conan recipe {root} with version
{version}")
+ if not package_manager.run_cmd(f"conan export {root}
--version={version} --user=minifi --channel=develop"):
+ print(f"Exporting the custom Conan recipe {root} failed")
+ return False
+ return True
+
+
+def add_conan_options_from_cmake_options(extension_options: list[str],
minifi_options: MinifiOptions) -> str:
+ conan_options = ""
+ for extension_option in extension_options:
+ if minifi_options.bool_options[extension_option.upper()].value not in
(None, "OFF"):
+ conan_options += f' -o "&:{extension_option.lower()}=True"'
+ return conan_options
+
+
+def run_conan_install(minifi_options: MinifiOptions, package_manager:
PackageManager) -> bool:
+ if not minifi_options.use_conan.value == "ON":
+ print("Conan install skipped because USE_CONAN is OFF")
+ return True
+ conan_options = add_conan_options_from_cmake_options(["ENABLE_ALL",
"ENABLE_ROCKSDB", "ENABLE_SFTP", "ENABLE_PROMETHEUS", "ENABLE_BZIP2",
"ENABLE_LZMA", "ENABLE_MQTT", "ENABLE_COUCHBASE",
+ "ENABLE_KAFKA",
"ENABLE_OPC", "SKIP_TESTS"], minifi_options)
+ if minifi_options.custom_malloc is not None and
minifi_options.custom_malloc.value not in (None, "OFF"):
+ conan_options += f' -o
"&:custom_malloc={minifi_options.custom_malloc.value}"'
+
+ if not package_manager.run_cmd("conan profile detect --exist-ok"):
+ print("Conan default profile detection failed")
+ return False
+
+ if not export_custom_conan_recipes(minifi_options, package_manager):
+ return False
+
+ compiler_settings = " --settings=compiler.cppstd=23"
+ generator_setting = " -c tools.cmake.cmaketoolchain:generator=Ninja" if
minifi_options.use_ninja.value == "ON" else ""
+ conan_remote_add_cmd = "conan remote add nifi-conan
https://apache.jfrog.io/artifactory/api/conan/nifi-conan --force"
+ if not package_manager.run_cmd(conan_remote_add_cmd):
+ print("Adding the nifi-conan remote failed")
+ return False
+ build_cmd = f"conan install {minifi_options.source_dir}
--output-folder={minifi_options.build_dir} --build=missing {conan_options} " \
+
f"--settings=build_type={minifi_options.build_type.value}{generator_setting}{compiler_settings}"
+ res = package_manager.run_cmd(build_cmd)
+ print("Conan install was successful" if res else "Conan install was
unsuccessful")
+ return res
+
+
+def _conan_build_env_prefix(minifi_options: MinifiOptions) -> str:
+ if minifi_options.use_conan.value == "ON" and platform.system() ==
"Windows":
+ conanbuild = os.path.join(str(minifi_options.build_dir),
"conanbuild.bat")
+ return f'call "{conanbuild}" && '
+ return ""
Review Comment:
`_conan_build_env_prefix()` unconditionally calls
`<build_dir>/conanbuild.bat` on Windows when `USE_CONAN=ON`. If Conan doesn't
generate this script (or it is in a different location), the subsequent
cmake/build commands will fail before doing any work.
##########
.github/workflows/ci.yml:
##########
@@ -84,9 +87,17 @@ jobs:
echo
"PATH=/opt/homebrew/opt/ccache:/opt/homebrew/opt/ccache/bin:/opt/homebrew/opt/ccache/libexec:$PATH"
>> $GITHUB_ENV
echo "DYLD_LIBRARY_PATH=$(brew --prefix)/lib" >> $GITHUB_ENV
echo -e "127.0.0.1\t$HOSTNAME" | sudo tee -a /etc/hosts > /dev/null
+ mkdir -p ~/.conan2/extensions/plugins/compatibility
+ cp .github/conan/compatibility.py
~/.conan2/extensions/plugins/compatibility/compatibility.py
- name: build
run: |
- python -m venv venv && source venv/bin/activate && pip install -r
requirements.txt && python main.py --noninteractive --skip-compiler-install
--cmake-options="-DCMAKE_C_FLAGS=${CPPFLAGS} ${CFLAGS}
-DCMAKE_CXX_FLAGS=${CPPFLAGS} ${CXXFLAGS}"
--minifi-options="${MACOS_MINIFI_OPTIONS}"
+ python -m venv venv && source venv/bin/activate && pip install -r
requirements.txt && \
+ python main.py --noninteractive --skip-compiler-install
--cmake-options="-DCMAKE_C_FLAGS=${CPPFLAGS} ${CFLAGS}
-DCMAKE_CXX_FLAGS=${CPPFLAGS} ${CXXFLAGS}"
--minifi-options="${MACOS_MINIFI_OPTIONS}"
+ working-directory: bootstrap
+ - name: Upload conan packages
+ if: always() && github.event_name == 'push' && github.ref ==
'refs/heads/main'
+ run: |
+ source venv/bin/activate && conan remote login nifi-conan && conan
upload "*" -r nifi-conan --confirm
working-directory: bootstrap
Review Comment:
`conan upload "*"` will upload the entire local Conan cache to the
nifi-conan remote (including transitive dependencies fetched/built from
ConanCenter). This is risky (size/time) and may unintentionally publish
packages that should not be hosted in the NiFi remote. Consider restricting
uploads to only MiNiFi-owned recipes/packages (e.g., `@minifi/develop`).
##########
thirdparty/open62541/all/conanfile.py:
##########
@@ -0,0 +1,169 @@
+# 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/open62541/all/conanfile.py
+from conan import ConanFile
+from conan.tools.cmake import cmake_layout, CMake, CMakeToolchain, CMakeDeps
+from conan.tools.scm import Version
+from conan.tools.files import apply_conandata_patches, collect_libs,
export_conandata_patches, copy, rm, rmdir, get
+from conan.errors import ConanInvalidConfiguration
+import glob
+import os
+
+required_conan_version = ">=2.0"
+
+
+class Open62541Conan(ConanFile):
+ name = "open62541"
+ description = "open62541 is an open source and free implementation of OPC
UA " \
+ "(OPC Unified Architecture) written in the common subset of
the " \
+ "C99 and C++98 languages. The library is usable with all
major " \
+ "compilers and provides the necessary tools to implement
dedicated " \
+ "OPC UA clients and servers, or to integrate OPC UA-based
communication " \
+ "into existing applications. open62541 library is platform
independent. " \
+ "All platform-specific functionality is implemented via
exchangeable " \
+ "plugins. Plugin implementations are provided for the major
operating systems."
+ license = ("MPL-2.0", "CC0-1.0")
+ url = "https://github.com/conan-io/conan-center-index";
+ homepage = "https://open62541.org/";
+ topics = (
+ "opc ua", "sdk", "server/client", "c", "iec-62541",
+ "industrial automation", "tsn", "time sensitive networks",
"publish-subscirbe", "pubsub"
+ )
Review Comment:
Typo in the topics list: `publish-subscirbe` should be `publish-subscribe`.
##########
cmake/Fetchlibrdkafka.cmake:
##########
@@ -55,3 +55,16 @@ target_include_directories(rdkafka SYSTEM PRIVATE
${ZSTD_INCLUDE_DIRS})
target_include_directories(rdkafka SYSTEM PRIVATE ${LZ4_INCLUDE_DIRS})
target_link_libraries(rdkafka INTERFACE zstd::zstd lz4::lz4)
+
+if (NOT TARGET RdKafka::rdkafka)
+ add_library(RdKafka::rdkafka++ ALIAS rdkafka)
+endif()
Review Comment:
The guard checks for `RdKafka::rdkafka`, but the code creates
`RdKafka::rdkafka++`. If some other path defines `RdKafka::rdkafka` without
defining `RdKafka::rdkafka++`, this will skip creating the alias and later
`target_link_libraries(... RdKafka::rdkafka++)` will fail.
##########
.github/workflows/ci.yml:
##########
@@ -192,14 +204,27 @@ jobs:
if ((Get-FileHash 'sqliteodbc_w64.exe').Hash -ne
"a4804e4f54f42c721df1323c5fcac101a8c7a577e7f20979227324ceab572d51") {Write
"Hash mismatch"; Exit 1}
Start-Process -FilePath ".\sqliteodbc_w64.exe" -ArgumentList "/S"
-Wait
shell: powershell
+ - name: Copy conan compatibility.py
+ shell: powershell
+ run: |
+ $compatDir = Join-Path $env:USERPROFILE
".conan2\extensions\plugins\compatibility"
+ New-Item -ItemType Directory -Force -Path $compatDir | Out-Null
+ Copy-Item ".github\conan\compatibility.py" (Join-Path $compatDir
"compatibility.py")
- name: Zero sccache stats
run: sccache --zero-stats
shell: bash
- name: build
+ working-directory: bootstrap
run: |
- python -m venv venv & venv\Scripts\activate & pip install -r
requirements.txt & python main.py --noninteractive --skip-compiler-install
--minifi-options="%WINDOWS_MINIFI_OPTIONS%"
--cmake-options="-DCMAKE_C_COMPILER_LAUNCHER=sccache
-DCMAKE_CXX_COMPILER_LAUNCHER=sccache"
+ python -m venv venv && venv\Scripts\activate && pip install -r
requirements.txt && ^
+ python main.py --noninteractive --skip-compiler-install
--minifi-options="%WINDOWS_MINIFI_OPTIONS%"
--cmake-options="-DCMAKE_C_COMPILER_LAUNCHER=sccache
-DCMAKE_CXX_COMPILER_LAUNCHER=sccache"
shell: cmd
+ - name: Upload conan packages
+ if: always() && github.event_name == 'push' && github.ref ==
'refs/heads/main'
working-directory: bootstrap
+ run: |
+ venv\Scripts\activate && conan remote login nifi-conan && conan
upload "*" -r nifi-conan --confirm
+ shell: cmd
Review Comment:
`conan upload "*"` will upload the entire local Conan cache to the
nifi-conan remote (including transitive dependencies fetched/built from
ConanCenter). This is risky (size/time) and may unintentionally publish
packages that should not be hosted in the NiFi remote. Consider restricting
uploads to only MiNiFi-owned recipes/packages (e.g., `@minifi/develop`).
##########
cmake/PahoMqttC.cmake:
##########
@@ -40,6 +38,4 @@ FetchContent_Declare(
FetchContent_MakeAvailable(paho.mqtt.c-external)
-# Set dependencies and target to link to
-add_library(paho.mqtt.c ALIAS paho-mqtt3as-static)
target_link_libraries(common_ssl_obj_static PUBLIC OpenSSL::SSL
OpenSSL::Crypto)
Review Comment:
After removing the `paho.mqtt.c` alias, this module no longer provides a
stable CMake target name for consumers. The extension now links
`eclipse-paho-mqtt-c::paho-mqtt3as-static`, but this BUILD-path FetchContent
likely creates an un-namespaced target such as `paho-mqtt3as-static` (as
implied by the old alias), so BUILD mode can fail at configure/link time.
--
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]
