Add windows tests #595

phrocker · 2019-06-18T11:48:05Z

Still a WIP..

To enable tests on windows I needed to either resolve many dependency issues with linkages or take a more appropriate step, which was make EL a true extension and create a way the process context dynamically. Still have yet to go through and cleanup. Would like to get most tests passing, but there are some tests which still fail in a way that make me suspect of underlying windows code.

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?
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.

phrocker · 2019-06-18T16:20:02Z

CMakeLists.txt

@@ -130,6 +143,8 @@ endif()
 include(CheckCXXCompilerFlag)
 if (WIN32)
 	add_definitions(-DWIN32_LEAN_AND_MEAN)
+	add_definitions(-D_CRT_SECURE_NO_WARNINGS)
+	add_compile_options("/W1")


W3 is likely more appropriate

phrocker · 2019-06-18T16:20:24Z

CMakeLists.txt

@@ -188,12 +204,12 @@ if (NOT OPENSSL_OFF)

 	else()
 		include(LibreSSL)
-		use_libre_ssl(${CMAKE_CURRENT_SOURCE_DIR} ${CMAKE_CURRENT_BINARY_DIR})
+		use_libre_ssl("${CMAKE_CURRENT_SOURCE_DIR}" "${CMAKE_CURRENT_BINARY_DIR}")


This was to address some odd issues on windows. Same as one below

CMakeLists.txt

phrocker · 2019-06-18T17:08:07Z

CMakeLists.txt

                -DCURL_DISABLE_CRYPTO_AUTH=ON
                -DCMAKE_USE_LIBSSH2=OFF
+				"-DCMAKE_DEBUG_POSTFIX="


odd spacing.

phrocker · 2019-06-18T17:09:20Z

CMakeLists.txt

@@ -437,21 +406,16 @@ if(BOOTSTRAP)
    return()
 endif()

-message("-------- BOOTSTRAP BEGIN --------")


This is not needed at all.

phrocker · 2019-06-18T17:09:45Z

CMakeLists.txt

@@ -414,21 +398,6 @@ include_directories(thirdparty/yaml-cpp-yaml-cpp-20171024/include)
 include_directories(thirdparty/rapidjson-1.1.0/include)

 ## Expression language extensions
-option(DISABLE_EXPRESSION_LANGUAGE "Disables the scripting extensions." OFF)


Removed because of changes to how we now build EL

phrocker · 2019-06-18T17:10:33Z

cmake/LibreSSL.cmake

-	URL "https://ftp.openbsd.org/pub/OpenBSD/LibreSSL/libressl-2.8.3.tar.gz"
+	### default is openbsd.org -- cloudflare is a reliable mirror
+	#URL "https://ftp.openbsd.org/pub/OpenBSD/LibreSSL/libressl-2.8.3.tar.gz"
+	URL "https://cloudflare.cdn.openbsd.org/pub/OpenBSD/LibreSSL/libressl-2.8.3.tar.gz"


during the time of this PR the original OpenBSD link failed.

phrocker · 2019-06-18T17:11:24Z

appveyor.yml


 environment:
  MSVC_DEFAULT_OPTIONS: ON
  APPVEYOR_SAVE_CACHE_ON_ERROR: true
-  GENERATOR: Ninja


switching to win_build script

phrocker · 2019-06-19T17:14:17Z

CMakeSettings.json

