Copilot commented on code in PR #2223: URL: https://github.com/apache/nifi-minifi-cpp/pull/2223#discussion_r3672702392
########## thirdparty/openssl-fips/all/conanfile.py: ########## @@ -0,0 +1,75 @@ +# 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.tools.build import build_jobs +from conan.tools.files import copy, get +from conan.tools.layout import basic_layout +from conan.tools.microsoft import is_msvc, VCVars + +required_conan_version = ">=2.0" + + +class OpenSSLFipsConan(ConanFile): + name = "openssl-fips" + description = "OpenSSL FIPS provider module (fips.so / fips.dll) built from FIPS-validated OpenSSL sources" + license = "Apache-2.0" + homepage = "https://github.com/openssl/openssl"; + topics = ("openssl", "fips", "ssl", "tls", "encryption", "security") + package_type = "shared-library" + settings = "os", "arch", "compiler", "build_type" + + def layout(self): + basic_layout(self, src_folder="src") + self.folders.build = self.folders.source + + def source(self): + get(self, **self.conan_data["sources"][self.version], strip_root=True) + + def generate(self): + if is_msvc(self): + VCVars(self).generate() + + @property + def _configure_flags(self): + return [ + "no-tests", + "no-capieng", + "no-legacy", + "no-ssl", + "no-engine", + "enable-fips", + ] + + def build(self): + flags = " ".join(self._configure_flags) + prefix_args = f'"--prefix={self.package_folder}" "--openssldir={self.package_folder}"' + if is_msvc(self): + self.run(f"perl Configure {flags} {prefix_args}", cwd=self.source_folder) + self.run("nmake", cwd=self.source_folder) Review Comment: On MSVC/Windows, OpenSSL’s `Configure` typically requires an explicit target (e.g., `VC-WIN64A`, `VC-WIN32`, `VC-ARM64`) and often won’t reliably auto-detect. This is likely to fail the Windows build. Derive the correct Configure target from `self.settings.arch` and pass it to `perl Configure <target> ...`. ########## thirdparty/openssl-fips/all/conanfile.py: ########## @@ -0,0 +1,75 @@ +# 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.tools.build import build_jobs +from conan.tools.files import copy, get +from conan.tools.layout import basic_layout +from conan.tools.microsoft import is_msvc, VCVars + +required_conan_version = ">=2.0" + + +class OpenSSLFipsConan(ConanFile): + name = "openssl-fips" + description = "OpenSSL FIPS provider module (fips.so / fips.dll) built from FIPS-validated OpenSSL sources" + license = "Apache-2.0" + homepage = "https://github.com/openssl/openssl"; + topics = ("openssl", "fips", "ssl", "tls", "encryption", "security") + package_type = "shared-library" + settings = "os", "arch", "compiler", "build_type" + + def layout(self): + basic_layout(self, src_folder="src") + self.folders.build = self.folders.source + + def source(self): + get(self, **self.conan_data["sources"][self.version], strip_root=True) + + def generate(self): + if is_msvc(self): + VCVars(self).generate() + + @property + def _configure_flags(self): + return [ + "no-tests", + "no-capieng", + "no-legacy", + "no-ssl", + "no-engine", + "enable-fips", + ] + + def build(self): + flags = " ".join(self._configure_flags) + prefix_args = f'"--prefix={self.package_folder}" "--openssldir={self.package_folder}"' + if is_msvc(self): + self.run(f"perl Configure {flags} {prefix_args}", cwd=self.source_folder) + self.run("nmake", cwd=self.source_folder) + else: + self.run(f'./Configure "CFLAGS=-fPIC" "CXXFLAGS=-fPIC" {flags} {prefix_args}', cwd=self.source_folder) + self.run(f"make -j{build_jobs(self)}", cwd=self.source_folder) Review Comment: Configuring OpenSSL with `--prefix`/`--openssldir` set to `self.package_folder` during `build()` can bake absolute paths into generated artifacts/config and breaks the usual Conan separation between build and package steps (hurting relocatability/reproducibility). Prefer configuring against a staging install directory under the build folder, then copy/install from that staging directory into `self.package_folder` in `package()`. ########## conanfile.py: ########## @@ -59,6 +59,7 @@ def requirements(self): self.requires("lz4/1.10.0", force=True) self.requires("libcurl/8.20.0", force=True) self.requires("openssl/3.6.2", force=True) + self.requires("openssl-fips/3.1.2@minifi/develop") self.requires("zlib/1.3.2", force=True) Review Comment: This introduces a version skew between the OpenSSL runtime (`openssl/3.6.2`) and the FIPS provider module built from OpenSSL 3.1.2 sources. In practice, provider modules are expected to be compatible with (and for FIPS validation, built as part of) a specific OpenSSL release line; mixing versions can lead to provider load failures or invalidating FIPS expectations. Consider aligning `openssl` and `openssl-fips` to the same OpenSSL version, or making the FIPS package also provide the matching OpenSSL build used for packaging. ########## cmake/GetOpenSSL.cmake: ########## @@ -23,7 +23,55 @@ if(MINIFI_OPENSSL_SOURCE STREQUAL "CONAN") set(OPENSSL_BIN_DIR "${openssl_PACKAGE_FOLDER_RELEASE}" CACHE STRING "" FORCE) - include(BundledOpenSSLFips) + find_package(openssl-fips REQUIRED) + set(OPENSSL_FIPS_BIN_DIR "${openssl-fips_PACKAGE_FOLDER_RELEASE}" CACHE STRING "" FORCE) + + if(APPLE OR WIN32 OR CMAKE_SIZEOF_VOID_P EQUAL 4 OR CMAKE_SYSTEM_PROCESSOR MATCHES "(arm64)|(ARM64)|(aarch64)|(armv8)") + set(LIBDIR "lib") + else() + set(LIBDIR "lib64") + endif() Review Comment: The `lib` vs `lib64` selection is heuristic and can easily point to a non-existent path (e.g., many OpenSSL source installs use `lib/ossl-modules` even on x86_64). This will break packaging when `install(FILES ...)` can’t find the module. A more robust approach is to probe `${OPENSSL_FIPS_BIN_DIR}/lib/ossl-modules/fips*` and fall back to `lib64` (or vice versa) based on what actually exists in the Conan package. ########## cmake/GetOpenSSL.cmake: ########## @@ -23,7 +23,55 @@ if(MINIFI_OPENSSL_SOURCE STREQUAL "CONAN") set(OPENSSL_BIN_DIR "${openssl_PACKAGE_FOLDER_RELEASE}" CACHE STRING "" FORCE) - include(BundledOpenSSLFips) + find_package(openssl-fips REQUIRED) + set(OPENSSL_FIPS_BIN_DIR "${openssl-fips_PACKAGE_FOLDER_RELEASE}" CACHE STRING "" FORCE) + + if(APPLE OR WIN32 OR CMAKE_SIZEOF_VOID_P EQUAL 4 OR CMAKE_SYSTEM_PROCESSOR MATCHES "(arm64)|(ARM64)|(aarch64)|(armv8)") + set(LIBDIR "lib") + else() + set(LIBDIR "lib64") + endif() + + if (WIN32) + set(BYPRODUCT_DYN_SUFFIX ".dll" CACHE STRING "" FORCE) + elseif(APPLE) + set(BYPRODUCT_DYN_SUFFIX ".dylib" CACHE STRING "" FORCE) + else() + set(BYPRODUCT_DYN_SUFFIX ".so" CACHE STRING "" FORCE) + endif() + + if (WIN32) + set(EXECUTABLE_SUFFIX ".exe" CACHE STRING "" FORCE) + else() + set(EXECUTABLE_SUFFIX "" CACHE STRING "" FORCE) + endif() + + set(FIPS_BYPRODUCTS "${LIBDIR}/ossl-modules/fips${BYPRODUCT_DYN_SUFFIX}") + + FOREACH(BYPRODUCT ${FIPS_BYPRODUCTS}) + LIST(APPEND OPENSSL_FIPS_FILE_LIST "${OPENSSL_FIPS_BIN_DIR}/${BYPRODUCT}") + ENDFOREACH(BYPRODUCT) Review Comment: The `lib` vs `lib64` selection is heuristic and can easily point to a non-existent path (e.g., many OpenSSL source installs use `lib/ossl-modules` even on x86_64). This will break packaging when `install(FILES ...)` can’t find the module. A more robust approach is to probe `${OPENSSL_FIPS_BIN_DIR}/lib/ossl-modules/fips*` and fall back to `lib64` (or vice versa) based on what actually exists in the Conan package. ########## thirdparty/openssl-fips/all/conanfile.py: ########## @@ -0,0 +1,75 @@ +# 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.tools.build import build_jobs +from conan.tools.files import copy, get +from conan.tools.layout import basic_layout +from conan.tools.microsoft import is_msvc, VCVars + +required_conan_version = ">=2.0" + + +class OpenSSLFipsConan(ConanFile): + name = "openssl-fips" + description = "OpenSSL FIPS provider module (fips.so / fips.dll) built from FIPS-validated OpenSSL sources" + license = "Apache-2.0" + homepage = "https://github.com/openssl/openssl"; + topics = ("openssl", "fips", "ssl", "tls", "encryption", "security") + package_type = "shared-library" + settings = "os", "arch", "compiler", "build_type" Review Comment: The recipe declares `package_type = \"shared-library\"`, but the package metadata explicitly exposes no libraries/includes. This mismatch can confuse consumers/tooling that use `package_type` to infer artifacts. Consider omitting `package_type` (or using a more fitting type/metadata approach for a provider module-only package) and documenting how consumers are expected to locate/use the shipped module. ########## thirdparty/openssl-fips/all/conanfile.py: ########## @@ -0,0 +1,75 @@ +# 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.tools.build import build_jobs +from conan.tools.files import copy, get +from conan.tools.layout import basic_layout +from conan.tools.microsoft import is_msvc, VCVars + +required_conan_version = ">=2.0" + + +class OpenSSLFipsConan(ConanFile): + name = "openssl-fips" + description = "OpenSSL FIPS provider module (fips.so / fips.dll) built from FIPS-validated OpenSSL sources" + license = "Apache-2.0" + homepage = "https://github.com/openssl/openssl"; + topics = ("openssl", "fips", "ssl", "tls", "encryption", "security") + package_type = "shared-library" + settings = "os", "arch", "compiler", "build_type" + + def layout(self): + basic_layout(self, src_folder="src") + self.folders.build = self.folders.source + + def source(self): + get(self, **self.conan_data["sources"][self.version], strip_root=True) + + def generate(self): + if is_msvc(self): + VCVars(self).generate() + + @property + def _configure_flags(self): + return [ + "no-tests", + "no-capieng", + "no-legacy", + "no-ssl", + "no-engine", + "enable-fips", + ] + + def build(self): + flags = " ".join(self._configure_flags) + prefix_args = f'"--prefix={self.package_folder}" "--openssldir={self.package_folder}"' + if is_msvc(self): + self.run(f"perl Configure {flags} {prefix_args}", cwd=self.source_folder) + self.run("nmake", cwd=self.source_folder) + else: + self.run(f'./Configure "CFLAGS=-fPIC" "CXXFLAGS=-fPIC" {flags} {prefix_args}', cwd=self.source_folder) + self.run(f"make -j{build_jobs(self)}", cwd=self.source_folder) + + def package(self): + copy(self, "LICENSE.txt", src=self.source_folder, dst=os.path.join(self.package_folder, "licenses")) + self.run("nmake install_fips" if is_msvc(self) else "make install_fips", cwd=self.source_folder) + + def package_info(self): + self.cpp_info.libs = [] + self.cpp_info.includedirs = [] Review Comment: The recipe declares `package_type = \"shared-library\"`, but the package metadata explicitly exposes no libraries/includes. This mismatch can confuse consumers/tooling that use `package_type` to infer artifacts. Consider omitting `package_type` (or using a more fitting type/metadata approach for a provider module-only package) and documenting how consumers are expected to locate/use the shipped module. -- 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]
