You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2020/01/15 13:11:52 UTC

[GitHub] [nifi-minifi-cpp] bakaid opened a new pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

bakaid opened a new pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709
 
 
   Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
   
   In order to streamline the review of the contribution we ask you
   to ensure the following steps have been taken:
   
   ### For all changes:
   - [ ] Is there a JIRA ticket associated with this PR? Is it referenced
        in the commit message?
   
   - [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
   
   - [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
   
   - [ ] Is your initial contribution a single, squashed commit?
   
   ### For code changes:
   - [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
   - [ ] If applicable, have you updated the LICENSE file?
   - [ ] If applicable, have you updated the NOTICE file?
   
   ### For documentation related changes:
   - [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
   
   ### Note:
   Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on issue #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on issue #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#issuecomment-575036567
 
 
   I'll continue reviewing tomorrow.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on issue #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on issue #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#issuecomment-575544828
 
 
   Looks good. I'll test it.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367275409
 
 

 ##########
 File path: libminifi/src/utils/file/PathUtils.cpp
 ##########
 @@ -48,6 +56,35 @@ bool PathUtils::getFileNameAndPath(const std::string &path, std::string &filePat
   return true;
 }
 
+std::string PathUtils::getFullPath(const std::string& path) {
+#ifdef WIN32
+  std::vector<char> buffer(MAX_PATH);
+  uint32_t len = 0U;
+  while (true) {
+      len = GetFullPathNameA(path.c_str(), buffer.size(), buffer.data(), nullptr /*lpFilePart*/);
 
 Review comment:
   nit - - can be used GetFullPathName.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367357073
 
 

 ##########
 File path: libminifi/test/unit/FileUtilsTests.cpp
 ##########
 @@ -146,3 +148,33 @@ TEST_CASE("TestFileUtils::create_dir", "[TestCreateDir]") {
   REQUIRE(FileUtils::create_dir(test_dir_path) == 0);  // Dir already exists, success should be returned
   REQUIRE(FileUtils::delete_dir(test_dir_path) == 0);  // Delete should be successful as welll
 }
+
+TEST_CASE("TestFileUtils::getFullPath", "[TestGetFullPath]") {
+  TestController testController;
+
+  char format[] = "/tmp/gt.XXXXXX";
+  const std::string tempDir = utils::file::PathUtils::getFullPath(testController.createTempDirectory(format));
+
+  const std::string cwd = utils::Environment::getCurrentWorkingDirectory();
+
+  REQUIRE(utils::Environment::setCurrentWorkingDirectory(tempDir.c_str()));
+  utils::ScopeGuard cwdGuard([&cwd]() {
+    utils::Environment::setCurrentWorkingDirectory(cwd.c_str());
+  });
+
+  const std::string tempDir1 = utils::file::FileUtils::concat_path(tempDir, "test1");
+  const std::string tempDir2 = utils::file::FileUtils::concat_path(tempDir, "test2");
+  REQUIRE(0 == utils::file::FileUtils::create_dir(tempDir1));
+  REQUIRE(0 == utils::file::FileUtils::create_dir(tempDir2));
+
+  REQUIRE(tempDir1 == utils::file::PathUtils::getFullPath(tempDir1));
+  REQUIRE(tempDir1 == utils::file::PathUtils::getFullPath("test1"));
+  REQUIRE(tempDir1 == utils::file::PathUtils::getFullPath("./test1"));
+  REQUIRE(tempDir1 == utils::file::PathUtils::getFullPath("././test1"));
 
 Review comment:
   Is this expected to work on Win?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p edited a comment on issue #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p edited a comment on issue #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#issuecomment-576176249
 
 
   On Windows, I built installer, installed, server was running, log created under {installedDir}/Log. 
   LGTM.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367514370
 
 

 ##########
 File path: libminifi/include/core/logging/WindowsEventLogSink.h
 ##########
 @@ -0,0 +1,70 @@
+/**
+ *
+ * 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.
+ */
+
+#pragma once
+
+#ifdef WIN32
+
+#include "spdlog/common.h"
+#include "spdlog/sinks/base_sink.h"
+#include "spdlog/details/log_msg.h"
+#include "spdlog/details/null_mutex.h"
+
+#include <Windows.h>
+
+#include <string>
+
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+namespace internal {
+
+class windowseventlog_sink : public spdlog::sinks::base_sink<spdlog::details::null_mutex> {
+ private:
+  HANDLE event_source_;
+
+  WORD type_from_level(const spdlog::details::log_msg& msg) const;
+
+  protected:
+   void _sink_it(const spdlog::details::log_msg& msg) override;
+
+   void _flush() override;
+
+  public:
+   windowseventlog_sink(const std::string& source_name = "ApacheNiFiMiNiFi");
+
+   virtual ~windowseventlog_sink();
 
 Review comment:
   I shouldn't have used override in libminifi at all. I copied the class from spdlog's syslog sink, which used it this way. Will fix.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367390488
 
 

 ##########
 File path: main/MiNiFiMain.cpp
 ##########
 @@ -142,94 +159,39 @@ int main(int argc, char **argv) {
 #ifdef WIN32
 	if (!SetConsoleCtrlHandler(consoleSignalHandler, TRUE)) {
 		logger->log_error("Cannot install signal handler");
-		std::cerr << "Cannot install signal handler" << std::endl;
-		return 1;
+		return -1;
 	}
 
 	if (signal(SIGINT, sigHandler) == SIG_ERR || signal(SIGTERM, sigHandler) == SIG_ERR ) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+		logger->log_error("Cannot install signal handler");
 		return -1;
 	}
 #ifdef SIGBREAK
 	if (signal(SIGBREAK, sigHandler) == SIG_ERR) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+		logger->log_error("Cannot install signal handler");
 		return -1;
 	}
 #endif
 #else
 	if (signal(SIGINT, sigHandler) == SIG_ERR || signal(SIGTERM, sigHandler) == SIG_ERR || signal(SIGPIPE, SIG_IGN) == SIG_ERR) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+    logger->log_error("Cannot install signal handler");
 
 Review comment:
   That's what you get for editing in Notepad++ in Windows. Thanks.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367376504
 
 

 ##########
 File path: libminifi/test/unit/EnvironmentUtilsTests.cpp
 ##########
 @@ -0,0 +1,154 @@
+/**
+ *
+ * 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 "../TestBase.h"
+#include "utils/Environment.h"
+#include "utils/file/PathUtils.h"
+
+#include <thread>
+#include <random>
+#include <string>
+#include <cstdint>
+#include <vector>
+
+TEST_CASE("getenv already existing", "[getenv]") {
+  auto res = utils::Environment::getEnvironmentVariable("PATH");
+  REQUIRE(true == res.first);
+  REQUIRE(0 < res.second.length());
+}
+
+TEST_CASE("getenv not existing", "[getenv]") {
+  auto res = utils::Environment::getEnvironmentVariable("GETENV1");
+  REQUIRE(false == res.first);
+  REQUIRE("" == res.second);
+}
+
+TEST_CASE("getenv empty existing", "[getenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("GETENV2", ""));
+  auto res = utils::Environment::getEnvironmentVariable("GETENV2");
+  REQUIRE(true == res.first);
+  REQUIRE("" == res.second);
+}
+
+TEST_CASE("setenv not existing overwrite", "[setenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV1", "test"));
+  auto res = utils::Environment::getEnvironmentVariable("SETENV1");
+  REQUIRE(true == res.first);
+  REQUIRE("test" == res.second);
+}
+
+TEST_CASE("setenv existing overwrite", "[setenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV2", "test"));
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV2", "test2"));
+  auto res = utils::Environment::getEnvironmentVariable("SETENV2");
+  REQUIRE(true == res.first);
+  REQUIRE("test2" == res.second);
+}
+
+TEST_CASE("setenv not existing no overwrite", "[setenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV3", "test", false /*overwrite*/));
+  auto res = utils::Environment::getEnvironmentVariable("SETENV3");
+  REQUIRE(true == res.first);
+  REQUIRE("test" == res.second);
+}
+
+TEST_CASE("setenv existing no overwrite", "[setenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV4", "test"));
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV4", "test2", false /*overwrite*/));
+  auto res = utils::Environment::getEnvironmentVariable("SETENV4");
+  REQUIRE(true == res.first);
+  REQUIRE("test" == res.second);
+}
+
+TEST_CASE("unsetenv not existing", "[unsetenv]") {
+  REQUIRE(false == utils::Environment::getEnvironmentVariable("UNSETENV1").first);
+  REQUIRE(true == utils::Environment::unsetEnvironmentVariable("UNSETENV1"));
+  REQUIRE(false == utils::Environment::getEnvironmentVariable("UNSETENV1").first);
+}
+
+TEST_CASE("unsetenv existing", "[unsetenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("UNSETENV2", "test"));
+  REQUIRE(true == utils::Environment::getEnvironmentVariable("UNSETENV2").first);
+  REQUIRE(true == utils::Environment::unsetEnvironmentVariable("UNSETENV2"));
+  REQUIRE(false == utils::Environment::getEnvironmentVariable("UNSETENV2").first);
+}
+
+TEST_CASE("multithreaded environment manipulation", "[getenv][setenv][unsetenv]") {
+  std::vector<std::thread> threads;
+  for (size_t i = 0U; i < 16U; i++) {
+    threads.emplace_back([](){
+      std::mt19937 gen(std::random_device { }());
+      for (size_t i = 0U; i < 10240U; i++) {
+        const uint8_t env_num = gen() % 8;
+        const std::string env_name = "GETSETUNSETENV" + std::to_string(env_num);
+        const uint8_t operation = gen() % 3;
+        switch (operation) {
+          case 0: {
+              auto res = utils::Environment::getEnvironmentVariable(env_name.c_str());
+              break;
+            }
+          case 1: {
+              const size_t value_len = gen() % 256;
+              std::vector<char> value(value_len + 1, '\0');
+              std::generate_n(value.begin(), value_len, [&]() -> char {
+                return 'A' + gen() % static_cast<uint8_t>('Z' - 'A');
+              });
+              const bool overwrite = gen() % 2;
+              utils::Environment::setEnvironmentVariable(env_name.c_str(), value.data(), overwrite);
+              break;
+            }
+          case 2: {
+              utils::Environment::unsetEnvironmentVariable(env_name.c_str());
+              break;
+            }
+        }
+      }
+      });
+    }
+  for (auto& thread : threads) {
+    thread.join();
+  }
+  for (size_t i = 0U; i < 8U; i++) {
+    const std::string env_name = "GETSETUNSETENV" + std::to_string(i);
+    bool isset = false;
+    std::string value;
+    std::tie(isset, value) = utils::Environment::getEnvironmentVariable(env_name.c_str());
+    if (isset) {
+      std::cerr << env_name << " is set to " << value << std::endl;
 
 Review comment:
   Fair enough, thanks!

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367313244
 
 

 ##########
 File path: libminifi/src/core/logging/Logger.cpp
 ##########
 @@ -0,0 +1,149 @@
+/**
+ *
+ * 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 "core/logging/Logger.h"
+
+#include <mutex>
+#include <memory>
+#include <sstream>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+
+
+LoggerControl::LoggerControl()
+    : is_enabled_(true) {
+}
+
+bool LoggerControl::is_enabled() const {
+  return is_enabled_;
+}
+
+void LoggerControl::setEnabled(bool status) {
+  is_enabled_ = status;
+}
+
+
+BaseLogger::~BaseLogger() {
+}
+
+bool BaseLogger::should_log(const LOG_LEVEL &level) {
+  return true;
+}
+
+LogBuilder::LogBuilder(BaseLogger *l, LOG_LEVEL level)
+    : ignore(false),
+      ptr(l),
+      level(level) {
+  if (!l->should_log(level)) {
+    setIgnore();
+  }
+}
+
+LogBuilder::~LogBuilder() {
+  if (!ignore)
+    log_string(level);
+}
+
+void LogBuilder::setIgnore() {
+  ignore = true;
+}
+
+void LogBuilder::log_string(LOG_LEVEL level) {
+  ptr->log_string(level, str.str());
+}
+
+
+bool Logger::should_log(const LOG_LEVEL &level) {
+  if (controller_ && !controller_->is_enabled())
+    return false;
+  spdlog::level::level_enum logger_level = spdlog::level::level_enum::info;
+  switch (level) {
+    case critical:
+      logger_level = spdlog::level::level_enum::critical;
+      break;
+    case err:
+      logger_level = spdlog::level::level_enum::err;
+      break;
+    case info:
+      break;
 
 Review comment:
   Where did it go? :)

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

szaszm commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367488333
 
 

 ##########
 File path: libminifi/src/core/logging/WindowsEventLogSink.cpp
 ##########
 @@ -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.
+ */
+
+#ifdef WIN32
+
+#include "core/logging/WindowsEventLogsink.h"
+
+#include "core/logging/WindowsMessageTextFile.h"
+#include "Exception.h"
+
+#include "spdlog/common.h"
+#include "spdlog/sinks/sink.h"
+#include "spdlog/details/log_msg.h"
+
+#include <string>
+
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+namespace internal {
+
+WORD windowseventlog_sink::type_from_level(const spdlog::details::log_msg& msg) const {
+  switch (static_cast<int>(msg.level)) {
 
 Review comment:
   To avoid similar issues in the future, we could avoid having a `default` case and rely on compiler warnings for missing cases.
   https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#enum2-use-enumerations-to-represent-sets-of-related-named-constants

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on issue #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on issue #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#issuecomment-574657086
 
 
   @arpadboda This code cleans up the minifi main code, MINIFI_HOME determination and Properties a bit. It also adds more verbose and specific logging, and a log backend to log to the Windows Event Log which gets used in the Windows Service until the logger is configured from minifi-log.properties, so we have information about the MINIFI_HOME determination logic even when we are running as a service, and without the need to attach a debugger.
   
   @amarmer as this is a Windows-heavy PR, it would be nice if you could look at that parts.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367392987
 
 

 ##########
 File path: libminifi/src/utils/Environment.cpp
 ##########
 @@ -0,0 +1,192 @@
+/**
+ * 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 "utils/Environment.h"
+
+#ifdef WIN32
+#include <Windows.h>
+#else
+#include <cstdlib>
+#include <cerrno>
+#include <unistd.h>
+#endif
+#include <mutex>
+#include <vector>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+bool Environment::runningAsService_(false);
+
+void Environment::accessEnvironment(const std::function<void(void)>& func) {
+  static std::recursive_mutex environmentMutex;
+  std::lock_guard<std::recursive_mutex> lock(environmentMutex);
+  func();
+}
+
+std::pair<bool, std::string> Environment::getEnvironmentVariable(const char* name) {
+  bool exists = false;
+  std::string value;
+
+  Environment::accessEnvironment([&exists, &value, name](){
+#ifdef WIN32
+    std::vector<char> buffer(32767U);  // https://docs.microsoft.com/en-gb/windows/win32/api/processenv/nf-processenv-getenvironmentvariablea
+    // GetEnvironmentVariableA does not set last error to 0 on success, so an error from a pervious API call would influence the GetLastError() later,
+    // so we set the last error to 0 before calling
+    SetLastError(ERROR_SUCCESS);
+    uint32_t ret = GetEnvironmentVariableA(name, buffer.data(), buffer.size());
+    if (ret > 0U) {
+      exists = true;
+      value = std::string(buffer.data(), ret);
+    } else if (GetLastError() == ERROR_SUCCESS) {
+      // Exists, but empty
+      exists = true;
+    }
+#else
+    char* ret = getenv(name);
+    if (ret != nullptr) {
+      exists = true;
+      value = ret;
+    }
+#endif
+  });
+
+  return std::make_pair(exists, std::move(value));
+}
+
+bool Environment::setEnvironmentVariable(const char* name, const char* value, bool overwrite /*= true*/) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name, value, overwrite](){
+#ifdef WIN32
+    if (!overwrite && Environment::getEnvironmentVariable(name).first) {
+      success = true;
+    } else {
+      success = SetEnvironmentVariableA(name, value);
+    }
+#else
+    int ret = setenv(name, value, static_cast<int>(overwrite));
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+bool Environment::unsetEnvironmentVariable(const char* name) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name](){
+#ifdef WIN32
+    success = SetEnvironmentVariableA(name, nullptr);
+#else
+    int ret = unsetenv(name);
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+std::string Environment::getCurrentWorkingDirectory() {
+  std::string cwd;
+
+  Environment::accessEnvironment([&cwd](){
+#ifdef WIN32
+    uint32_t len = 0U;
+    std::vector<char> buffer;
+    // https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getcurrentdirectory
+    // "If the buffer that is pointed to by lpBuffer is not large enough,
+    // the return value specifies the required size of the buffer,
+    // in characters, including the null-terminating character."
+    while (true) {
+      len = GetCurrentDirectoryA(buffer.size(), buffer.data());
+      if (len > buffer.size()) {
+        buffer.resize(len);
+        continue;
+      } else {
+        break;
+      }
+    }
+    if (len > 0U) {
+      cwd = std::string(buffer.data(), len);
+    }
+#else
+    std::vector<char> buffer(1024U);
+    char* path = nullptr;
+    while (true) {
+      path = getcwd(buffer.data(), buffer.size());
+      if (path == nullptr) {
 
 Review comment:
   The original code reflected my mental reasoning, but this is more more concise, changed, thanks.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367310470
 
 

 ##########
 File path: libminifi/CMakeLists.txt
 ##########
 @@ -91,10 +91,38 @@ endif()
 
 file(GLOB SOURCES  "src/utils/file/*.cpp" "src/sitetosite/*.cpp"  "src/core/logging/*.cpp"  "src/core/state/*.cpp" "src/core/state/nodes/*.cpp" "src/c2/protocols/*.cpp" "src/c2/triggers/*.cpp" "src/c2/*.cpp" "src/io/*.cpp" ${SOCKET_SOURCES} ${TLS_SOURCES} "src/core/controller/*.cpp" "src/controllers/*.cpp" "src/core/*.cpp"  "src/core/repository/*.cpp" "src/core/yaml/*.cpp" "src/core/reporting/*.cpp"  "src/provenance/*.cpp" "src/utils/*.cpp" "src/*.cpp")
 
+if(WIN32)
+	if(CMAKE_SIZEOF_VOID_P EQUAL 8)
+		set(MC_COMPILER_SEARCH_PATH "$ENV{WindowsSdkVerBinPath}\\x64")
+	elseif(CMAKE_SIZEOF_VOID_P EQUAL 4)
+		set(MC_COMPILER_SEARCH_PATH "$ENV{WindowsSdkVerBinPath}\\x86")
 
 Review comment:
   Might be a dumb question, but I wonder what happens in case I'm to compile a 32 bit version even though my current system is 64 bit. ( Do we even support that? In case we don't, I wonder if we can rely on this and remote the corresponding parameter of the Windows bat file )

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367477412
 
 

 ##########
 File path: libminifi/src/core/logging/WindowsEventLogSink.cpp
 ##########
 @@ -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.
+ */
+
+#ifdef WIN32
+
+#include "core/logging/WindowsEventLogsink.h"
+
+#include "core/logging/WindowsMessageTextFile.h"
+#include "Exception.h"
+
+#include "spdlog/common.h"
+#include "spdlog/sinks/sink.h"
+#include "spdlog/details/log_msg.h"
+
+#include <string>
+
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+namespace internal {
+
+WORD windowseventlog_sink::type_from_level(const spdlog::details::log_msg& msg) const {
+  switch (static_cast<int>(msg.level)) {
 
 Review comment:
   As you said, we don't use spdlog::level::off, it is not well documented either what it should be mapped to, so I've decided to log all unexpected/potential future unknown enums at error log level.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

szaszm commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367520554
 
 

 ##########
 File path: libminifi/test/TestBase.cpp
 ##########
 @@ -18,6 +18,21 @@
 
 #include "./TestBase.h"
 
+#include "spdlog/spdlog.h"
+
+void LogTestController::setLevel(const std::string name, spdlog::level::level_enum level) {
+  logger_->log_info("Setting log level for %s to %s", name, spdlog::level::to_str(level));
+  std::string adjusted_name = name;
+  const std::string clazz = "class ";
+  auto haz_clazz = name.find(clazz);
+  if (haz_clazz == 0)
+    adjusted_name = name.substr(clazz.length(), name.length() - clazz.length());
+  if (config && config->shortenClassNames()) {
+    utils::ClassUtils::shortenClassName(adjusted_name, adjusted_name);
+  }
 
 Review comment:
   I didn't realize that it's only been moved. Nevermind then.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

szaszm commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367481345
 
 

 ##########
 File path: libminifi/include/core/logging/WindowsEventLogSink.h
 ##########
 @@ -0,0 +1,70 @@
+/**
+ *
+ * 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.
+ */
+
+#pragma once
+
+#ifdef WIN32
+
+#include "spdlog/common.h"
+#include "spdlog/sinks/base_sink.h"
+#include "spdlog/details/log_msg.h"
+#include "spdlog/details/null_mutex.h"
+
+#include <Windows.h>
+
+#include <string>
+
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+namespace internal {
+
+class windowseventlog_sink : public spdlog::sinks::base_sink<spdlog::details::null_mutex> {
+ private:
+  HANDLE event_source_;
+
+  WORD type_from_level(const spdlog::details::log_msg& msg) const;
+
+  protected:
+   void _sink_it(const spdlog::details::log_msg& msg) override;
+
+   void _flush() override;
+
+  public:
+   windowseventlog_sink(const std::string& source_name = "ApacheNiFiMiNiFi");
+
+   virtual ~windowseventlog_sink();
 
 Review comment:
   You used `override` on `_flush` and `_sink_it`, but not on the dtor. Why?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367268047
 
 

 ##########
 File path: libminifi/src/utils/Environment.cpp
 ##########
 @@ -0,0 +1,192 @@
+/**
+ * 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 "utils/Environment.h"
+
+#ifdef WIN32
+#include <Windows.h>
+#else
+#include <cstdlib>
+#include <cerrno>
+#include <unistd.h>
+#endif
+#include <mutex>
+#include <vector>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+bool Environment::runningAsService_(false);
+
+void Environment::accessEnvironment(const std::function<void(void)>& func) {
+  static std::recursive_mutex environmentMutex;
+  std::lock_guard<std::recursive_mutex> lock(environmentMutex);
+  func();
+}
+
+std::pair<bool, std::string> Environment::getEnvironmentVariable(const char* name) {
+  bool exists = false;
+  std::string value;
+
+  Environment::accessEnvironment([&exists, &value, name](){
+#ifdef WIN32
+    std::vector<char> buffer(32767U);  // https://docs.microsoft.com/en-gb/windows/win32/api/processenv/nf-processenv-getenvironmentvariablea
+    // GetEnvironmentVariableA does not set last error to 0 on success, so an error from a pervious API call would influence the GetLastError() later,
+    // so we set the last error to 0 before calling
+    SetLastError(ERROR_SUCCESS);
+    uint32_t ret = GetEnvironmentVariableA(name, buffer.data(), buffer.size());
+    if (ret > 0U) {
+      exists = true;
+      value = std::string(buffer.data(), ret);
+    } else if (GetLastError() == ERROR_SUCCESS) {
+      // Exists, but empty
+      exists = true;
+    }
+#else
+    char* ret = getenv(name);
+    if (ret != nullptr) {
+      exists = true;
+      value = ret;
+    }
+#endif
+  });
+
+  return std::make_pair(exists, std::move(value));
+}
+
+bool Environment::setEnvironmentVariable(const char* name, const char* value, bool overwrite /*= true*/) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name, value, overwrite](){
+#ifdef WIN32
+    if (!overwrite && Environment::getEnvironmentVariable(name).first) {
+      success = true;
+    } else {
+      success = SetEnvironmentVariableA(name, value);
+    }
+#else
+    int ret = setenv(name, value, static_cast<int>(overwrite));
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+bool Environment::unsetEnvironmentVariable(const char* name) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name](){
+#ifdef WIN32
+    success = SetEnvironmentVariableA(name, nullptr);
 
 Review comment:
   nit - can used SetEnvironmentVariable.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367513962
 
 

 ##########
 File path: libminifi/include/core/logging/WindowsEventLogSink.h
 ##########
 @@ -0,0 +1,70 @@
+/**
+ *
+ * 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.
+ */
+
+#pragma once
+
+#ifdef WIN32
+
+#include "spdlog/common.h"
+#include "spdlog/sinks/base_sink.h"
+#include "spdlog/details/log_msg.h"
+#include "spdlog/details/null_mutex.h"
+
+#include <Windows.h>
+
+#include <string>
+
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+namespace internal {
+
+class windowseventlog_sink : public spdlog::sinks::base_sink<spdlog::details::null_mutex> {
+ private:
+  HANDLE event_source_;
+
+  WORD type_from_level(const spdlog::details::log_msg& msg) const;
+
+  protected:
+   void _sink_it(const spdlog::details::log_msg& msg) override;
+
+   void _flush() override;
+
+  public:
+   windowseventlog_sink(const std::string& source_name = "ApacheNiFiMiNiFi");
+
+   virtual ~windowseventlog_sink();
+
+   windowseventlog_sink(const windowseventlog_sink&) = delete;
+   windowseventlog_sink& operator=(const windowseventlog_sink&) = delete;
+   windowseventlog_sink(windowseventlog_sink&&) = delete;
+   windowseventlog_sink& operator=(windowseventlog_sink&&) = delete;
+};
+}
+}
+}
+}
+}
+}
+}
 
 Review comment:
   Good catch, will fix.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] arpadboda closed pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

arpadboda closed pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709
 
 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367274461
 
 

 ##########
 File path: libminifi/src/utils/file/PathUtils.cpp
 ##########
 @@ -48,6 +56,35 @@ bool PathUtils::getFileNameAndPath(const std::string &path, std::string &filePat
   return true;
 }
 
+std::string PathUtils::getFullPath(const std::string& path) {
+#ifdef WIN32
+  std::vector<char> buffer(MAX_PATH);
 
 Review comment:
   MAX_PATH just 260, could be used `char buffer[MAX_PATH+1];`

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367516333
 
 

 ##########
 File path: libminifi/src/core/logging/WindowsEventLogSink.cpp
 ##########
 @@ -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.
+ */
+
+#ifdef WIN32
+
+#include "core/logging/WindowsEventLogsink.h"
+
+#include "core/logging/WindowsMessageTextFile.h"
+#include "Exception.h"
+
+#include "spdlog/common.h"
+#include "spdlog/sinks/sink.h"
+#include "spdlog/details/log_msg.h"
+
+#include <string>
+
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+namespace internal {
+
+WORD windowseventlog_sink::type_from_level(const spdlog::details::log_msg& msg) const {
+  switch (static_cast<int>(msg.level)) {
 
 Review comment:
   Completely agree, I prefer that solution as well, but we should clean up our warnings a bit first, because currently we are not warning free even with our current set of warnings, and any new warning would be most likely lost in the current ones. I wouldn't put this on -Werror (or only just in libminifi), because that could break third party extensions.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367268177
 
 

 ##########
 File path: libminifi/src/utils/Environment.cpp
 ##########
 @@ -0,0 +1,192 @@
+/**
+ * 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 "utils/Environment.h"
+
+#ifdef WIN32
+#include <Windows.h>
+#else
+#include <cstdlib>
+#include <cerrno>
+#include <unistd.h>
+#endif
+#include <mutex>
+#include <vector>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+bool Environment::runningAsService_(false);
+
+void Environment::accessEnvironment(const std::function<void(void)>& func) {
+  static std::recursive_mutex environmentMutex;
+  std::lock_guard<std::recursive_mutex> lock(environmentMutex);
+  func();
+}
+
+std::pair<bool, std::string> Environment::getEnvironmentVariable(const char* name) {
+  bool exists = false;
+  std::string value;
+
+  Environment::accessEnvironment([&exists, &value, name](){
+#ifdef WIN32
+    std::vector<char> buffer(32767U);  // https://docs.microsoft.com/en-gb/windows/win32/api/processenv/nf-processenv-getenvironmentvariablea
+    // GetEnvironmentVariableA does not set last error to 0 on success, so an error from a pervious API call would influence the GetLastError() later,
+    // so we set the last error to 0 before calling
+    SetLastError(ERROR_SUCCESS);
+    uint32_t ret = GetEnvironmentVariableA(name, buffer.data(), buffer.size());
+    if (ret > 0U) {
+      exists = true;
+      value = std::string(buffer.data(), ret);
+    } else if (GetLastError() == ERROR_SUCCESS) {
+      // Exists, but empty
+      exists = true;
+    }
+#else
+    char* ret = getenv(name);
+    if (ret != nullptr) {
+      exists = true;
+      value = ret;
+    }
+#endif
+  });
+
+  return std::make_pair(exists, std::move(value));
+}
+
+bool Environment::setEnvironmentVariable(const char* name, const char* value, bool overwrite /*= true*/) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name, value, overwrite](){
+#ifdef WIN32
+    if (!overwrite && Environment::getEnvironmentVariable(name).first) {
+      success = true;
+    } else {
+      success = SetEnvironmentVariableA(name, value);
+    }
+#else
+    int ret = setenv(name, value, static_cast<int>(overwrite));
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+bool Environment::unsetEnvironmentVariable(const char* name) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name](){
+#ifdef WIN32
+    success = SetEnvironmentVariableA(name, nullptr);
+#else
+    int ret = unsetenv(name);
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+std::string Environment::getCurrentWorkingDirectory() {
+  std::string cwd;
+
+  Environment::accessEnvironment([&cwd](){
+#ifdef WIN32
+    uint32_t len = 0U;
+    std::vector<char> buffer;
+    // https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getcurrentdirectory
+    // "If the buffer that is pointed to by lpBuffer is not large enough,
+    // the return value specifies the required size of the buffer,
+    // in characters, including the null-terminating character."
+    while (true) {
+      len = GetCurrentDirectoryA(buffer.size(), buffer.data());
 
 Review comment:
   nit - can be used GetCurrentDirectory.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on issue #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on issue #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#issuecomment-575147378
 
 
   @arpadboda Thanks for the review, addressed your review comments, also updated the MessageCompiler finder cmake magic, let's wait for the AppVeyor to see if it works now.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367331713
 
 

 ##########
 File path: libminifi/src/utils/Environment.cpp
 ##########
 @@ -0,0 +1,192 @@
+/**
+ * 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 "utils/Environment.h"
+
+#ifdef WIN32
+#include <Windows.h>
+#else
+#include <cstdlib>
+#include <cerrno>
+#include <unistd.h>
+#endif
+#include <mutex>
+#include <vector>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+bool Environment::runningAsService_(false);
+
+void Environment::accessEnvironment(const std::function<void(void)>& func) {
+  static std::recursive_mutex environmentMutex;
+  std::lock_guard<std::recursive_mutex> lock(environmentMutex);
+  func();
+}
+
+std::pair<bool, std::string> Environment::getEnvironmentVariable(const char* name) {
+  bool exists = false;
+  std::string value;
+
+  Environment::accessEnvironment([&exists, &value, name](){
+#ifdef WIN32
+    std::vector<char> buffer(32767U);  // https://docs.microsoft.com/en-gb/windows/win32/api/processenv/nf-processenv-getenvironmentvariablea
+    // GetEnvironmentVariableA does not set last error to 0 on success, so an error from a pervious API call would influence the GetLastError() later,
+    // so we set the last error to 0 before calling
+    SetLastError(ERROR_SUCCESS);
+    uint32_t ret = GetEnvironmentVariableA(name, buffer.data(), buffer.size());
+    if (ret > 0U) {
+      exists = true;
+      value = std::string(buffer.data(), ret);
+    } else if (GetLastError() == ERROR_SUCCESS) {
+      // Exists, but empty
+      exists = true;
+    }
+#else
+    char* ret = getenv(name);
+    if (ret != nullptr) {
+      exists = true;
+      value = ret;
+    }
+#endif
+  });
+
+  return std::make_pair(exists, std::move(value));
+}
+
+bool Environment::setEnvironmentVariable(const char* name, const char* value, bool overwrite /*= true*/) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name, value, overwrite](){
+#ifdef WIN32
+    if (!overwrite && Environment::getEnvironmentVariable(name).first) {
+      success = true;
+    } else {
+      success = SetEnvironmentVariableA(name, value);
+    }
+#else
+    int ret = setenv(name, value, static_cast<int>(overwrite));
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+bool Environment::unsetEnvironmentVariable(const char* name) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name](){
+#ifdef WIN32
+    success = SetEnvironmentVariableA(name, nullptr);
+#else
+    int ret = unsetenv(name);
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+std::string Environment::getCurrentWorkingDirectory() {
+  std::string cwd;
+
+  Environment::accessEnvironment([&cwd](){
+#ifdef WIN32
+    uint32_t len = 0U;
+    std::vector<char> buffer;
+    // https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getcurrentdirectory
+    // "If the buffer that is pointed to by lpBuffer is not large enough,
+    // the return value specifies the required size of the buffer,
+    // in characters, including the null-terminating character."
+    while (true) {
+      len = GetCurrentDirectoryA(buffer.size(), buffer.data());
+      if (len > buffer.size()) {
+        buffer.resize(len);
+        continue;
+      } else {
+        break;
+      }
+    }
+    if (len > 0U) {
+      cwd = std::string(buffer.data(), len);
+    }
+#else
+    std::vector<char> buffer(1024U);
+    char* path = nullptr;
+    while (true) {
+      path = getcwd(buffer.data(), buffer.size());
+      if (path == nullptr) {
 
 Review comment:
   This seems to be a bit overcomplicated
   
   ```    
       while (true) {
         path = getcwd(buffer.data(), buffer.size());
         if (path != nullptr) {
           cwd = path;
           break;
         } else if (errno == ERANGE) {
           buffer.resize(buffer.size() * 2);
         } else {
           break;  // error
         }
      }
   #endif
   ```

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367336311
 
 

 ##########
 File path: libminifi/src/core/logging/Logger.cpp
 ##########
 @@ -0,0 +1,149 @@
+/**
+ *
+ * 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 "core/logging/Logger.h"
+
+#include <mutex>
+#include <memory>
+#include <sstream>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+
+
+LoggerControl::LoggerControl()
+    : is_enabled_(true) {
+}
+
+bool LoggerControl::is_enabled() const {
+  return is_enabled_;
+}
+
+void LoggerControl::setEnabled(bool status) {
+  is_enabled_ = status;
+}
+
+
+BaseLogger::~BaseLogger() {
+}
+
+bool BaseLogger::should_log(const LOG_LEVEL &level) {
+  return true;
+}
+
+LogBuilder::LogBuilder(BaseLogger *l, LOG_LEVEL level)
+    : ignore(false),
+      ptr(l),
+      level(level) {
+  if (!l->should_log(level)) {
+    setIgnore();
+  }
+}
+
+LogBuilder::~LogBuilder() {
+  if (!ignore)
+    log_string(level);
+}
+
+void LogBuilder::setIgnore() {
+  ignore = true;
+}
+
+void LogBuilder::log_string(LOG_LEVEL level) {
+  ptr->log_string(level, str.str());
+}
+
+
+bool Logger::should_log(const LOG_LEVEL &level) {
+  if (controller_ && !controller_->is_enabled())
+    return false;
+  spdlog::level::level_enum logger_level = spdlog::level::level_enum::info;
+  switch (level) {
+    case critical:
+      logger_level = spdlog::level::level_enum::critical;
+      break;
+    case err:
+      logger_level = spdlog::level::level_enum::err;
+      break;
+    case info:
+      break;
+    case debug:
+      logger_level = spdlog::level::level_enum::debug;
+      break;
+    case off:
+      logger_level = spdlog::level::level_enum::off;
+      break;
+    case trace:
+      logger_level = spdlog::level::level_enum::trace;
+      break;
+    case warn:
+      logger_level = spdlog::level::level_enum::warn;
+      break;
+  }
+
+  std::lock_guard<std::mutex> lock(mutex_);
+  if (!delegate_->should_log(logger_level)) {
 
 Review comment:
   Agreed, although this is not new code.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367267576
 
 

 ##########
 File path: libminifi/src/utils/Environment.cpp
 ##########
 @@ -0,0 +1,192 @@
+/**
+ * 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 "utils/Environment.h"
+
+#ifdef WIN32
+#include <Windows.h>
+#else
+#include <cstdlib>
+#include <cerrno>
+#include <unistd.h>
+#endif
+#include <mutex>
+#include <vector>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+bool Environment::runningAsService_(false);
+
+void Environment::accessEnvironment(const std::function<void(void)>& func) {
+  static std::recursive_mutex environmentMutex;
+  std::lock_guard<std::recursive_mutex> lock(environmentMutex);
+  func();
+}
+
+std::pair<bool, std::string> Environment::getEnvironmentVariable(const char* name) {
+  bool exists = false;
+  std::string value;
+
+  Environment::accessEnvironment([&exists, &value, name](){
+#ifdef WIN32
+    std::vector<char> buffer(32767U);  // https://docs.microsoft.com/en-gb/windows/win32/api/processenv/nf-processenv-getenvironmentvariablea
+    // GetEnvironmentVariableA does not set last error to 0 on success, so an error from a pervious API call would influence the GetLastError() later,
+    // so we set the last error to 0 before calling
+    SetLastError(ERROR_SUCCESS);
+    uint32_t ret = GetEnvironmentVariableA(name, buffer.data(), buffer.size());
 
 Review comment:
   nit - can be used GetEnvironmentVariable.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367267981
 
 

 ##########
 File path: libminifi/src/utils/Environment.cpp
 ##########
 @@ -0,0 +1,192 @@
+/**
+ * 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 "utils/Environment.h"
+
+#ifdef WIN32
+#include <Windows.h>
+#else
+#include <cstdlib>
+#include <cerrno>
+#include <unistd.h>
+#endif
+#include <mutex>
+#include <vector>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+bool Environment::runningAsService_(false);
+
+void Environment::accessEnvironment(const std::function<void(void)>& func) {
+  static std::recursive_mutex environmentMutex;
+  std::lock_guard<std::recursive_mutex> lock(environmentMutex);
+  func();
+}
+
+std::pair<bool, std::string> Environment::getEnvironmentVariable(const char* name) {
+  bool exists = false;
+  std::string value;
+
+  Environment::accessEnvironment([&exists, &value, name](){
+#ifdef WIN32
+    std::vector<char> buffer(32767U);  // https://docs.microsoft.com/en-gb/windows/win32/api/processenv/nf-processenv-getenvironmentvariablea
+    // GetEnvironmentVariableA does not set last error to 0 on success, so an error from a pervious API call would influence the GetLastError() later,
+    // so we set the last error to 0 before calling
+    SetLastError(ERROR_SUCCESS);
+    uint32_t ret = GetEnvironmentVariableA(name, buffer.data(), buffer.size());
+    if (ret > 0U) {
+      exists = true;
+      value = std::string(buffer.data(), ret);
+    } else if (GetLastError() == ERROR_SUCCESS) {
+      // Exists, but empty
+      exists = true;
+    }
+#else
+    char* ret = getenv(name);
+    if (ret != nullptr) {
+      exists = true;
+      value = ret;
+    }
+#endif
+  });
+
+  return std::make_pair(exists, std::move(value));
+}
+
+bool Environment::setEnvironmentVariable(const char* name, const char* value, bool overwrite /*= true*/) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name, value, overwrite](){
+#ifdef WIN32
+    if (!overwrite && Environment::getEnvironmentVariable(name).first) {
+      success = true;
+    } else {
+      success = SetEnvironmentVariableA(name, value);
 
 Review comment:
   nit - can used SetEnvironmentVariable.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367273019
 
 

 ##########
 File path: libminifi/src/utils/Environment.cpp
 ##########
 @@ -0,0 +1,192 @@
+/**
+ * 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 "utils/Environment.h"
+
+#ifdef WIN32
+#include <Windows.h>
+#else
+#include <cstdlib>
+#include <cerrno>
+#include <unistd.h>
+#endif
+#include <mutex>
+#include <vector>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+bool Environment::runningAsService_(false);
+
+void Environment::accessEnvironment(const std::function<void(void)>& func) {
+  static std::recursive_mutex environmentMutex;
+  std::lock_guard<std::recursive_mutex> lock(environmentMutex);
+  func();
+}
+
+std::pair<bool, std::string> Environment::getEnvironmentVariable(const char* name) {
+  bool exists = false;
+  std::string value;
+
+  Environment::accessEnvironment([&exists, &value, name](){
+#ifdef WIN32
+    std::vector<char> buffer(32767U);  // https://docs.microsoft.com/en-gb/windows/win32/api/processenv/nf-processenv-getenvironmentvariablea
+    // GetEnvironmentVariableA does not set last error to 0 on success, so an error from a pervious API call would influence the GetLastError() later,
+    // so we set the last error to 0 before calling
+    SetLastError(ERROR_SUCCESS);
+    uint32_t ret = GetEnvironmentVariableA(name, buffer.data(), buffer.size());
+    if (ret > 0U) {
+      exists = true;
+      value = std::string(buffer.data(), ret);
+    } else if (GetLastError() == ERROR_SUCCESS) {
+      // Exists, but empty
+      exists = true;
+    }
+#else
+    char* ret = getenv(name);
+    if (ret != nullptr) {
+      exists = true;
+      value = ret;
+    }
+#endif
+  });
+
+  return std::make_pair(exists, std::move(value));
+}
+
+bool Environment::setEnvironmentVariable(const char* name, const char* value, bool overwrite /*= true*/) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name, value, overwrite](){
+#ifdef WIN32
+    if (!overwrite && Environment::getEnvironmentVariable(name).first) {
+      success = true;
+    } else {
+      success = SetEnvironmentVariableA(name, value);
+    }
+#else
+    int ret = setenv(name, value, static_cast<int>(overwrite));
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+bool Environment::unsetEnvironmentVariable(const char* name) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name](){
+#ifdef WIN32
+    success = SetEnvironmentVariableA(name, nullptr);
+#else
+    int ret = unsetenv(name);
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+std::string Environment::getCurrentWorkingDirectory() {
+  std::string cwd;
+
+  Environment::accessEnvironment([&cwd](){
+#ifdef WIN32
+    uint32_t len = 0U;
 
 Review comment:
   This works, but simpler would be:
   char dir[MAX_PATH+1] ;
   GetCurrentDirectory( sizeof(dir), dir ) ;
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r366867563
 
 

 ##########
 File path: main/MiNiFiMain.cpp
 ##########
 @@ -142,94 +159,39 @@ int main(int argc, char **argv) {
 #ifdef WIN32
 	if (!SetConsoleCtrlHandler(consoleSignalHandler, TRUE)) {
 		logger->log_error("Cannot install signal handler");
-		std::cerr << "Cannot install signal handler" << std::endl;
-		return 1;
+		return -1;
 	}
 
 	if (signal(SIGINT, sigHandler) == SIG_ERR || signal(SIGTERM, sigHandler) == SIG_ERR ) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+		logger->log_error("Cannot install signal handler");
 		return -1;
 	}
 #ifdef SIGBREAK
 	if (signal(SIGBREAK, sigHandler) == SIG_ERR) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+		logger->log_error("Cannot install signal handler");
 		return -1;
 	}
 #endif
 #else
 	if (signal(SIGINT, sigHandler) == SIG_ERR || signal(SIGTERM, sigHandler) == SIG_ERR || signal(SIGPIPE, SIG_IGN) == SIG_ERR) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+    logger->log_error("Cannot install signal handler");
 		return -1;
 	}
 #endif
-	// assumes POSIX compliant environment
-	std::string minifiHome;
-	if (const char *env_p = std::getenv(MINIFI_HOME_ENV_KEY)) {
-		minifiHome = env_p;
-		logger->log_info("Using MINIFI_HOME=%s from environment.", minifiHome);
-	}
-	else {
-		logger->log_info("MINIFI_HOME is not set; determining based on environment.");
-		char *path = nullptr;
-		char full_path[PATH_MAX];
-#ifndef WIN32
-		path = realpath(argv[0], full_path);
-#else
-		path = nullptr;
-#endif
-
-		if (path != nullptr) {
-			std::string minifiHomePath(path);
-			if (minifiHomePath.find_last_of("/\\") != std::string::npos) {
-				minifiHomePath = minifiHomePath.substr(0, minifiHomePath.find_last_of("/\\"));  //Remove /minifi from path
-				minifiHome = minifiHomePath.substr(0, minifiHomePath.find_last_of("/\\"));    //Remove /bin from path
-			}
-		}
-
-		// attempt to use cwd as MINIFI_HOME
-		if (minifiHome.empty() || !validHome(minifiHome)) {
-			char cwd[PATH_MAX];
-#ifdef WIN32
-			_getcwd(cwd, PATH_MAX);
-			auto handle = GetModuleHandle(0);
-			GetModuleFileNameA(NULL, cwd, sizeof(cwd));
-			std::string fullPath = cwd;
-			std::string minifiFileName, minifiPath;
-			minifi::utils::file::PathUtils::getFileNameAndPath(fullPath, minifiPath, minifiFileName);
-			if (utils::StringUtils::endsWith(minifiPath, "bin")) {
-				minifiHome = minifiPath.substr(0, minifiPath.size()-3);
-			}
-			else {
-				minifiHome = minifiPath;
-			}
-		
-#else
-			getcwd(cwd, PATH_MAX);
-			minifiHome = cwd;
-#endif
-			
-		}
-
 
-		logger->log_debug("Setting %s to %s", MINIFI_HOME_ENV_KEY, minifiHome);
-#ifdef WIN32
-		SetEnvironmentVariable(MINIFI_HOME_ENV_KEY, minifiHome.c_str());
-#else
-		setenv(MINIFI_HOME_ENV_KEY, minifiHome.c_str(), 0);
-#endif
+	// Determine MINIFI_HOME
+  const std::string minifiHome = determineMinifiHome(logger);
+	if (minifiHome.empty()) {
+	  // determineMinifiHome already logged everything we need
+	  return -1;
 	}
 
-	if (!validHome(minifiHome)) {
-		minifiHome = minifiHome.substr(0, minifiHome.find_last_of("/\\"));    //Remove /bin from path
-		if (!validHome(minifiHome)) {
-			logger->log_error("No valid MINIFI_HOME could be inferred. "
-				"Please set MINIFI_HOME or run minifi from a valid location. minifiHome is %s", minifiHome);
-			return -1;
-		}
+	// chdir to MINIFI_HOME
+	if (!utils::Environment::setCurrentWorkingDirectory(minifiHome.c_str())) {
 
 Review comment:
   This will solve the problem of relative paths being relative to the cwd, instead of MINIFI_HOME. A lot of code has been written under the assumption that this is true, but with the Windows service case it is explicitly not (the default working directory is C:\Windows\System32).
   After looking through the code, I think this should not break anything, but please take a minute to think it through this as well.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367268177
 
 

 ##########
 File path: libminifi/src/utils/Environment.cpp
 ##########
 @@ -0,0 +1,192 @@
+/**
+ * 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 "utils/Environment.h"
+
+#ifdef WIN32
+#include <Windows.h>
+#else
+#include <cstdlib>
+#include <cerrno>
+#include <unistd.h>
+#endif
+#include <mutex>
+#include <vector>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+bool Environment::runningAsService_(false);
+
+void Environment::accessEnvironment(const std::function<void(void)>& func) {
+  static std::recursive_mutex environmentMutex;
+  std::lock_guard<std::recursive_mutex> lock(environmentMutex);
+  func();
+}
+
+std::pair<bool, std::string> Environment::getEnvironmentVariable(const char* name) {
+  bool exists = false;
+  std::string value;
+
+  Environment::accessEnvironment([&exists, &value, name](){
+#ifdef WIN32
+    std::vector<char> buffer(32767U);  // https://docs.microsoft.com/en-gb/windows/win32/api/processenv/nf-processenv-getenvironmentvariablea
+    // GetEnvironmentVariableA does not set last error to 0 on success, so an error from a pervious API call would influence the GetLastError() later,
+    // so we set the last error to 0 before calling
+    SetLastError(ERROR_SUCCESS);
+    uint32_t ret = GetEnvironmentVariableA(name, buffer.data(), buffer.size());
+    if (ret > 0U) {
+      exists = true;
+      value = std::string(buffer.data(), ret);
+    } else if (GetLastError() == ERROR_SUCCESS) {
+      // Exists, but empty
+      exists = true;
+    }
+#else
+    char* ret = getenv(name);
+    if (ret != nullptr) {
+      exists = true;
+      value = ret;
+    }
+#endif
+  });
+
+  return std::make_pair(exists, std::move(value));
+}
+
+bool Environment::setEnvironmentVariable(const char* name, const char* value, bool overwrite /*= true*/) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name, value, overwrite](){
+#ifdef WIN32
+    if (!overwrite && Environment::getEnvironmentVariable(name).first) {
+      success = true;
+    } else {
+      success = SetEnvironmentVariableA(name, value);
+    }
+#else
+    int ret = setenv(name, value, static_cast<int>(overwrite));
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+bool Environment::unsetEnvironmentVariable(const char* name) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name](){
+#ifdef WIN32
+    success = SetEnvironmentVariableA(name, nullptr);
+#else
+    int ret = unsetenv(name);
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+std::string Environment::getCurrentWorkingDirectory() {
+  std::string cwd;
+
+  Environment::accessEnvironment([&cwd](){
+#ifdef WIN32
+    uint32_t len = 0U;
+    std::vector<char> buffer;
+    // https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getcurrentdirectory
+    // "If the buffer that is pointed to by lpBuffer is not large enough,
+    // the return value specifies the required size of the buffer,
+    // in characters, including the null-terminating character."
+    while (true) {
+      len = GetCurrentDirectoryA(buffer.size(), buffer.data());
 
 Review comment:
   nit - can be used GetCurrentDirectory.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367268406
 
 

 ##########
 File path: libminifi/src/utils/Environment.cpp
 ##########
 @@ -0,0 +1,192 @@
+/**
+ * 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 "utils/Environment.h"
+
+#ifdef WIN32
+#include <Windows.h>
+#else
+#include <cstdlib>
+#include <cerrno>
+#include <unistd.h>
+#endif
+#include <mutex>
+#include <vector>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+bool Environment::runningAsService_(false);
+
+void Environment::accessEnvironment(const std::function<void(void)>& func) {
+  static std::recursive_mutex environmentMutex;
+  std::lock_guard<std::recursive_mutex> lock(environmentMutex);
+  func();
+}
+
+std::pair<bool, std::string> Environment::getEnvironmentVariable(const char* name) {
+  bool exists = false;
+  std::string value;
+
+  Environment::accessEnvironment([&exists, &value, name](){
+#ifdef WIN32
+    std::vector<char> buffer(32767U);  // https://docs.microsoft.com/en-gb/windows/win32/api/processenv/nf-processenv-getenvironmentvariablea
+    // GetEnvironmentVariableA does not set last error to 0 on success, so an error from a pervious API call would influence the GetLastError() later,
+    // so we set the last error to 0 before calling
+    SetLastError(ERROR_SUCCESS);
+    uint32_t ret = GetEnvironmentVariableA(name, buffer.data(), buffer.size());
+    if (ret > 0U) {
+      exists = true;
+      value = std::string(buffer.data(), ret);
+    } else if (GetLastError() == ERROR_SUCCESS) {
+      // Exists, but empty
+      exists = true;
+    }
+#else
+    char* ret = getenv(name);
+    if (ret != nullptr) {
+      exists = true;
+      value = ret;
+    }
+#endif
+  });
+
+  return std::make_pair(exists, std::move(value));
+}
+
+bool Environment::setEnvironmentVariable(const char* name, const char* value, bool overwrite /*= true*/) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name, value, overwrite](){
+#ifdef WIN32
+    if (!overwrite && Environment::getEnvironmentVariable(name).first) {
+      success = true;
+    } else {
+      success = SetEnvironmentVariableA(name, value);
+    }
+#else
+    int ret = setenv(name, value, static_cast<int>(overwrite));
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+bool Environment::unsetEnvironmentVariable(const char* name) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name](){
+#ifdef WIN32
+    success = SetEnvironmentVariableA(name, nullptr);
+#else
+    int ret = unsetenv(name);
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+std::string Environment::getCurrentWorkingDirectory() {
+  std::string cwd;
+
+  Environment::accessEnvironment([&cwd](){
+#ifdef WIN32
+    uint32_t len = 0U;
+    std::vector<char> buffer;
+    // https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getcurrentdirectory
+    // "If the buffer that is pointed to by lpBuffer is not large enough,
+    // the return value specifies the required size of the buffer,
+    // in characters, including the null-terminating character."
+    while (true) {
+      len = GetCurrentDirectoryA(buffer.size(), buffer.data());
+      if (len > buffer.size()) {
+        buffer.resize(len);
+        continue;
+      } else {
+        break;
+      }
+    }
+    if (len > 0U) {
+      cwd = std::string(buffer.data(), len);
+    }
+#else
+    std::vector<char> buffer(1024U);
+    char* path = nullptr;
+    while (true) {
+      path = getcwd(buffer.data(), buffer.size());
+      if (path == nullptr) {
+        if (errno == ERANGE) {
+          buffer.resize(buffer.size() * 2);
+          continue;
+        } else {
+          break;
+        }
+      } else {
+        break;
+      }
+    }
+    if (path != nullptr) {
+      cwd = path;
+    }
+#endif
+  });
+
+  return cwd;
+}
+
+bool Environment::setCurrentWorkingDirectory(const char* directory) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, directory](){
+#ifdef WIN32
+    success = SetCurrentDirectoryA(directory);
 
 Review comment:
   nit - can be used SetCurrentDirectory.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367336039
 
 

 ##########
 File path: libminifi/src/core/logging/Logger.cpp
 ##########
 @@ -0,0 +1,149 @@
+/**
+ *
+ * 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 "core/logging/Logger.h"
+
+#include <mutex>
+#include <memory>
+#include <sstream>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+
+
+LoggerControl::LoggerControl()
+    : is_enabled_(true) {
+}
+
+bool LoggerControl::is_enabled() const {
+  return is_enabled_;
+}
+
+void LoggerControl::setEnabled(bool status) {
+  is_enabled_ = status;
+}
+
+
+BaseLogger::~BaseLogger() {
+}
+
+bool BaseLogger::should_log(const LOG_LEVEL &level) {
+  return true;
+}
+
+LogBuilder::LogBuilder(BaseLogger *l, LOG_LEVEL level)
+    : ignore(false),
+      ptr(l),
+      level(level) {
+  if (!l->should_log(level)) {
+    setIgnore();
+  }
+}
+
+LogBuilder::~LogBuilder() {
+  if (!ignore)
+    log_string(level);
+}
+
+void LogBuilder::setIgnore() {
+  ignore = true;
+}
+
+void LogBuilder::log_string(LOG_LEVEL level) {
+  ptr->log_string(level, str.str());
+}
+
+
+bool Logger::should_log(const LOG_LEVEL &level) {
+  if (controller_ && !controller_->is_enabled())
+    return false;
+  spdlog::level::level_enum logger_level = spdlog::level::level_enum::info;
+  switch (level) {
+    case critical:
+      logger_level = spdlog::level::level_enum::critical;
+      break;
+    case err:
+      logger_level = spdlog::level::level_enum::err;
+      break;
+    case info:
+      break;
 
 Review comment:
   Nowhere, I copied it verbatim: https://github.com/apache/nifi-minifi-cpp/blob/master/libminifi/include/core/logging/Logger.h#L203
   logger_level is set to info before the switch-case. I don't like it too much either, but it's the original code.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367319673
 
 

 ##########
 File path: libminifi/src/utils/Environment.cpp
 ##########
 @@ -0,0 +1,192 @@
+/**
+ * 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 "utils/Environment.h"
+
+#ifdef WIN32
+#include <Windows.h>
+#else
+#include <cstdlib>
+#include <cerrno>
+#include <unistd.h>
+#endif
+#include <mutex>
+#include <vector>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+bool Environment::runningAsService_(false);
+
+void Environment::accessEnvironment(const std::function<void(void)>& func) {
+  static std::recursive_mutex environmentMutex;
+  std::lock_guard<std::recursive_mutex> lock(environmentMutex);
+  func();
+}
+
+std::pair<bool, std::string> Environment::getEnvironmentVariable(const char* name) {
+  bool exists = false;
+  std::string value;
+
+  Environment::accessEnvironment([&exists, &value, name](){
+#ifdef WIN32
+    std::vector<char> buffer(32767U);  // https://docs.microsoft.com/en-gb/windows/win32/api/processenv/nf-processenv-getenvironmentvariablea
+    // GetEnvironmentVariableA does not set last error to 0 on success, so an error from a pervious API call would influence the GetLastError() later,
+    // so we set the last error to 0 before calling
+    SetLastError(ERROR_SUCCESS);
+    uint32_t ret = GetEnvironmentVariableA(name, buffer.data(), buffer.size());
+    if (ret > 0U) {
+      exists = true;
+      value = std::string(buffer.data(), ret);
+    } else if (GetLastError() == ERROR_SUCCESS) {
+      // Exists, but empty
+      exists = true;
+    }
+#else
+    char* ret = getenv(name);
+    if (ret != nullptr) {
+      exists = true;
+      value = ret;
+    }
+#endif
+  });
+
+  return std::make_pair(exists, std::move(value));
+}
+
+bool Environment::setEnvironmentVariable(const char* name, const char* value, bool overwrite /*= true*/) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name, value, overwrite](){
+#ifdef WIN32
+    if (!overwrite && Environment::getEnvironmentVariable(name).first) {
+      success = true;
+    } else {
+      success = SetEnvironmentVariableA(name, value);
+    }
+#else
+    int ret = setenv(name, value, static_cast<int>(overwrite));
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+bool Environment::unsetEnvironmentVariable(const char* name) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name](){
+#ifdef WIN32
+    success = SetEnvironmentVariableA(name, nullptr);
+#else
+    int ret = unsetenv(name);
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+std::string Environment::getCurrentWorkingDirectory() {
+  std::string cwd;
+
+  Environment::accessEnvironment([&cwd](){
+#ifdef WIN32
+    uint32_t len = 0U;
+    std::vector<char> buffer;
+    // https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getcurrentdirectory
+    // "If the buffer that is pointed to by lpBuffer is not large enough,
+    // the return value specifies the required size of the buffer,
+    // in characters, including the null-terminating character."
+    while (true) {
+      len = GetCurrentDirectoryA(buffer.size(), buffer.data());
+      if (len > buffer.size()) {
 
 Review comment:
   Fancy:
   ```
   if (len <= buffer.size()){
     break;
   }
   buffer.resize(len);
   ```
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367314250
 
 

 ##########
 File path: libminifi/src/core/logging/LoggerConfiguration.cpp
 ##########
 @@ -281,6 +294,22 @@ std::shared_ptr<spdlog::logger> LoggerConfiguration::get_logger(std::shared_ptr<
   return spdlog::get(name);
 }
 
+std::shared_ptr<spdlog::sinks::sink> LoggerConfiguration::create_syslog_sink() {
 
 Review comment:
   Nice one!

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367273019
 
 

 ##########
 File path: libminifi/src/utils/Environment.cpp
 ##########
 @@ -0,0 +1,192 @@
+/**
+ * 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 "utils/Environment.h"
+
+#ifdef WIN32
+#include <Windows.h>
+#else
+#include <cstdlib>
+#include <cerrno>
+#include <unistd.h>
+#endif
+#include <mutex>
+#include <vector>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+bool Environment::runningAsService_(false);
+
+void Environment::accessEnvironment(const std::function<void(void)>& func) {
+  static std::recursive_mutex environmentMutex;
+  std::lock_guard<std::recursive_mutex> lock(environmentMutex);
+  func();
+}
+
+std::pair<bool, std::string> Environment::getEnvironmentVariable(const char* name) {
+  bool exists = false;
+  std::string value;
+
+  Environment::accessEnvironment([&exists, &value, name](){
+#ifdef WIN32
+    std::vector<char> buffer(32767U);  // https://docs.microsoft.com/en-gb/windows/win32/api/processenv/nf-processenv-getenvironmentvariablea
+    // GetEnvironmentVariableA does not set last error to 0 on success, so an error from a pervious API call would influence the GetLastError() later,
+    // so we set the last error to 0 before calling
+    SetLastError(ERROR_SUCCESS);
+    uint32_t ret = GetEnvironmentVariableA(name, buffer.data(), buffer.size());
+    if (ret > 0U) {
+      exists = true;
+      value = std::string(buffer.data(), ret);
+    } else if (GetLastError() == ERROR_SUCCESS) {
+      // Exists, but empty
+      exists = true;
+    }
+#else
+    char* ret = getenv(name);
+    if (ret != nullptr) {
+      exists = true;
+      value = ret;
+    }
+#endif
+  });
+
+  return std::make_pair(exists, std::move(value));
+}
+
+bool Environment::setEnvironmentVariable(const char* name, const char* value, bool overwrite /*= true*/) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name, value, overwrite](){
+#ifdef WIN32
+    if (!overwrite && Environment::getEnvironmentVariable(name).first) {
+      success = true;
+    } else {
+      success = SetEnvironmentVariableA(name, value);
+    }
+#else
+    int ret = setenv(name, value, static_cast<int>(overwrite));
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+bool Environment::unsetEnvironmentVariable(const char* name) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name](){
+#ifdef WIN32
+    success = SetEnvironmentVariableA(name, nullptr);
+#else
+    int ret = unsetenv(name);
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+std::string Environment::getCurrentWorkingDirectory() {
+  std::string cwd;
+
+  Environment::accessEnvironment([&cwd](){
+#ifdef WIN32
+    uint32_t len = 0U;
 
 Review comment:
   This works, but simpler would be:
   char dir[MAX_PATH+1] ;
   GetCurrentDirectory( sizeof(dir), dir ) ;
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

szaszm commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367488333
 
 

 ##########
 File path: libminifi/src/core/logging/WindowsEventLogSink.cpp
 ##########
 @@ -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.
+ */
+
+#ifdef WIN32
+
+#include "core/logging/WindowsEventLogsink.h"
+
+#include "core/logging/WindowsMessageTextFile.h"
+#include "Exception.h"
+
+#include "spdlog/common.h"
+#include "spdlog/sinks/sink.h"
+#include "spdlog/details/log_msg.h"
+
+#include <string>
+
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+namespace internal {
+
+WORD windowseventlog_sink::type_from_level(const spdlog::details::log_msg& msg) const {
+  switch (static_cast<int>(msg.level)) {
 
 Review comment:
   To avoid similar issues in the future, we could avoid having a `default` case and rely on compiler warnings for missing cases.
   https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#enum2-use-enumerations-to-represent-sets-of-related-named-constants
   
   edit: this was meant to be a reply on the comment of @arpadboda on the same line

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367383981
 
 

 ##########
 File path: libminifi/src/core/logging/Logger.cpp
 ##########
 @@ -0,0 +1,149 @@
+/**
+ *
+ * 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 "core/logging/Logger.h"
+
+#include <mutex>
+#include <memory>
+#include <sstream>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+
+
+LoggerControl::LoggerControl()
+    : is_enabled_(true) {
+}
+
+bool LoggerControl::is_enabled() const {
+  return is_enabled_;
+}
+
+void LoggerControl::setEnabled(bool status) {
+  is_enabled_ = status;
+}
+
+
+BaseLogger::~BaseLogger() {
+}
+
+bool BaseLogger::should_log(const LOG_LEVEL &level) {
+  return true;
+}
+
+LogBuilder::LogBuilder(BaseLogger *l, LOG_LEVEL level)
+    : ignore(false),
+      ptr(l),
+      level(level) {
+  if (!l->should_log(level)) {
+    setIgnore();
+  }
+}
+
+LogBuilder::~LogBuilder() {
+  if (!ignore)
+    log_string(level);
 
 Review comment:
   Is it? As far as I see LogBuilder has no parent or child classes, or virtual functions for that matter.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367267981
 
 

 ##########
 File path: libminifi/src/utils/Environment.cpp
 ##########
 @@ -0,0 +1,192 @@
+/**
+ * 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 "utils/Environment.h"
+
+#ifdef WIN32
+#include <Windows.h>
+#else
+#include <cstdlib>
+#include <cerrno>
+#include <unistd.h>
+#endif
+#include <mutex>
+#include <vector>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+bool Environment::runningAsService_(false);
+
+void Environment::accessEnvironment(const std::function<void(void)>& func) {
+  static std::recursive_mutex environmentMutex;
+  std::lock_guard<std::recursive_mutex> lock(environmentMutex);
+  func();
+}
+
+std::pair<bool, std::string> Environment::getEnvironmentVariable(const char* name) {
+  bool exists = false;
+  std::string value;
+
+  Environment::accessEnvironment([&exists, &value, name](){
+#ifdef WIN32
+    std::vector<char> buffer(32767U);  // https://docs.microsoft.com/en-gb/windows/win32/api/processenv/nf-processenv-getenvironmentvariablea
+    // GetEnvironmentVariableA does not set last error to 0 on success, so an error from a pervious API call would influence the GetLastError() later,
+    // so we set the last error to 0 before calling
+    SetLastError(ERROR_SUCCESS);
+    uint32_t ret = GetEnvironmentVariableA(name, buffer.data(), buffer.size());
+    if (ret > 0U) {
+      exists = true;
+      value = std::string(buffer.data(), ret);
+    } else if (GetLastError() == ERROR_SUCCESS) {
+      // Exists, but empty
+      exists = true;
+    }
+#else
+    char* ret = getenv(name);
+    if (ret != nullptr) {
+      exists = true;
+      value = ret;
+    }
+#endif
+  });
+
+  return std::make_pair(exists, std::move(value));
+}
+
+bool Environment::setEnvironmentVariable(const char* name, const char* value, bool overwrite /*= true*/) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name, value, overwrite](){
+#ifdef WIN32
+    if (!overwrite && Environment::getEnvironmentVariable(name).first) {
+      success = true;
+    } else {
+      success = SetEnvironmentVariableA(name, value);
 
 Review comment:
   nit - can used SetEnvironmentVariable.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367333246
 
 

 ##########
 File path: libminifi/src/utils/Environment.cpp
 ##########
 @@ -0,0 +1,192 @@
+/**
+ * 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 "utils/Environment.h"
+
+#ifdef WIN32
+#include <Windows.h>
+#else
+#include <cstdlib>
+#include <cerrno>
+#include <unistd.h>
+#endif
+#include <mutex>
+#include <vector>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+bool Environment::runningAsService_(false);
+
+void Environment::accessEnvironment(const std::function<void(void)>& func) {
+  static std::recursive_mutex environmentMutex;
+  std::lock_guard<std::recursive_mutex> lock(environmentMutex);
+  func();
+}
+
+std::pair<bool, std::string> Environment::getEnvironmentVariable(const char* name) {
+  bool exists = false;
+  std::string value;
+
+  Environment::accessEnvironment([&exists, &value, name](){
+#ifdef WIN32
+    std::vector<char> buffer(32767U);  // https://docs.microsoft.com/en-gb/windows/win32/api/processenv/nf-processenv-getenvironmentvariablea
+    // GetEnvironmentVariableA does not set last error to 0 on success, so an error from a pervious API call would influence the GetLastError() later,
+    // so we set the last error to 0 before calling
+    SetLastError(ERROR_SUCCESS);
+    uint32_t ret = GetEnvironmentVariableA(name, buffer.data(), buffer.size());
 
 Review comment:
   I have been bitten before by a rouge header appearing that included `Windows.h` with `UNICODE` defined, making the macros use the W versions of functions, causing segfaults when called with `char*` strings, so I prefer to explicitly define either the ANSI or the Unicode version of the winapi functions. I think it improves readability as well.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367268047
 
 

 ##########
 File path: libminifi/src/utils/Environment.cpp
 ##########
 @@ -0,0 +1,192 @@
+/**
+ * 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 "utils/Environment.h"
+
+#ifdef WIN32
+#include <Windows.h>
+#else
+#include <cstdlib>
+#include <cerrno>
+#include <unistd.h>
+#endif
+#include <mutex>
+#include <vector>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+bool Environment::runningAsService_(false);
+
+void Environment::accessEnvironment(const std::function<void(void)>& func) {
+  static std::recursive_mutex environmentMutex;
+  std::lock_guard<std::recursive_mutex> lock(environmentMutex);
+  func();
+}
+
+std::pair<bool, std::string> Environment::getEnvironmentVariable(const char* name) {
+  bool exists = false;
+  std::string value;
+
+  Environment::accessEnvironment([&exists, &value, name](){
+#ifdef WIN32
+    std::vector<char> buffer(32767U);  // https://docs.microsoft.com/en-gb/windows/win32/api/processenv/nf-processenv-getenvironmentvariablea
+    // GetEnvironmentVariableA does not set last error to 0 on success, so an error from a pervious API call would influence the GetLastError() later,
+    // so we set the last error to 0 before calling
+    SetLastError(ERROR_SUCCESS);
+    uint32_t ret = GetEnvironmentVariableA(name, buffer.data(), buffer.size());
+    if (ret > 0U) {
+      exists = true;
+      value = std::string(buffer.data(), ret);
+    } else if (GetLastError() == ERROR_SUCCESS) {
+      // Exists, but empty
+      exists = true;
+    }
+#else
+    char* ret = getenv(name);
+    if (ret != nullptr) {
+      exists = true;
+      value = ret;
+    }
+#endif
+  });
+
+  return std::make_pair(exists, std::move(value));
+}
+
+bool Environment::setEnvironmentVariable(const char* name, const char* value, bool overwrite /*= true*/) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name, value, overwrite](){
+#ifdef WIN32
+    if (!overwrite && Environment::getEnvironmentVariable(name).first) {
+      success = true;
+    } else {
+      success = SetEnvironmentVariableA(name, value);
+    }
+#else
+    int ret = setenv(name, value, static_cast<int>(overwrite));
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+bool Environment::unsetEnvironmentVariable(const char* name) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name](){
+#ifdef WIN32
+    success = SetEnvironmentVariableA(name, nullptr);
 
 Review comment:
   nit - can used SetEnvironmentVariable.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367335301
 
 

 ##########
 File path: libminifi/CMakeLists.txt
 ##########
 @@ -91,10 +91,38 @@ endif()
 
 file(GLOB SOURCES  "src/utils/file/*.cpp" "src/sitetosite/*.cpp"  "src/core/logging/*.cpp"  "src/core/state/*.cpp" "src/core/state/nodes/*.cpp" "src/c2/protocols/*.cpp" "src/c2/triggers/*.cpp" "src/c2/*.cpp" "src/io/*.cpp" ${SOCKET_SOURCES} ${TLS_SOURCES} "src/core/controller/*.cpp" "src/controllers/*.cpp" "src/core/*.cpp"  "src/core/repository/*.cpp" "src/core/yaml/*.cpp" "src/core/reporting/*.cpp"  "src/provenance/*.cpp" "src/utils/*.cpp" "src/*.cpp")
 
+if(WIN32)
+	if(CMAKE_SIZEOF_VOID_P EQUAL 8)
+		set(MC_COMPILER_SEARCH_PATH "$ENV{WindowsSdkVerBinPath}\\x64")
+	elseif(CMAKE_SIZEOF_VOID_P EQUAL 4)
+		set(MC_COMPILER_SEARCH_PATH "$ENV{WindowsSdkVerBinPath}\\x86")
 
 Review comment:
   The `mc.exe` to use depends on the target, not the host. CMAKE_SIZEOF_VOID_P is defined for the target architecture, so we are in the clear.
   AppVeyor builds fail to find the mc.exe though, so I have to tweak this a little bit. :(

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367513648
 
 

 ##########
 File path: libminifi/test/TestBase.cpp
 ##########
 @@ -18,6 +18,21 @@
 
 #include "./TestBase.h"
 
+#include "spdlog/spdlog.h"
+
+void LogTestController::setLevel(const std::string name, spdlog::level::level_enum level) {
+  logger_->log_info("Setting log level for %s to %s", name, spdlog::level::to_str(level));
+  std::string adjusted_name = name;
+  const std::string clazz = "class ";
+  auto haz_clazz = name.find(clazz);
+  if (haz_clazz == 0)
+    adjusted_name = name.substr(clazz.length(), name.length() - clazz.length());
+  if (config && config->shortenClassNames()) {
+    utils::ClassUtils::shortenClassName(adjusted_name, adjusted_name);
+  }
 
 Review comment:
   This has just been moved from the h to the cpp file to avoid some include issues. You are welcome to create a refactor task for this, if it is used in more places I would agree that a good solution.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on issue #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on issue #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#issuecomment-575609524
 
 
   Addressed the new set of review comments, Travis builds seems fixed. Will wait for others to test this.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on issue #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on issue #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#issuecomment-576176249
 
 
   I built installer, installed, server was running, log created under {installedDir}/Log. 
   LGTM.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367274461
 
 

 ##########
 File path: libminifi/src/utils/file/PathUtils.cpp
 ##########
 @@ -48,6 +56,35 @@ bool PathUtils::getFileNameAndPath(const std::string &path, std::string &filePat
   return true;
 }
 
+std::string PathUtils::getFullPath(const std::string& path) {
+#ifdef WIN32
+  std::vector<char> buffer(MAX_PATH);
 
 Review comment:
   MAX_PATH just 260, could be used `char buffer[MAX_PATH+1];`

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367315605
 
 

 ##########
 File path: libminifi/src/core/logging/WindowsEventLogSink.cpp
 ##########
 @@ -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.
+ */
+
+#ifdef WIN32
+
+#include "core/logging/WindowsEventLogsink.h"
+
+#include "core/logging/WindowsMessageTextFile.h"
+#include "Exception.h"
+
+#include "spdlog/common.h"
+#include "spdlog/sinks/sink.h"
+#include "spdlog/details/log_msg.h"
+
+#include <string>
+
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+namespace internal {
+
+WORD windowseventlog_sink::type_from_level(const spdlog::details::log_msg& msg) const {
+  switch (static_cast<int>(msg.level)) {
 
 Review comment:
   Just a note here, spdlog::level::off exists, even though we don't use it. 
   Guess we shouldn't map to error type via the default case. 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367367243
 
 

 ##########
 File path: main/MiNiFiMain.cpp
 ##########
 @@ -142,94 +159,39 @@ int main(int argc, char **argv) {
 #ifdef WIN32
 	if (!SetConsoleCtrlHandler(consoleSignalHandler, TRUE)) {
 		logger->log_error("Cannot install signal handler");
-		std::cerr << "Cannot install signal handler" << std::endl;
-		return 1;
+		return -1;
 	}
 
 	if (signal(SIGINT, sigHandler) == SIG_ERR || signal(SIGTERM, sigHandler) == SIG_ERR ) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+		logger->log_error("Cannot install signal handler");
 		return -1;
 	}
 #ifdef SIGBREAK
 	if (signal(SIGBREAK, sigHandler) == SIG_ERR) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+		logger->log_error("Cannot install signal handler");
 		return -1;
 	}
 #endif
 #else
 	if (signal(SIGINT, sigHandler) == SIG_ERR || signal(SIGTERM, sigHandler) == SIG_ERR || signal(SIGPIPE, SIG_IGN) == SIG_ERR) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+    logger->log_error("Cannot install signal handler");
 
 Review comment:
   This looks very strangely indented. 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367369231
 
 

 ##########
 File path: libminifi/test/unit/FileUtilsTests.cpp
 ##########
 @@ -146,3 +148,33 @@ TEST_CASE("TestFileUtils::create_dir", "[TestCreateDir]") {
   REQUIRE(FileUtils::create_dir(test_dir_path) == 0);  // Dir already exists, success should be returned
   REQUIRE(FileUtils::delete_dir(test_dir_path) == 0);  // Delete should be successful as welll
 }
+
+TEST_CASE("TestFileUtils::getFullPath", "[TestGetFullPath]") {
+  TestController testController;
+
+  char format[] = "/tmp/gt.XXXXXX";
+  const std::string tempDir = utils::file::PathUtils::getFullPath(testController.createTempDirectory(format));
+
+  const std::string cwd = utils::Environment::getCurrentWorkingDirectory();
+
+  REQUIRE(utils::Environment::setCurrentWorkingDirectory(tempDir.c_str()));
+  utils::ScopeGuard cwdGuard([&cwd]() {
+    utils::Environment::setCurrentWorkingDirectory(cwd.c_str());
+  });
+
+  const std::string tempDir1 = utils::file::FileUtils::concat_path(tempDir, "test1");
+  const std::string tempDir2 = utils::file::FileUtils::concat_path(tempDir, "test2");
+  REQUIRE(0 == utils::file::FileUtils::create_dir(tempDir1));
+  REQUIRE(0 == utils::file::FileUtils::create_dir(tempDir2));
+
+  REQUIRE(tempDir1 == utils::file::PathUtils::getFullPath(tempDir1));
+  REQUIRE(tempDir1 == utils::file::PathUtils::getFullPath("test1"));
+  REQUIRE(tempDir1 == utils::file::PathUtils::getFullPath("./test1"));
+  REQUIRE(tempDir1 == utils::file::PathUtils::getFullPath("././test1"));
 
 Review comment:
   Yep, and it does.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367275409
 
 

 ##########
 File path: libminifi/src/utils/file/PathUtils.cpp
 ##########
 @@ -48,6 +56,35 @@ bool PathUtils::getFileNameAndPath(const std::string &path, std::string &filePat
   return true;
 }
 
+std::string PathUtils::getFullPath(const std::string& path) {
+#ifdef WIN32
+  std::vector<char> buffer(MAX_PATH);
+  uint32_t len = 0U;
+  while (true) {
+      len = GetFullPathNameA(path.c_str(), buffer.size(), buffer.data(), nullptr /*lpFilePart*/);
 
 Review comment:
   nit - - can be used GetFullPathName.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367368093
 
 

 ##########
 File path: main/MiNiFiMain.cpp
 ##########
 @@ -142,94 +159,39 @@ int main(int argc, char **argv) {
 #ifdef WIN32
 	if (!SetConsoleCtrlHandler(consoleSignalHandler, TRUE)) {
 		logger->log_error("Cannot install signal handler");
-		std::cerr << "Cannot install signal handler" << std::endl;
-		return 1;
+		return -1;
 	}
 
 	if (signal(SIGINT, sigHandler) == SIG_ERR || signal(SIGTERM, sigHandler) == SIG_ERR ) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+		logger->log_error("Cannot install signal handler");
 		return -1;
 	}
 #ifdef SIGBREAK
 	if (signal(SIGBREAK, sigHandler) == SIG_ERR) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+		logger->log_error("Cannot install signal handler");
 		return -1;
 	}
 #endif
 #else
 	if (signal(SIGINT, sigHandler) == SIG_ERR || signal(SIGTERM, sigHandler) == SIG_ERR || signal(SIGPIPE, SIG_IGN) == SIG_ERR) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+    logger->log_error("Cannot install signal handler");
 		return -1;
 	}
 #endif
-	// assumes POSIX compliant environment
-	std::string minifiHome;
-	if (const char *env_p = std::getenv(MINIFI_HOME_ENV_KEY)) {
-		minifiHome = env_p;
-		logger->log_info("Using MINIFI_HOME=%s from environment.", minifiHome);
-	}
-	else {
-		logger->log_info("MINIFI_HOME is not set; determining based on environment.");
-		char *path = nullptr;
-		char full_path[PATH_MAX];
-#ifndef WIN32
-		path = realpath(argv[0], full_path);
-#else
-		path = nullptr;
-#endif
-
-		if (path != nullptr) {
-			std::string minifiHomePath(path);
-			if (minifiHomePath.find_last_of("/\\") != std::string::npos) {
-				minifiHomePath = minifiHomePath.substr(0, minifiHomePath.find_last_of("/\\"));  //Remove /minifi from path
-				minifiHome = minifiHomePath.substr(0, minifiHomePath.find_last_of("/\\"));    //Remove /bin from path
-			}
-		}
-
-		// attempt to use cwd as MINIFI_HOME
-		if (minifiHome.empty() || !validHome(minifiHome)) {
-			char cwd[PATH_MAX];
-#ifdef WIN32
-			_getcwd(cwd, PATH_MAX);
-			auto handle = GetModuleHandle(0);
-			GetModuleFileNameA(NULL, cwd, sizeof(cwd));
-			std::string fullPath = cwd;
-			std::string minifiFileName, minifiPath;
-			minifi::utils::file::PathUtils::getFileNameAndPath(fullPath, minifiPath, minifiFileName);
-			if (utils::StringUtils::endsWith(minifiPath, "bin")) {
-				minifiHome = minifiPath.substr(0, minifiPath.size()-3);
-			}
-			else {
-				minifiHome = minifiPath;
-			}
-		
-#else
-			getcwd(cwd, PATH_MAX);
-			minifiHome = cwd;
-#endif
-			
-		}
-
 
-		logger->log_debug("Setting %s to %s", MINIFI_HOME_ENV_KEY, minifiHome);
-#ifdef WIN32
-		SetEnvironmentVariable(MINIFI_HOME_ENV_KEY, minifiHome.c_str());
-#else
-		setenv(MINIFI_HOME_ENV_KEY, minifiHome.c_str(), 0);
-#endif
+	// Determine MINIFI_HOME
+  const std::string minifiHome = determineMinifiHome(logger);
+	if (minifiHome.empty()) {
+	  // determineMinifiHome already logged everything we need
+	  return -1;
 	}
 
-	if (!validHome(minifiHome)) {
-		minifiHome = minifiHome.substr(0, minifiHome.find_last_of("/\\"));    //Remove /bin from path
-		if (!validHome(minifiHome)) {
-			logger->log_error("No valid MINIFI_HOME could be inferred. "
-				"Please set MINIFI_HOME or run minifi from a valid location. minifiHome is %s", minifiHome);
-			return -1;
-		}
+	// chdir to MINIFI_HOME
+	if (!utils::Environment::setCurrentWorkingDirectory(minifiHome.c_str())) {
 
 Review comment:
   Will run some tests, but looks good at first

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367355868
 
 

 ##########
 File path: libminifi/test/unit/EnvironmentUtilsTests.cpp
 ##########
 @@ -0,0 +1,154 @@
+/**
+ *
+ * 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 "../TestBase.h"
+#include "utils/Environment.h"
+#include "utils/file/PathUtils.h"
+
+#include <thread>
+#include <random>
+#include <string>
+#include <cstdint>
+#include <vector>
+
+TEST_CASE("getenv already existing", "[getenv]") {
+  auto res = utils::Environment::getEnvironmentVariable("PATH");
+  REQUIRE(true == res.first);
+  REQUIRE(0 < res.second.length());
+}
+
+TEST_CASE("getenv not existing", "[getenv]") {
+  auto res = utils::Environment::getEnvironmentVariable("GETENV1");
+  REQUIRE(false == res.first);
+  REQUIRE("" == res.second);
+}
+
+TEST_CASE("getenv empty existing", "[getenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("GETENV2", ""));
+  auto res = utils::Environment::getEnvironmentVariable("GETENV2");
+  REQUIRE(true == res.first);
+  REQUIRE("" == res.second);
+}
+
+TEST_CASE("setenv not existing overwrite", "[setenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV1", "test"));
+  auto res = utils::Environment::getEnvironmentVariable("SETENV1");
+  REQUIRE(true == res.first);
+  REQUIRE("test" == res.second);
+}
+
+TEST_CASE("setenv existing overwrite", "[setenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV2", "test"));
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV2", "test2"));
+  auto res = utils::Environment::getEnvironmentVariable("SETENV2");
+  REQUIRE(true == res.first);
+  REQUIRE("test2" == res.second);
+}
+
+TEST_CASE("setenv not existing no overwrite", "[setenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV3", "test", false /*overwrite*/));
+  auto res = utils::Environment::getEnvironmentVariable("SETENV3");
+  REQUIRE(true == res.first);
+  REQUIRE("test" == res.second);
+}
+
+TEST_CASE("setenv existing no overwrite", "[setenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV4", "test"));
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV4", "test2", false /*overwrite*/));
+  auto res = utils::Environment::getEnvironmentVariable("SETENV4");
+  REQUIRE(true == res.first);
+  REQUIRE("test" == res.second);
+}
+
+TEST_CASE("unsetenv not existing", "[unsetenv]") {
+  REQUIRE(false == utils::Environment::getEnvironmentVariable("UNSETENV1").first);
+  REQUIRE(true == utils::Environment::unsetEnvironmentVariable("UNSETENV1"));
+  REQUIRE(false == utils::Environment::getEnvironmentVariable("UNSETENV1").first);
+}
+
+TEST_CASE("unsetenv existing", "[unsetenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("UNSETENV2", "test"));
+  REQUIRE(true == utils::Environment::getEnvironmentVariable("UNSETENV2").first);
+  REQUIRE(true == utils::Environment::unsetEnvironmentVariable("UNSETENV2"));
+  REQUIRE(false == utils::Environment::getEnvironmentVariable("UNSETENV2").first);
+}
+
+TEST_CASE("multithreaded environment manipulation", "[getenv][setenv][unsetenv]") {
+  std::vector<std::thread> threads;
+  for (size_t i = 0U; i < 16U; i++) {
+    threads.emplace_back([](){
+      std::mt19937 gen(std::random_device { }());
+      for (size_t i = 0U; i < 10240U; i++) {
+        const uint8_t env_num = gen() % 8;
+        const std::string env_name = "GETSETUNSETENV" + std::to_string(env_num);
+        const uint8_t operation = gen() % 3;
+        switch (operation) {
+          case 0: {
+              auto res = utils::Environment::getEnvironmentVariable(env_name.c_str());
+              break;
+            }
+          case 1: {
+              const size_t value_len = gen() % 256;
+              std::vector<char> value(value_len + 1, '\0');
+              std::generate_n(value.begin(), value_len, [&]() -> char {
+                return 'A' + gen() % static_cast<uint8_t>('Z' - 'A');
+              });
+              const bool overwrite = gen() % 2;
+              utils::Environment::setEnvironmentVariable(env_name.c_str(), value.data(), overwrite);
+              break;
+            }
+          case 2: {
+              utils::Environment::unsetEnvironmentVariable(env_name.c_str());
+              break;
+            }
+        }
+      }
+      });
+    }
+  for (auto& thread : threads) {
+    thread.join();
+  }
+  for (size_t i = 0U; i < 8U; i++) {
+    const std::string env_name = "GETSETUNSETENV" + std::to_string(i);
+    bool isset = false;
+    std::string value;
+    std::tie(isset, value) = utils::Environment::getEnvironmentVariable(env_name.c_str());
+    if (isset) {
+      std::cerr << env_name << " is set to " << value << std::endl;
 
 Review comment:
   Maybe I didn't have enough coffee, but I miss some "REQUIRE" calls here 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r366867563
 
 

 ##########
 File path: main/MiNiFiMain.cpp
 ##########
 @@ -142,94 +159,39 @@ int main(int argc, char **argv) {
 #ifdef WIN32
 	if (!SetConsoleCtrlHandler(consoleSignalHandler, TRUE)) {
 		logger->log_error("Cannot install signal handler");
-		std::cerr << "Cannot install signal handler" << std::endl;
-		return 1;
+		return -1;
 	}
 
 	if (signal(SIGINT, sigHandler) == SIG_ERR || signal(SIGTERM, sigHandler) == SIG_ERR ) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+		logger->log_error("Cannot install signal handler");
 		return -1;
 	}
 #ifdef SIGBREAK
 	if (signal(SIGBREAK, sigHandler) == SIG_ERR) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+		logger->log_error("Cannot install signal handler");
 		return -1;
 	}
 #endif
 #else
 	if (signal(SIGINT, sigHandler) == SIG_ERR || signal(SIGTERM, sigHandler) == SIG_ERR || signal(SIGPIPE, SIG_IGN) == SIG_ERR) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+    logger->log_error("Cannot install signal handler");
 		return -1;
 	}
 #endif
-	// assumes POSIX compliant environment
-	std::string minifiHome;
-	if (const char *env_p = std::getenv(MINIFI_HOME_ENV_KEY)) {
-		minifiHome = env_p;
-		logger->log_info("Using MINIFI_HOME=%s from environment.", minifiHome);
-	}
-	else {
-		logger->log_info("MINIFI_HOME is not set; determining based on environment.");
-		char *path = nullptr;
-		char full_path[PATH_MAX];
-#ifndef WIN32
-		path = realpath(argv[0], full_path);
-#else
-		path = nullptr;
-#endif
-
-		if (path != nullptr) {
-			std::string minifiHomePath(path);
-			if (minifiHomePath.find_last_of("/\\") != std::string::npos) {
-				minifiHomePath = minifiHomePath.substr(0, minifiHomePath.find_last_of("/\\"));  //Remove /minifi from path
-				minifiHome = minifiHomePath.substr(0, minifiHomePath.find_last_of("/\\"));    //Remove /bin from path
-			}
-		}
-
-		// attempt to use cwd as MINIFI_HOME
-		if (minifiHome.empty() || !validHome(minifiHome)) {
-			char cwd[PATH_MAX];
-#ifdef WIN32
-			_getcwd(cwd, PATH_MAX);
-			auto handle = GetModuleHandle(0);
-			GetModuleFileNameA(NULL, cwd, sizeof(cwd));
-			std::string fullPath = cwd;
-			std::string minifiFileName, minifiPath;
-			minifi::utils::file::PathUtils::getFileNameAndPath(fullPath, minifiPath, minifiFileName);
-			if (utils::StringUtils::endsWith(minifiPath, "bin")) {
-				minifiHome = minifiPath.substr(0, minifiPath.size()-3);
-			}
-			else {
-				minifiHome = minifiPath;
-			}
-		
-#else
-			getcwd(cwd, PATH_MAX);
-			minifiHome = cwd;
-#endif
-			
-		}
-
 
-		logger->log_debug("Setting %s to %s", MINIFI_HOME_ENV_KEY, minifiHome);
-#ifdef WIN32
-		SetEnvironmentVariable(MINIFI_HOME_ENV_KEY, minifiHome.c_str());
-#else
-		setenv(MINIFI_HOME_ENV_KEY, minifiHome.c_str(), 0);
-#endif
+	// Determine MINIFI_HOME
+  const std::string minifiHome = determineMinifiHome(logger);
+	if (minifiHome.empty()) {
+	  // determineMinifiHome already logged everything we need
+	  return -1;
 	}
 
-	if (!validHome(minifiHome)) {
-		minifiHome = minifiHome.substr(0, minifiHome.find_last_of("/\\"));    //Remove /bin from path
-		if (!validHome(minifiHome)) {
-			logger->log_error("No valid MINIFI_HOME could be inferred. "
-				"Please set MINIFI_HOME or run minifi from a valid location. minifiHome is %s", minifiHome);
-			return -1;
-		}
+	// chdir to MINIFI_HOME
+	if (!utils::Environment::setCurrentWorkingDirectory(minifiHome.c_str())) {
 
 Review comment:
   This will solve the problem of relative paths being relative to the cwd, instead of MINIFI_HOME. A lot of code has been written under the assumption that this is true, but with the Windows service case it is explicitly not (the default working directory is C:\Windows\System32

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367346480
 
 

 ##########
 File path: libminifi/src/core/logging/Logger.cpp
 ##########
 @@ -0,0 +1,149 @@
+/**
+ *
+ * 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 "core/logging/Logger.h"
+
+#include <mutex>
+#include <memory>
+#include <sstream>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+
+
+LoggerControl::LoggerControl()
+    : is_enabled_(true) {
+}
+
+bool LoggerControl::is_enabled() const {
+  return is_enabled_;
+}
+
+void LoggerControl::setEnabled(bool status) {
+  is_enabled_ = status;
+}
+
+
+BaseLogger::~BaseLogger() {
+}
+
+bool BaseLogger::should_log(const LOG_LEVEL &level) {
+  return true;
+}
+
+LogBuilder::LogBuilder(BaseLogger *l, LOG_LEVEL level)
+    : ignore(false),
+      ptr(l),
+      level(level) {
+  if (!l->should_log(level)) {
+    setIgnore();
+  }
+}
+
+LogBuilder::~LogBuilder() {
+  if (!ignore)
+    log_string(level);
+}
+
+void LogBuilder::setIgnore() {
+  ignore = true;
+}
+
+void LogBuilder::log_string(LOG_LEVEL level) {
+  ptr->log_string(level, str.str());
+}
+
+
+bool Logger::should_log(const LOG_LEVEL &level) {
+  if (controller_ && !controller_->is_enabled())
+    return false;
+  spdlog::level::level_enum logger_level = spdlog::level::level_enum::info;
+  switch (level) {
+    case critical:
+      logger_level = spdlog::level::level_enum::critical;
+      break;
+    case err:
+      logger_level = spdlog::level::level_enum::err;
+      break;
+    case info:
+      break;
 
 Review comment:
   Oh, god, someone must have skipped the lesson where default case was explained... 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

szaszm commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367490826
 
 

 ##########
 File path: libminifi/test/TestBase.cpp
 ##########
 @@ -18,6 +18,21 @@
 
 #include "./TestBase.h"
 
+#include "spdlog/spdlog.h"
+
+void LogTestController::setLevel(const std::string name, spdlog::level::level_enum level) {
+  logger_->log_info("Setting log level for %s to %s", name, spdlog::level::to_str(level));
+  std::string adjusted_name = name;
+  const std::string clazz = "class ";
+  auto haz_clazz = name.find(clazz);
+  if (haz_clazz == 0)
+    adjusted_name = name.substr(clazz.length(), name.length() - clazz.length());
+  if (config && config->shortenClassNames()) {
+    utils::ClassUtils::shortenClassName(adjusted_name, adjusted_name);
+  }
 
 Review comment:
   I believe the logic to fix class names doesn't belong here but rather in a function that's used to query class names.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367392339
 
 

 ##########
 File path: libminifi/src/utils/Environment.cpp
 ##########
 @@ -0,0 +1,192 @@
+/**
+ * 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 "utils/Environment.h"
+
+#ifdef WIN32
+#include <Windows.h>
+#else
+#include <cstdlib>
+#include <cerrno>
+#include <unistd.h>
+#endif
+#include <mutex>
+#include <vector>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+bool Environment::runningAsService_(false);
+
+void Environment::accessEnvironment(const std::function<void(void)>& func) {
+  static std::recursive_mutex environmentMutex;
+  std::lock_guard<std::recursive_mutex> lock(environmentMutex);
+  func();
+}
+
+std::pair<bool, std::string> Environment::getEnvironmentVariable(const char* name) {
+  bool exists = false;
+  std::string value;
+
+  Environment::accessEnvironment([&exists, &value, name](){
+#ifdef WIN32
+    std::vector<char> buffer(32767U);  // https://docs.microsoft.com/en-gb/windows/win32/api/processenv/nf-processenv-getenvironmentvariablea
+    // GetEnvironmentVariableA does not set last error to 0 on success, so an error from a pervious API call would influence the GetLastError() later,
+    // so we set the last error to 0 before calling
+    SetLastError(ERROR_SUCCESS);
+    uint32_t ret = GetEnvironmentVariableA(name, buffer.data(), buffer.size());
+    if (ret > 0U) {
+      exists = true;
+      value = std::string(buffer.data(), ret);
+    } else if (GetLastError() == ERROR_SUCCESS) {
+      // Exists, but empty
+      exists = true;
+    }
+#else
+    char* ret = getenv(name);
+    if (ret != nullptr) {
+      exists = true;
+      value = ret;
+    }
+#endif
+  });
+
+  return std::make_pair(exists, std::move(value));
+}
+
+bool Environment::setEnvironmentVariable(const char* name, const char* value, bool overwrite /*= true*/) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name, value, overwrite](){
+#ifdef WIN32
+    if (!overwrite && Environment::getEnvironmentVariable(name).first) {
+      success = true;
+    } else {
+      success = SetEnvironmentVariableA(name, value);
+    }
+#else
+    int ret = setenv(name, value, static_cast<int>(overwrite));
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+bool Environment::unsetEnvironmentVariable(const char* name) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name](){
+#ifdef WIN32
+    success = SetEnvironmentVariableA(name, nullptr);
+#else
+    int ret = unsetenv(name);
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+std::string Environment::getCurrentWorkingDirectory() {
+  std::string cwd;
+
+  Environment::accessEnvironment([&cwd](){
+#ifdef WIN32
+    uint32_t len = 0U;
+    std::vector<char> buffer;
+    // https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getcurrentdirectory
+    // "If the buffer that is pointed to by lpBuffer is not large enough,
+    // the return value specifies the required size of the buffer,
+    // in characters, including the null-terminating character."
+    while (true) {
+      len = GetCurrentDirectoryA(buffer.size(), buffer.data());
+      if (len > buffer.size()) {
 
 Review comment:
   Yep, and you actually caught a potential bug. On `len == buffer.size()` we should retry as well, as on insufficient space it returns the required length of the buffer INCLUDING the terminating \0, on success however it returns the length of the string WITHOUT the terminating \0;

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367469930
 
 

 ##########
 File path: libminifi/src/core/logging/Logger.cpp
 ##########
 @@ -0,0 +1,149 @@
+/**
+ *
+ * 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 "core/logging/Logger.h"
+
+#include <mutex>
+#include <memory>
+#include <sstream>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+
+
+LoggerControl::LoggerControl()
+    : is_enabled_(true) {
+}
+
+bool LoggerControl::is_enabled() const {
+  return is_enabled_;
+}
+
+void LoggerControl::setEnabled(bool status) {
+  is_enabled_ = status;
+}
+
+
+BaseLogger::~BaseLogger() {
+}
+
+bool BaseLogger::should_log(const LOG_LEVEL &level) {
+  return true;
+}
+
+LogBuilder::LogBuilder(BaseLogger *l, LOG_LEVEL level)
+    : ignore(false),
+      ptr(l),
+      level(level) {
+  if (!l->should_log(level)) {
+    setIgnore();
+  }
+}
+
+LogBuilder::~LogBuilder() {
+  if (!ignore)
+    log_string(level);
 
 Review comment:
   My bad, it's virtual is BaseLogger, this code is correct. 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

am-c-p-p commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367268406
 
 

 ##########
 File path: libminifi/src/utils/Environment.cpp
 ##########
 @@ -0,0 +1,192 @@
+/**
+ * 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 "utils/Environment.h"
+
+#ifdef WIN32
+#include <Windows.h>
+#else
+#include <cstdlib>
+#include <cerrno>
+#include <unistd.h>
+#endif
+#include <mutex>
+#include <vector>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace utils {
+
+bool Environment::runningAsService_(false);
+
+void Environment::accessEnvironment(const std::function<void(void)>& func) {
+  static std::recursive_mutex environmentMutex;
+  std::lock_guard<std::recursive_mutex> lock(environmentMutex);
+  func();
+}
+
+std::pair<bool, std::string> Environment::getEnvironmentVariable(const char* name) {
+  bool exists = false;
+  std::string value;
+
+  Environment::accessEnvironment([&exists, &value, name](){
+#ifdef WIN32
+    std::vector<char> buffer(32767U);  // https://docs.microsoft.com/en-gb/windows/win32/api/processenv/nf-processenv-getenvironmentvariablea
+    // GetEnvironmentVariableA does not set last error to 0 on success, so an error from a pervious API call would influence the GetLastError() later,
+    // so we set the last error to 0 before calling
+    SetLastError(ERROR_SUCCESS);
+    uint32_t ret = GetEnvironmentVariableA(name, buffer.data(), buffer.size());
+    if (ret > 0U) {
+      exists = true;
+      value = std::string(buffer.data(), ret);
+    } else if (GetLastError() == ERROR_SUCCESS) {
+      // Exists, but empty
+      exists = true;
+    }
+#else
+    char* ret = getenv(name);
+    if (ret != nullptr) {
+      exists = true;
+      value = ret;
+    }
+#endif
+  });
+
+  return std::make_pair(exists, std::move(value));
+}
+
+bool Environment::setEnvironmentVariable(const char* name, const char* value, bool overwrite /*= true*/) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name, value, overwrite](){
+#ifdef WIN32
+    if (!overwrite && Environment::getEnvironmentVariable(name).first) {
+      success = true;
+    } else {
+      success = SetEnvironmentVariableA(name, value);
+    }
+#else
+    int ret = setenv(name, value, static_cast<int>(overwrite));
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+bool Environment::unsetEnvironmentVariable(const char* name) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, name](){
+#ifdef WIN32
+    success = SetEnvironmentVariableA(name, nullptr);
+#else
+    int ret = unsetenv(name);
+    success = ret == 0;
+#endif
+  });
+
+  return success;
+}
+
+std::string Environment::getCurrentWorkingDirectory() {
+  std::string cwd;
+
+  Environment::accessEnvironment([&cwd](){
+#ifdef WIN32
+    uint32_t len = 0U;
+    std::vector<char> buffer;
+    // https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getcurrentdirectory
+    // "If the buffer that is pointed to by lpBuffer is not large enough,
+    // the return value specifies the required size of the buffer,
+    // in characters, including the null-terminating character."
+    while (true) {
+      len = GetCurrentDirectoryA(buffer.size(), buffer.data());
+      if (len > buffer.size()) {
+        buffer.resize(len);
+        continue;
+      } else {
+        break;
+      }
+    }
+    if (len > 0U) {
+      cwd = std::string(buffer.data(), len);
+    }
+#else
+    std::vector<char> buffer(1024U);
+    char* path = nullptr;
+    while (true) {
+      path = getcwd(buffer.data(), buffer.size());
+      if (path == nullptr) {
+        if (errno == ERANGE) {
+          buffer.resize(buffer.size() * 2);
+          continue;
+        } else {
+          break;
+        }
+      } else {
+        break;
+      }
+    }
+    if (path != nullptr) {
+      cwd = path;
+    }
+#endif
+  });
+
+  return cwd;
+}
+
+bool Environment::setCurrentWorkingDirectory(const char* directory) {
+  bool success = false;
+
+  Environment::accessEnvironment([&success, directory](){
+#ifdef WIN32
+    success = SetCurrentDirectoryA(directory);
 
 Review comment:
   nit - can be used SetCurrentDirectory.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367313445
 
 

 ##########
 File path: libminifi/src/core/logging/Logger.cpp
 ##########
 @@ -0,0 +1,149 @@
+/**
+ *
+ * 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 "core/logging/Logger.h"
+
+#include <mutex>
+#include <memory>
+#include <sstream>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+
+
+LoggerControl::LoggerControl()
+    : is_enabled_(true) {
+}
+
+bool LoggerControl::is_enabled() const {
+  return is_enabled_;
+}
+
+void LoggerControl::setEnabled(bool status) {
+  is_enabled_ = status;
+}
+
+
+BaseLogger::~BaseLogger() {
+}
+
+bool BaseLogger::should_log(const LOG_LEVEL &level) {
+  return true;
+}
+
+LogBuilder::LogBuilder(BaseLogger *l, LOG_LEVEL level)
+    : ignore(false),
+      ptr(l),
+      level(level) {
+  if (!l->should_log(level)) {
+    setIgnore();
+  }
+}
+
+LogBuilder::~LogBuilder() {
+  if (!ignore)
+    log_string(level);
+}
+
+void LogBuilder::setIgnore() {
+  ignore = true;
+}
+
+void LogBuilder::log_string(LOG_LEVEL level) {
+  ptr->log_string(level, str.str());
+}
+
+
+bool Logger::should_log(const LOG_LEVEL &level) {
+  if (controller_ && !controller_->is_enabled())
+    return false;
+  spdlog::level::level_enum logger_level = spdlog::level::level_enum::info;
+  switch (level) {
+    case critical:
+      logger_level = spdlog::level::level_enum::critical;
+      break;
+    case err:
+      logger_level = spdlog::level::level_enum::err;
+      break;
+    case info:
+      break;
+    case debug:
+      logger_level = spdlog::level::level_enum::debug;
+      break;
+    case off:
+      logger_level = spdlog::level::level_enum::off;
+      break;
+    case trace:
+      logger_level = spdlog::level::level_enum::trace;
+      break;
+    case warn:
+      logger_level = spdlog::level::level_enum::warn;
+      break;
+  }
+
+  std::lock_guard<std::mutex> lock(mutex_);
+  if (!delegate_->should_log(logger_level)) {
 
 Review comment:
   Fancy: just return this? 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367369538
 
 

 ##########
 File path: libminifi/test/unit/EnvironmentUtilsTests.cpp
 ##########
 @@ -0,0 +1,154 @@
+/**
+ *
+ * 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 "../TestBase.h"
+#include "utils/Environment.h"
+#include "utils/file/PathUtils.h"
+
+#include <thread>
+#include <random>
+#include <string>
+#include <cstdint>
+#include <vector>
+
+TEST_CASE("getenv already existing", "[getenv]") {
+  auto res = utils::Environment::getEnvironmentVariable("PATH");
+  REQUIRE(true == res.first);
+  REQUIRE(0 < res.second.length());
+}
+
+TEST_CASE("getenv not existing", "[getenv]") {
+  auto res = utils::Environment::getEnvironmentVariable("GETENV1");
+  REQUIRE(false == res.first);
+  REQUIRE("" == res.second);
+}
+
+TEST_CASE("getenv empty existing", "[getenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("GETENV2", ""));
+  auto res = utils::Environment::getEnvironmentVariable("GETENV2");
+  REQUIRE(true == res.first);
+  REQUIRE("" == res.second);
+}
+
+TEST_CASE("setenv not existing overwrite", "[setenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV1", "test"));
+  auto res = utils::Environment::getEnvironmentVariable("SETENV1");
+  REQUIRE(true == res.first);
+  REQUIRE("test" == res.second);
+}
+
+TEST_CASE("setenv existing overwrite", "[setenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV2", "test"));
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV2", "test2"));
+  auto res = utils::Environment::getEnvironmentVariable("SETENV2");
+  REQUIRE(true == res.first);
+  REQUIRE("test2" == res.second);
+}
+
+TEST_CASE("setenv not existing no overwrite", "[setenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV3", "test", false /*overwrite*/));
+  auto res = utils::Environment::getEnvironmentVariable("SETENV3");
+  REQUIRE(true == res.first);
+  REQUIRE("test" == res.second);
+}
+
+TEST_CASE("setenv existing no overwrite", "[setenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV4", "test"));
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("SETENV4", "test2", false /*overwrite*/));
+  auto res = utils::Environment::getEnvironmentVariable("SETENV4");
+  REQUIRE(true == res.first);
+  REQUIRE("test" == res.second);
+}
+
+TEST_CASE("unsetenv not existing", "[unsetenv]") {
+  REQUIRE(false == utils::Environment::getEnvironmentVariable("UNSETENV1").first);
+  REQUIRE(true == utils::Environment::unsetEnvironmentVariable("UNSETENV1"));
+  REQUIRE(false == utils::Environment::getEnvironmentVariable("UNSETENV1").first);
+}
+
+TEST_CASE("unsetenv existing", "[unsetenv]") {
+  REQUIRE(true == utils::Environment::setEnvironmentVariable("UNSETENV2", "test"));
+  REQUIRE(true == utils::Environment::getEnvironmentVariable("UNSETENV2").first);
+  REQUIRE(true == utils::Environment::unsetEnvironmentVariable("UNSETENV2"));
+  REQUIRE(false == utils::Environment::getEnvironmentVariable("UNSETENV2").first);
+}
+
+TEST_CASE("multithreaded environment manipulation", "[getenv][setenv][unsetenv]") {
+  std::vector<std::thread> threads;
+  for (size_t i = 0U; i < 16U; i++) {
+    threads.emplace_back([](){
+      std::mt19937 gen(std::random_device { }());
+      for (size_t i = 0U; i < 10240U; i++) {
+        const uint8_t env_num = gen() % 8;
+        const std::string env_name = "GETSETUNSETENV" + std::to_string(env_num);
+        const uint8_t operation = gen() % 3;
+        switch (operation) {
+          case 0: {
+              auto res = utils::Environment::getEnvironmentVariable(env_name.c_str());
+              break;
+            }
+          case 1: {
+              const size_t value_len = gen() % 256;
+              std::vector<char> value(value_len + 1, '\0');
+              std::generate_n(value.begin(), value_len, [&]() -> char {
+                return 'A' + gen() % static_cast<uint8_t>('Z' - 'A');
+              });
+              const bool overwrite = gen() % 2;
+              utils::Environment::setEnvironmentVariable(env_name.c_str(), value.data(), overwrite);
+              break;
+            }
+          case 2: {
+              utils::Environment::unsetEnvironmentVariable(env_name.c_str());
+              break;
+            }
+        }
+      }
+      });
+    }
+  for (auto& thread : threads) {
+    thread.join();
+  }
+  for (size_t i = 0U; i < 8U; i++) {
+    const std::string env_name = "GETSETUNSETENV" + std::to_string(i);
+    bool isset = false;
+    std::string value;
+    std::tie(isset, value) = utils::Environment::getEnvironmentVariable(env_name.c_str());
+    if (isset) {
+      std::cerr << env_name << " is set to " << value << std::endl;
 
 Review comment:
   I ain't no oracle, so I can't tell the results of random operations on multiple threads. The only real thing I'm concerned about is this not segfaulting (or triggering ASAN/valgrind). The outcome is not deterministic.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

arpadboda commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367312866
 
 

 ##########
 File path: libminifi/src/core/logging/Logger.cpp
 ##########
 @@ -0,0 +1,149 @@
+/**
+ *
+ * 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 "core/logging/Logger.h"
+
+#include <mutex>
+#include <memory>
+#include <sstream>
+#include <iostream>
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+
+
+LoggerControl::LoggerControl()
+    : is_enabled_(true) {
+}
+
+bool LoggerControl::is_enabled() const {
+  return is_enabled_;
+}
+
+void LoggerControl::setEnabled(bool status) {
+  is_enabled_ = status;
+}
+
+
+BaseLogger::~BaseLogger() {
+}
+
+bool BaseLogger::should_log(const LOG_LEVEL &level) {
+  return true;
+}
+
+LogBuilder::LogBuilder(BaseLogger *l, LOG_LEVEL level)
+    : ignore(false),
+      ptr(l),
+      level(level) {
+  if (!l->should_log(level)) {
+    setIgnore();
+  }
+}
+
+LogBuilder::~LogBuilder() {
+  if (!ignore)
+    log_string(level);
 
 Review comment:
   I know that this existed before, just a note: it's a virtual function call in the dtor. 

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

szaszm commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r367480050
 
 

 ##########
 File path: libminifi/include/core/logging/WindowsEventLogSink.h
 ##########
 @@ -0,0 +1,70 @@
+/**
+ *
+ * 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.
+ */
+
+#pragma once
+
+#ifdef WIN32
+
+#include "spdlog/common.h"
+#include "spdlog/sinks/base_sink.h"
+#include "spdlog/details/log_msg.h"
+#include "spdlog/details/null_mutex.h"
+
+#include <Windows.h>
+
+#include <string>
+
+
+namespace org {
+namespace apache {
+namespace nifi {
+namespace minifi {
+namespace core {
+namespace logging {
+namespace internal {
+
+class windowseventlog_sink : public spdlog::sinks::base_sink<spdlog::details::null_mutex> {
+ private:
+  HANDLE event_source_;
+
+  WORD type_from_level(const spdlog::details::log_msg& msg) const;
+
+  protected:
+   void _sink_it(const spdlog::details::log_msg& msg) override;
+
+   void _flush() override;
+
+  public:
+   windowseventlog_sink(const std::string& source_name = "ApacheNiFiMiNiFi");
+
+   virtual ~windowseventlog_sink();
+
+   windowseventlog_sink(const windowseventlog_sink&) = delete;
+   windowseventlog_sink& operator=(const windowseventlog_sink&) = delete;
+   windowseventlog_sink(windowseventlog_sink&&) = delete;
+   windowseventlog_sink& operator=(windowseventlog_sink&&) = delete;
+};
+}
+}
+}
+}
+}
+}
+}
 
 Review comment:
   missing namespace end comments

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [nifi-minifi-cpp] bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic

Posted by GitBox <gi...@apache.org>.

bakaid commented on a change in pull request #709: MINIFICPP-1088 - clean up minifiexe and MINIFI_HOME logic
URL: https://github.com/apache/nifi-minifi-cpp/pull/709#discussion_r366869925
 
 

 ##########
 File path: main/MiNiFiMain.cpp
 ##########
 @@ -142,94 +159,39 @@ int main(int argc, char **argv) {
 #ifdef WIN32
 	if (!SetConsoleCtrlHandler(consoleSignalHandler, TRUE)) {
 		logger->log_error("Cannot install signal handler");
-		std::cerr << "Cannot install signal handler" << std::endl;
-		return 1;
+		return -1;
 	}
 
 	if (signal(SIGINT, sigHandler) == SIG_ERR || signal(SIGTERM, sigHandler) == SIG_ERR ) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+		logger->log_error("Cannot install signal handler");
 		return -1;
 	}
 #ifdef SIGBREAK
 	if (signal(SIGBREAK, sigHandler) == SIG_ERR) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+		logger->log_error("Cannot install signal handler");
 		return -1;
 	}
 #endif
 #else
 	if (signal(SIGINT, sigHandler) == SIG_ERR || signal(SIGTERM, sigHandler) == SIG_ERR || signal(SIGPIPE, SIG_IGN) == SIG_ERR) {
-		std::cerr << "Cannot install signal handler" << std::endl;
+    logger->log_error("Cannot install signal handler");
 		return -1;
 	}
 #endif
-	// assumes POSIX compliant environment
 
 Review comment:
   The original order of sources for MINIFI_HOME here was
   for *nix: MINIFI_HOME env -> realpath(argv[0]) (path of the current executable) -> cwd
   for Windows: MINIFI_HOME env -> GetModuleHandle (path of the current executable) -> cwd
   This has been cleaned up and unified in `determineMinifiHome` as:
    MINIFI_HOME env -> path of the current executable -> cwd
   The method used for determining the path of the current executable used now is superior to `realpath(argv[0])`

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services