@@ -2,14 +2,14 @@
  // See https://go.microsoft.com//fwlink//?linkid=834763 for more information about this file.
  "configurations": [
    {
-      "name": "x64-RelWithDebInfo",
-      "generator": "Ninja",


This will be an option through the build script.

phrocker · 2019-06-19T19:14:03Z

thirdparty/rocksdb/CMakeLists.txt

@@ -620,15 +620,15 @@ target_link_libraries(${ROCKSDB_STATIC_LIB}
  ${THIRDPARTY_LIBS} ${SYSTEM_LIBS})

 if(WIN32)
-  add_library(${ROCKSDB_IMPORT_LIB} SHARED ${SOURCES})
+#  add_library(${ROCKSDB_IMPORT_LIB} SHARED ${SOURCES})


should remove these commented lines

nanofi/tests/CAPITests.cpp

phrocker · 2019-06-20T12:11:35Z

CMakeLists.txt

 option(DISABLE_CIVET "Disables CivetWeb components." OFF)
 if (NOT DISABLE_CIVET)
-	createExtension(CIVETWEB CIVETWEB "This enables ListenHTTP" "extensions/civetweb" "${TEST_DIR}/civetweb-tests")


test moved to standard processors. civetweb is an extension that really isn't an extension at all because it's required for every other extension. Could be part of core, but it could be disabled if we were willing to disable curl support.

phrocker · 2019-06-20T12:12:42Z

cmake/BuildTests.cmake

@@ -130,6 +132,7 @@ FOREACH(testfile ${UNIT_TESTS})
 ENDFOREACH()
 message("-- Finished building ${UNIT_TEST_COUNT} unit test file(s)...")

+if(NOT WIN32)


Leaving this for a subsequent PR.

phrocker · 2019-06-20T12:13:13Z

extensions/coap/CMakeLists.txt

@@ -70,19 +70,6 @@ include_directories(${COAP_INCLUDE_DIRS})
 target_link_libraries (nanofi-coap-c ${COAP_LIBRARIES})
 target_link_libraries (nanofi-coap-c ${HTTP-CURL})
 target_link_libraries (minifi-coap nanofi-coap-c ${COAP_LIBRARIES})
-if (WIN32)


Some of the cruft that has been copy/pasted. Should consider stubbing out some of this into modular cmake in the future.

phrocker · 2019-06-20T12:13:36Z

extensions/expression-language/ExpressionContextBuilder.h

+namespace expressions {
+
+/**
+


need a comment as to why this exists.

My gut reaction is: why is this necessary for this PR? This needs to be explained that linking was problematic given the expression language extension was required in some form. This meant that on Windows, where linking is different, we couldn't enable tests without ensuring the pared down EL existed everywhere. To avoid this, a paradigm I've avoided for quite some time was put into place: hot loading the process context through another artifact.

phrocker · 2019-06-20T12:13:51Z

extensions/expression-language/ExpressionLoader.h

+
+  virtual std::unique_ptr<ObjectFactory> assign(const std::string &class_name) override {
+    if (utils::StringUtils::equalsIgnoreCase(class_name, "ProcessContextBuilder")) {
+      std::cout << "oh boi" << std::endl;


phrocker · 2019-06-20T12:14:18Z

extensions/expression-language/ExpressionLoader.h

+  virtual std::unique_ptr<ObjectFactory> assign(const std::string &class_name) override {
+    if (utils::StringUtils::equalsIgnoreCase(class_name, "ProcessContextBuilder")) {
+      std::cout << "oh boi" << std::endl;
+      return std::unique_ptr<ObjectFactory>(new core::DefautObjectFactory<core::expressions::ExpressionContextBuilder>());


In this case we are using a managed ProcessContextBuilder to support instantiating a different object to construct an EL context.

phrocker · 2019-06-20T12:14:46Z

extensions/expression-language/ProcessContextExpr.cpp

  }
  auto name = property.getName();
  if (expressions_.find(name) == expressions_.end()) {
    std::string expression_str;
-    getProperty(name, expression_str);


This extension does not appear to be linted.

phrocker · 2019-06-20T12:15:06Z

extensions/expression-language/ProcessContextExpr.h

+ public:
+
+  /**
+   std::forward of argument list did not work on all platforms.


comment doesn't have much context about its reasoning.

phrocker · 2019-06-20T12:15:44Z

extensions/expression-language/impl/expression/Expression.h

-#include <Value.h>
-#include <core/FlowFile.h>
-#include <core/VariableRegistry.h>
+#include "../../common/Value.h"


I think this relative path was added by way of the IDE's auto rename. should fix.

phrocker · 2019-06-20T12:16:27Z

extensions/expression-language/tests/ExpressionLanguageTests.cpp

@@ -187,13 +195,13 @@ TEST_CASE("GetFile PutFile dynamic attribute", "[expressionLanguageTestGetFilePu
  auto repo = std::make_shared<TestRepository>();

  std::string in_dir("/tmp/gt.XXXXXX");
-  REQUIRE(testController.createTempDirectory(&in_dir[0]) != nullptr);
+  REQUIRE(!testController.createTempDirectory(&in_dir[0]).empty());


This doesn't serve a great deal of purpose. we should be using the temp dirs not just throwing them away.

phrocker · 2019-06-20T12:17:05Z

extensions/http-curl/tests/C2JstackTest.cpp

@@ -121,7 +121,6 @@ int main(int argc, char **argv) {

  configuration->set("c2.rest.url", "http://localhost:8727/update");
  configuration->set("c2.agent.heartbeat.period", "1000");
-  mkdir("content_repository", S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH);


content repo isn't needed for this test and will be auto-created if need be.

phrocker · 2019-06-20T12:17:35Z

extensions/http-curl/tests/C2VerifyHeartbeatAndStop.cpp

@@ -63,7 +63,7 @@ class Responder : public CivetHandler {
    std::string response;
    int blockSize = 1024 * sizeof(char), readBytes;

-    char buffer[blockSize];
+    char buffer[1024];


Windows pointed this out. No point in keeping blockSize around.

phrocker · 2019-06-20T12:18:34Z

extensions/standard-processors/processors/GetFile.cpp

@@ -52,50 +52,44 @@ namespace minifi {
 namespace processors {

 core::Property GetFile::BatchSize(
-    core::PropertyBuilder::createProperty("Batch Size")->withDescription("The maximum number of files to pull in each iteration")->withDefaultValue<uint32_t>(10)
-        ->build());


artifact of fixing linter errors with eclipse. this is easily avoided by ignoring white space in the review.

phrocker · 2019-06-20T12:19:08Z

extensions/standard-processors/processors/GetFile.cpp

@@ -163,11 +157,13 @@ void GetFile::onTrigger(core::ProcessContext *context, core::ProcessSession *ses
    if (request_.pollInterval == 0 || (getTimeMillis() - last_listing_time_) > request_.pollInterval) {
      std::string directory;
      const std::shared_ptr<core::FlowFile> flow_file;
-      if (!context->getProperty(Directory, directory, flow_file)) {
+      if (context->getProperty(Directory, directory, flow_file)) {


This appeared to be a logical error found via windows tests.

phrocker · 2019-06-20T12:19:53Z

extensions/standard-processors/tests/.minifi-tmpfile.txt

@@ -0,0 +1,2 @@
+one,two,three


I think our gitignore needs a little work for the windows side...

phrocker · 2019-06-20T12:20:11Z

extensions/standard-processors/tests/unit/ExtractTextTests.cpp

@@ -67,7 +67,7 @@ TEST_CASE("Test usage of ExtractText", "[extracttextTest]") {

    char dir[] = "/tmp/gt.XXXXXX";

-    REQUIRE(testController.createTempDirectory(dir) != nullptr);
+    REQUIRE(!testController.createTempDirectory(dir).empty());


again I don't know the purpose of this. This test probably fails currently.

phrocker · 2019-06-20T12:44:25Z

Windows.md

+
+## Building via the build script
+
+The preferred way of building the project is via the win_build_vs.bat script found in our root source folder. You must supply a single command, the build directory. Typically you create a directory


There are additional options. Should add explanations.

apiri · 2019-07-21T19:18:17Z

@phrocker will scope it out.

apiri · 2019-07-22T00:59:08Z

@phrocker had a few stumbles verifying but seems like my VS environment and path was a little messed up in conjunction with not reading the README updates. Following the approaches outlined in the README got me consistent builds using both the profiles in the VS functionality as well as the build script. My WIX installation was good to get MSIs. Overall, build looked good and process was certainly cleaner than the last time I had gone through the process.

apiri · 2019-07-22T01:00:18Z

Oh, and to further clarify, I just scoped out the Release w/ & w/o JNI configurations

phrocker · 2019-08-01T14:06:33Z

bootstrap.sh

@@ -472,7 +472,11 @@ build_cmake_command(){
  if [ $? == 0 ]; then
    echo "Using libcurl-openssl..."
  else
-    CMAKE_BUILD_COMMAND="${CMAKE_BUILD_COMMAND} -DUSE_CURL_NSS=true .."
+  	if [ "${USE_SHARED_LIBS}" = "${TRUE}" ]; then
+	    CMAKE_BUILD_COMMAND="${CMAKE_BUILD_COMMAND} -DUSE_CURL_NSS=true .."


this should be removed.

MINIFICPP-883: change appveyor to use winbuild MINIFICPP-833: remove Win64 req MINIFICPP-883: Minor update to move to new image MINIFICPP-796: Enable tests MINIFICPP-796: Update path creator MINIFICPP-796: Minor update MINIFICPP-796: more adjustments MINIFICPP-796: Updates to tests and cmake MINIFICPP-769: Fix inclusion of minifi MINIFICPP-796: remove nanofi MINIFICPP-796: Remove test MINIFICPP-796: Updates Update SecureSocketGetTCPTest.cpp Update appveyor.yml Update appveyor.yml Update appveyor.yml Update CMakeLists.txt com

fix linter errors yet another update update get file and its test update fix logic with test skipping

phrocker · 2019-08-15T16:18:18Z

@apiri @arpadboda was that comment a +1? I added some code to fix some bugs in tests. Some windows tests fail but I think we can address those later.

apiri · 2019-08-15T17:03:51Z

@phrocker Yes, in general with the caveats at the time, build and structure looked good here. Can give it another run through on my environment with the latest item you pushed, although scanning through it appears the changes are just whitespace in 421cb2e.

phrocker · 2019-08-16T14:08:54Z

@apiri thanks. pushed another fix for a unit test and to remove cout statements. will wait for a +1 to merge.

apiri

hey @phrocker,

I performed the build from a semi-clean Windows environment and things look good. I'm sure there will be more work to be done and things to uncover but seems like a great foundation to work from and we can call this initial effort complete. I'll leave it to your preference as to how you wish to merge all these commits in there :)

phrocker commented Jun 18, 2019

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

phrocker commented Jun 18, 2019

View reviewed changes

CMakeLists.txt Outdated

-DCURL_DISABLE_CRYPTO_AUTH=ON

-DCMAKE_USE_LIBSSH2=OFF

"-DCMAKE_DEBUG_POSTFIX="

Copy link

Contributor Author

phrocker Jun 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

odd spacing.

phrocker mentioned this pull request Jun 18, 2019

MINIFICPP-883: Add windows build script #571

Closed

8 tasks

phrocker commented Jun 18, 2019

View reviewed changes

phrocker commented Jun 19, 2019

View reviewed changes

nanofi/tests/CAPITests.cpp Outdated Show resolved Hide resolved

phrocker commented Jun 20, 2019

View reviewed changes

phrocker force-pushed the MINIFICPP-796 branch from 51f7250 to 48129d5 Compare June 20, 2019 12:29

phrocker commented Jun 20, 2019

View reviewed changes

phrocker commented Aug 1, 2019

View reviewed changes

phrocker force-pushed the MINIFICPP-796 branch from 623d2c7 to c1ff70e Compare August 15, 2019 15:32

phrocker added 11 commits August 15, 2019 11:38

fix more tests

e4dab14

fix linter errors yet another update update get file and its test update fix logic with test skipping

MINIFICPP-796: Review updates

0294497

Don't rely on NSS when libressl is being built

f89b548

MINIFICPP-932: Don't rely on NSS

2e5abab

MINIFICPP-796: Fix cmake after rebase

10ec35c

MINIFICPP-796: Fix kafka

a4a9dad

MINIFICPP-796: fix tests

d491d4c

MINIFICPP-796: Updates after rebase

648e625

MINIFICPP-796: Fix linter errors

59ca738

MINIFICPP-796: Change default build to release

19483bf

phrocker force-pushed the MINIFICPP-796 branch from c1ff70e to 4ab790d Compare August 15, 2019 15:39

phrocker force-pushed the MINIFICPP-796 branch from 052fc8c to 223d176 Compare August 15, 2019 16:22

phrocker added 2 commits August 15, 2019 12:56

MINIFICPP-796: Update installer

768a857

MINIFICPP-796-848: fix tests

c58b654

phrocker force-pushed the MINIFICPP-796 branch from 223d176 to c58b654 Compare August 15, 2019 16:56

MINIFICPP-796-848: fix tests

421cb2e

MINIFICPP-796: Import test fix from master and remove cout statements

ed631db

MINIFICPP-796: Fix indentation

1c628a3

apiri approved these changes Aug 16, 2019

View reviewed changes

asfgit closed this in a6876db Aug 16, 2019

bakaid mentioned this pull request Aug 26, 2019

MINIFICPP-1014 - Fix SFTP extension build on MacOS #633

Merged

8 tasks


		## Building via the build script

		The preferred way of building the project is via the win_build_vs.bat script found in our root source folder. You must supply a single command, the build directory. Typically you create a directory

Add windows tests #595

Add windows tests #595

Conversation

phrocker commented Jun 18, 2019

For all changes:

For code changes:

For documentation related changes:

Note:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phrocker Jun 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phrocker Jun 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apiri commented Jul 21, 2019

apiri commented Jul 22, 2019

apiri commented Jul 22, 2019

Choose a reason for hiding this comment

phrocker commented Aug 15, 2019

apiri commented Aug 15, 2019

phrocker commented Aug 16, 2019

apiri left a comment

Choose a reason for hiding this comment

phrocker Jun 18, 2019 •

edited

Loading

phrocker Jun 20, 2019 •

edited

Loading