fgerlits commented on code in PR #1993:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1993#discussion_r2291267600
##########
core-framework/include/Defaults.h:
##########
Review Comment:
please add the necessary standard library `#include`s; we shouldn't rely on
the file including this to do that
##########
libminifi/src/utils/file/AssetManager.cpp:
##########
@@ -16,18 +16,27 @@
*/
#include "utils/file/AssetManager.h"
-#include "utils/file/FileUtils.h"
+
+#include "core/logging/LoggerFactory.h"
#include "rapidjson/document.h"
#include "rapidjson/writer.h"
-#include "core/logging/LoggerFactory.h"
#include "utils/Hash.h"
+#include "utils/Locations.h"
+#include "utils/file/FileUtils.h"
#undef GetObject // windows.h #defines GetObject = GetObjectA or GetObjectW,
which conflicts with rapidjson
namespace org::apache::nifi::minifi::utils::file {
+std::filesystem::path getRootFromConfigure(const Configure& configuration) {
Review Comment:
can we hide this somehow, eg. in an anonymous namespace?
##########
minifi_main/MainHelper.h:
##########
Review Comment:
I know you didn't change this, but I would prefer to put these helpers in
some minifi namespace.
##########
minifi_main/MainHelper.cpp:
##########
@@ -106,8 +108,50 @@ std::filesystem::path determineMinifiHome(const
std::shared_ptr<logging::Logger>
}
/* Set the valid MINIFI_HOME in our environment */
- logger->log_info("Using " MINIFI_HOME_ENV_KEY "={}", minifi_home);
- utils::Environment::setEnvironmentVariable(MINIFI_HOME_ENV_KEY,
minifi_home.string().c_str());
+ logger->log_info("Using {}={}", MINIFI_HOME_ENV_KEY, minifi_home);
+ utils::Environment::setEnvironmentVariable(MINIFI_HOME_ENV_KEY.data(),
minifi_home.string().c_str());
return minifi_home;
}
+
+Locations getFromMinifiHome(const std::filesystem::path& minifi_home) {
+ return {
+ .working_dir_ = minifi_home,
+ .lock_path_ = minifi_home / "LOCK",
+ .log_properties_path_ = minifi_home / DEFAULT_LOG_PROPERTIES_FILE,
+ .uid_properties_path_ = minifi_home / DEFAULT_UID_PROPERTIES_FILE,
+ .properties_path_ = minifi_home / DEFAULT_NIFI_PROPERTIES_FILE,
+ .logs_dir_ = minifi_home / "logs",
+ .fips_bin_path_ = minifi_home / "fips",
+ .fips_conf_path_ = minifi_home / "fips",
+ };
+}
+
+Locations getFromFHS() {
+ return {
+ .working_dir_ = std::filesystem::path(RPM_WORK_DIR),
+ .lock_path_ = std::filesystem::path(RPM_WORK_DIR) / "LOCK",
+ .log_properties_path_ = std::filesystem::path(RPM_CONFIG_DIR) /
"minifi-log.properties",
+ .uid_properties_path_ = std::filesystem::path(RPM_CONFIG_DIR) /
"minifi-uid.properties",
+ .properties_path_ = std::filesystem::path(RPM_CONFIG_DIR) /
"minifi.properties",
+ .logs_dir_ = std::filesystem::path(RPM_LOG_DIR),
+ .fips_bin_path_ = std::filesystem::path(RPM_LIB_DIR) / "fips",
+ .fips_conf_path_ = std::filesystem::path(RPM_CONFIG_DIR) / "fips"
+ };
+}
+
+std::optional<Locations> determineLocations(const
std::shared_ptr<logging::Logger>& logger) {
+ if (const auto minifi_home_env =
utils::Environment::getEnvironmentVariable(MINIFI_HOME_ENV_KEY.data())) {
+ if (minifi_home_env == MINIFI_HOME_ENV_VALUE_FHS) {
+ return getFromFHS();
+ }
+ }
+ if (const auto executable_path = utils::file::get_executable_path();
executable_path.parent_path() == "/usr/bin") {
+ return getFromFHS();
+ }
Review Comment:
If `MINIFI_HOME` is set, it would seem more natural to use it even if the
binary is in `/usr/bin`. What is the reason for not allowing this?
##########
minifi_main/MainHelper.h:
##########
@@ -42,29 +40,60 @@ extern "C" {
#define STOP_WAIT_TIME_MS 30*1000
//! Default YAML location
-//! Define home environment variable
-#define MINIFI_HOME_ENV_KEY "MINIFI_HOME"
-
#ifdef _MSC_VER
#ifndef PATH_MAX
#define PATH_MAX 260
#endif
#endif
-/**
- * Validates a MINIFI_HOME value.
- * @param home_path
- * @return true if home_path represents a valid MINIFI_HOME
- */
-bool validHome(const std::string &home_path);
+struct Locations {
+ std::filesystem::path working_dir_;
+ std::filesystem::path lock_path_;
+ std::filesystem::path log_properties_path_;
+ std::filesystem::path uid_properties_path_;
+ std::filesystem::path properties_path_;
+ std::filesystem::path logs_dir_;
+ std::filesystem::path fips_bin_path_;
+ std::filesystem::path fips_conf_path_;
+};
/**
* Configures the logger to log everything to syslog/Windows Event Log, and
for the minimum log level to INFO
*/
void setSyslogLogger();
-/**
- * Determines the full path of MINIFI_HOME
- * @return MINIFI_HOME on success, empty string on failure
- */
std::filesystem::path determineMinifiHome(const
std::shared_ptr<org::apache::nifi::minifi::core::logging::Logger>& logger);
+
+std::optional<Locations> determineLocations(const
std::shared_ptr<org::apache::nifi::minifi::core::logging::Logger>& logger);
+
+template <>
+struct fmt::formatter<Locations> {
+ constexpr auto parse(format_parse_context& ctx) ->
format_parse_context::iterator {
+ return ctx.begin();
+ }
+
+ // Format the Locations object
+ template <typename Context>
+ auto format(const Locations& loc, Context& ctx) const -> decltype(ctx.out())
{
+ constexpr std::string_view fmt_str =
+ "\n"
+ "{:<24} {}\n"
+ "{:<24} {}\n"
+ "{:<24} {}\n"
+ "{:<24} {}\n"
+ "{:<24} {}\n"
+ "{:<24} {}\n"
+ "{:<24} {}\n"
+ "{:<24} {}";
+
+ return fmt::format_to(ctx.out(), fmt_str,
+ "Working dir:", loc.working_dir_,
+ "Lock path:", loc.lock_path_,
+ "Log properties path:", loc.log_properties_path_,
+ "UID properties path:", loc.uid_properties_path_,
+ "Properties path:", loc.properties_path_,
+ "Logs dir:", loc.logs_dir_,
+ "Fips binary path:", loc.fips_bin_path_,
+ "Fips conf path:", loc.fips_conf_path_);
+ }
+};
Review Comment:
just a suggestion, but (as long as you don't need a parser) the fmtlib api
has a simpler alternate syntax for this:
```c++
inline auto format_as(const Locations& loc) {
static constexpr std::string_view fmt_str =
"\n"
"{:<24} {}\n"
"{:<24} {}\n"
"{:<24} {}\n"
"{:<24} {}\n"
"{:<24} {}\n"
"{:<24} {}\n"
"{:<24} {}\n"
"{:<24} {}";
return fmt::format(fmt_str,
"Working dir:", loc.working_dir_,
"Lock path:", loc.lock_path_,
"Log properties path:", loc.log_properties_path_,
"UID properties path:", loc.uid_properties_path_,
"Properties path:", loc.properties_path_,
"Logs dir:", loc.logs_dir_,
"Fips binary path:", loc.fips_bin_path_,
"Fips conf path:", loc.fips_conf_path_);
}
```
`format_as` should be a free function in the same namespace as `Locations`
##########
libminifi/src/core/repository/FileSystemRepository.cpp:
##########
@@ -29,7 +29,7 @@ bool FileSystemRepository::initialize(const
std::shared_ptr<Configure>& configur
if (std::string directory_str;
configuration->get(Configure::nifi_dbcontent_repository_directory_default,
directory_str) && !directory_str.empty()) {
directory_ = directory_str;
} else {
- directory_ = configuration->getHome().string();
+ directory_ = std::filesystem::current_path().string();
Review Comment:
this looks strange -- is `current_path()` the directory we started MiNiFi
from? why do we want to create the repository files there?
##########
core-framework/include/Defaults.h:
##########
@@ -23,3 +23,6 @@ const std::filesystem::path DEFAULT_NIFI_PROPERTIES_FILE =
std::filesystem::path
const std::filesystem::path DEFAULT_LOG_PROPERTIES_FILE =
std::filesystem::path("conf") / "minifi-log.properties";
const std::filesystem::path DEFAULT_UID_PROPERTIES_FILE =
std::filesystem::path("conf") / "minifi-uid.properties";
const std::filesystem::path DEFAULT_BOOTSTRAP_FILE =
std::filesystem::path("conf") / "bootstrap.conf";
+
+constexpr std::string_view MINIFI_HOME_ENV_KEY = "MINIFI_HOME";
+constexpr std::string_view MINIFI_HOME_ENV_VALUE_FHS = "FHS";
Review Comment:
minor, but these should be `inline`, so we don't get a separate copy for
every translation unit `Defaults.h` is included in
##########
packaging/rpm/check_rpm_contents.sh:
##########
@@ -0,0 +1,45 @@
+#!/bin/bash
+
+if [ "$#" -ne 2 ]; then
+ echo "Usage: $0 <RPM_FILE> <EXPECTED_LIST_FILE>"
+ exit 1
+fi
+
+RPM_FILE="$1"
+EXPECTED_LIST_FILE="$2"
+
+if [ ! -f "$RPM_FILE" ]; then
+ echo "Error: RPM file not found at '$RPM_FILE'"
+ exit 1
+fi
+
+if [ ! -f "$EXPECTED_LIST_FILE" ]; then
+ echo "Error: Expected list file not found at '$EXPECTED_LIST_FILE'"
+ exit 1
+fi
+
+ACTUAL_SORTED=$(mktemp)
+EXPECTED_SORTED=$(mktemp)
+
+trap 'rm -f "$ACTUAL_SORTED" "$EXPECTED_SORTED"' EXIT
+
+echo "--- Analyzing RPM package: $RPM_FILE ---"
+
+rpm -qlp "$RPM_FILE" | sort > "$ACTUAL_SORTED"
+sort "$EXPECTED_LIST_FILE" > "$EXPECTED_SORTED"
+
+DIFFERENCES=$(diff "$ACTUAL_SORTED" "$EXPECTED_SORTED")
Review Comment:
can this line be deleted?
--
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]
