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/07/14 07:22:39 UTC
[GitHub] [nifi-minifi-cpp] hunyadi-dev opened a new pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
hunyadi-dev opened a new pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837
This is a draft PR used for running CI jobs on different platforms.
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
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r455637198
##########
File path: libminifi/include/core/PropertyValidation.h
##########
@@ -202,7 +202,7 @@ class UnsignedIntValidator : public PropertyValidator {
class LongValidator : public PropertyValidator {
public:
- explicit LongValidator(const std::string &name, int64_t min = (std::numeric_limits<int64_t>::min)(), int64_t max = (std::numeric_limits<int64_t>::max)())
+ explicit LongValidator(const std::string &name, int64_t min = std::numeric_limits<int64_t>::min(), int64_t max = (std::numeric_limits<int64_t>::max)())
Review comment:
Good catch, removed.
##########
File path: libminifi/src/utils/file/FileUtils.cpp
##########
@@ -40,7 +40,7 @@ uint64_t FileUtils::computeChecksum(const std::string &file_name, uint64_t up_to
while (stream && remaining_bytes_to_be_read > 0) {
// () around std::min are needed because Windows.h defines min (and max) as a macro
Review comment:
Good catch, removed.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r487110101
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ set(BYPRODUCT "lib/spdlog.lib")
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
+ endif()
+
+ # Set build options
+ set(SPDLOG_SOURCE_DIR "${BINARY_DIR}/thirdparty/spdlog-src")
+ set(SPDLOG_INSTALL_DIR "${BINARY_DIR}/thirdparty/spdlog-install")
+ set(SPDLOG_LIBRARY "${SPDLOG_INSTALL_DIR}/${BYPRODUCT}")
+ set(SPDLOG_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${SPDLOG_INSTALL_DIR}"
+ "-DSPDLOG_BUILD_EXAMPLE=OFF"
+ "-DSPDLOG_BUILD_TESTS=OFF"
+ "-DSPDLOG_BUILD_TESTING=OFF"
+ "-DSPDLOG_BUILD_BENCH=OFF"
+ "-DSPDLOG_BUILD_SHARED=OFF")
+
+ # Build project
+ ExternalProject_Add(
+ spdlog-external
+ URL "https://github.com/gabime/spdlog/archive/v1.7.0.zip"
Review comment:
Would you mind trying to bump this to 1.8.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
[GitHub] [nifi-minifi-cpp] szaszm commented on pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.8.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#issuecomment-731148336
I got rid of the cmake issue in `make debian`.
- by upgrading to debian 10 buster: https://github.com/szaszm/nifi-minifi-cpp/tree/spdlog_upgrade_debian_buster
- by using stretch-backports and stretch-backports-sloppy (due to an accidental upload to backports by the package maintainer): https://github.com/szaszm/nifi-minifi-cpp/tree/MINIFICPP-1121_spdlog_upgrade
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r463666731
##########
File path: libminifi/src/core/logging/LoggerConfiguration.cpp
##########
@@ -298,22 +299,23 @@ std::shared_ptr<spdlog::sinks::sink> LoggerConfiguration::create_syslog_sink() {
#ifdef WIN32
return std::make_shared<internal::windowseventlog_sink>("ApacheNiFiMiNiFi");
#else
- return std::make_shared<spdlog::sinks::syslog_sink>("ApacheNiFiMiNiFi");
+ return std::dynamic_pointer_cast<spdlog::sinks::sink>(spdlog::syslog_logger_mt("ApacheNiFiMiNiFi", 0, LOG_USER, false));
Review comment:
```suggestion
return spdlog::syslog_logger_mt("ApacheNiFiMiNiFi", 0, LOG_USER, false);
```
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r487110101
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ set(BYPRODUCT "lib/spdlog.lib")
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
+ endif()
+
+ # Set build options
+ set(SPDLOG_SOURCE_DIR "${BINARY_DIR}/thirdparty/spdlog-src")
+ set(SPDLOG_INSTALL_DIR "${BINARY_DIR}/thirdparty/spdlog-install")
+ set(SPDLOG_LIBRARY "${SPDLOG_INSTALL_DIR}/${BYPRODUCT}")
+ set(SPDLOG_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${SPDLOG_INSTALL_DIR}"
+ "-DSPDLOG_BUILD_EXAMPLE=OFF"
+ "-DSPDLOG_BUILD_TESTS=OFF"
+ "-DSPDLOG_BUILD_TESTING=OFF"
+ "-DSPDLOG_BUILD_BENCH=OFF"
+ "-DSPDLOG_BUILD_SHARED=OFF")
+
+ # Build project
+ ExternalProject_Add(
+ spdlog-external
+ URL "https://github.com/gabime/spdlog/archive/v1.7.0.zip"
Review comment:
Would you mind trying to bump this to 1.8.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
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.8.0
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r487698416
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ set(BYPRODUCT "lib/spdlog.lib")
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
+ endif()
+
+ # Set build options
+ set(SPDLOG_SOURCE_DIR "${BINARY_DIR}/thirdparty/spdlog-src")
+ set(SPDLOG_INSTALL_DIR "${BINARY_DIR}/thirdparty/spdlog-install")
+ set(SPDLOG_LIBRARY "${SPDLOG_INSTALL_DIR}/${BYPRODUCT}")
+ set(SPDLOG_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${SPDLOG_INSTALL_DIR}"
+ "-DSPDLOG_BUILD_EXAMPLE=OFF"
+ "-DSPDLOG_BUILD_TESTS=OFF"
+ "-DSPDLOG_BUILD_TESTING=OFF"
+ "-DSPDLOG_BUILD_BENCH=OFF"
+ "-DSPDLOG_BUILD_SHARED=OFF")
+
+ # Build project
+ ExternalProject_Add(
+ spdlog-external
+ URL "https://github.com/gabime/spdlog/archive/v1.7.0.zip"
Review comment:
Updated.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r456351621
##########
File path: LICENSE
##########
@@ -258,23 +258,27 @@ This product bundles 'spdlog' which is available under an MIT license.
Copyright (c) 2016 spdlog.
- Permission is hereby granted, free of charge, to any person obtaining a copy
- of this software and associated documentation files (the "Software"), to deal
- in the Software without restriction, including without limitation the rights
- to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- copies of the Software, and to permit persons to whom the Software is
- furnished to do so, subject to the following conditions:
-
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
-
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- THE SOFTWARE.
+ Permission is hereby granted, free of charge, to any person obtaining a copy
+ of this software and associated documentation files (the "Software"), to deal
+ in the Software without restriction, including without limitation the rights
+ to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ copies of the Software, and to permit persons to whom the Software is
+ furnished to do so, subject to the following conditions:
+
+ The above copyright notice and this permission notice shall be included in
+ all copies or substantial portions of the Software.
+
+ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ THE SOFTWARE.
+
+ -- NOTE: Third party dependency used by this software --
+ This software depends on the fmt lib (MIT License),
+ and users must comply to its license: https://github.com/fmtlib/fmt/blob/master/LICENSE.rst
Review comment:
first comment first bullet: In the `LICENSE` file lines 248-260, there is an outdated list of copyright notices. Please update those to reflect the current version of spdlog's [`LICENSE`](https://github.com/gabime/spdlog/blob/v1.7.0/LICENSE#L3) file.
Thanks for the updates. :)
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r456471246
##########
File path: NOTICE
##########
@@ -44,11 +44,13 @@ Amazon Technologies, Inc (http://www.amazon.com/).
THIRD PARTY COMPONENTS
**********************
This software includes third party software subject to the following copyrights:
-- XML parsing and utility functions from TinyXml2 - Lee Thomason.
-- JSON parsing and utility functions from JsonCpp - Copyright (c) 2007-2010 Baptiste Lepilleur.
+- Very fast, header-only/compiled, C++ logging library from spdlog - Copyright (c) 2016 Gabi Melman
+- An open-source formatting library for C++ from fmt - Copyright (c) 2012 - present, Victor Zverovich
+- XML parsing and utility functions from TinyXml2 - Lee Thomason
+- JSON parsing and utility functions from JsonCpp - Copyright (c) 2007-2010 Baptiste Lepilleur
- OpenSSL build files for cmake used for Android Builds - Copyright (C) 2007-2012 LuaDist and Copyright (C) 2013 Brian Sidebotham
- Android tool chain cmake build files - Copyright (c) 2010-2011, Ethan Rublee and Copyright (c) 2011-2014, Andrey Kamaev
-- gsl-lite - Copyright (c) 2015 Martin Moene and Copyright (c) 2015 Microsoft Corporation. All rights reserved.
+- gsl-lite - Copyright (c) 2015 Martin Moene and Copyright (c) 2015 Microsoft Corporation. All rights reserved
Review comment:
Ah, I did not know that these notices are literal. Will add it back again :)
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r456392073
##########
File path: NOTICE
##########
@@ -44,11 +44,13 @@ Amazon Technologies, Inc (http://www.amazon.com/).
THIRD PARTY COMPONENTS
**********************
This software includes third party software subject to the following copyrights:
-- XML parsing and utility functions from TinyXml2 - Lee Thomason.
-- JSON parsing and utility functions from JsonCpp - Copyright (c) 2007-2010 Baptiste Lepilleur.
+- Very fast, header-only/compiled, C++ logging library from spdlog - Copyright (c) 2016 Gabi Melman
+- An open-source formatting library for C++ from fmt - Copyright (c) 2012 - present, Victor Zverovich
+- XML parsing and utility functions from TinyXml2 - Lee Thomason
+- JSON parsing and utility functions from JsonCpp - Copyright (c) 2007-2010 Baptiste Lepilleur
- OpenSSL build files for cmake used for Android Builds - Copyright (C) 2007-2012 LuaDist and Copyright (C) 2013 Brian Sidebotham
- Android tool chain cmake build files - Copyright (c) 2010-2011, Ethan Rublee and Copyright (c) 2011-2014, Andrey Kamaev
-- gsl-lite - Copyright (c) 2015 Martin Moene and Copyright (c) 2015 Microsoft Corporation. All rights reserved.
+- gsl-lite - Copyright (c) 2015 Martin Moene and Copyright (c) 2015 Microsoft Corporation. All rights reserved
Review comment:
The dot was part of the original: https://github.com/gsl-lite/gsl-lite/blob/master/LICENSE#L4
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r458790534
##########
File path: CMakeLists.txt
##########
@@ -264,9 +264,12 @@ endif()
# spdlog
add_library(spdlog INTERFACE)
-target_include_directories(spdlog INTERFACE "${CMAKE_CURRENT_SOURCE_DIR}/thirdparty/spdlog-20170710/include")
+target_include_directories(spdlog INTERFACE "${CMAKE_CURRENT_SOURCE_DIR}/thirdparty/spdlog-1.7.0/include")
if (NOT WIN32)
set_property(TARGET spdlog APPEND PROPERTY INTERFACE_COMPILE_DEFINITIONS "SPDLOG_ENABLE_SYSLOG")
+else()
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DNOMINMAX")
Review comment:
this could go to the top level, around line 111, as `add_definitions(-DNOMINMAX)`, I think
##########
File path: CMakeLists.txt
##########
@@ -264,9 +264,12 @@ endif()
# spdlog
add_library(spdlog INTERFACE)
-target_include_directories(spdlog INTERFACE "${CMAKE_CURRENT_SOURCE_DIR}/thirdparty/spdlog-20170710/include")
+target_include_directories(spdlog INTERFACE "${CMAKE_CURRENT_SOURCE_DIR}/thirdparty/spdlog-1.7.0/include")
if (NOT WIN32)
set_property(TARGET spdlog APPEND PROPERTY INTERFACE_COMPILE_DEFINITIONS "SPDLOG_ENABLE_SYSLOG")
+else()
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DNOMINMAX")
Review comment:
these could go to the top level, around line 111, as `add_definitions(-DNOMINMAX)`, I think
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.8.0
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r487698416
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ set(BYPRODUCT "lib/spdlog.lib")
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
+ endif()
+
+ # Set build options
+ set(SPDLOG_SOURCE_DIR "${BINARY_DIR}/thirdparty/spdlog-src")
+ set(SPDLOG_INSTALL_DIR "${BINARY_DIR}/thirdparty/spdlog-install")
+ set(SPDLOG_LIBRARY "${SPDLOG_INSTALL_DIR}/${BYPRODUCT}")
+ set(SPDLOG_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${SPDLOG_INSTALL_DIR}"
+ "-DSPDLOG_BUILD_EXAMPLE=OFF"
+ "-DSPDLOG_BUILD_TESTS=OFF"
+ "-DSPDLOG_BUILD_TESTING=OFF"
+ "-DSPDLOG_BUILD_BENCH=OFF"
+ "-DSPDLOG_BUILD_SHARED=OFF")
+
+ # Build project
+ ExternalProject_Add(
+ spdlog-external
+ URL "https://github.com/gabime/spdlog/archive/v1.7.0.zip"
Review comment:
I don't see anything that shouldbe broken by the version update, done.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r487110101
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ set(BYPRODUCT "lib/spdlog.lib")
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
+ endif()
+
+ # Set build options
+ set(SPDLOG_SOURCE_DIR "${BINARY_DIR}/thirdparty/spdlog-src")
+ set(SPDLOG_INSTALL_DIR "${BINARY_DIR}/thirdparty/spdlog-install")
+ set(SPDLOG_LIBRARY "${SPDLOG_INSTALL_DIR}/${BYPRODUCT}")
+ set(SPDLOG_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${SPDLOG_INSTALL_DIR}"
+ "-DSPDLOG_BUILD_EXAMPLE=OFF"
+ "-DSPDLOG_BUILD_TESTS=OFF"
+ "-DSPDLOG_BUILD_TESTING=OFF"
+ "-DSPDLOG_BUILD_BENCH=OFF"
+ "-DSPDLOG_BUILD_SHARED=OFF")
+
+ # Build project
+ ExternalProject_Add(
+ spdlog-external
+ URL "https://github.com/gabime/spdlog/archive/v1.7.0.zip"
Review comment:
Would you mind trying to bump this to 1.8.0?
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ set(BYPRODUCT "lib/spdlog.lib")
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
+ endif()
+
+ # Set build options
+ set(SPDLOG_SOURCE_DIR "${BINARY_DIR}/thirdparty/spdlog-src")
+ set(SPDLOG_INSTALL_DIR "${BINARY_DIR}/thirdparty/spdlog-install")
+ set(SPDLOG_LIBRARY "${SPDLOG_INSTALL_DIR}/${BYPRODUCT}")
+ set(SPDLOG_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${SPDLOG_INSTALL_DIR}"
+ "-DSPDLOG_BUILD_EXAMPLE=OFF"
+ "-DSPDLOG_BUILD_TESTS=OFF"
+ "-DSPDLOG_BUILD_TESTING=OFF"
+ "-DSPDLOG_BUILD_BENCH=OFF"
+ "-DSPDLOG_BUILD_SHARED=OFF")
+
+ # Build project
+ ExternalProject_Add(
+ spdlog-external
+ URL "https://github.com/gabime/spdlog/archive/v1.7.0.zip"
Review comment:
Would you mind trying to bump this to 1.8.0?
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ set(BYPRODUCT "lib/spdlog.lib")
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
+ endif()
+
+ # Set build options
+ set(SPDLOG_SOURCE_DIR "${BINARY_DIR}/thirdparty/spdlog-src")
+ set(SPDLOG_INSTALL_DIR "${BINARY_DIR}/thirdparty/spdlog-install")
+ set(SPDLOG_LIBRARY "${SPDLOG_INSTALL_DIR}/${BYPRODUCT}")
+ set(SPDLOG_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${SPDLOG_INSTALL_DIR}"
+ "-DSPDLOG_BUILD_EXAMPLE=OFF"
+ "-DSPDLOG_BUILD_TESTS=OFF"
+ "-DSPDLOG_BUILD_TESTING=OFF"
+ "-DSPDLOG_BUILD_BENCH=OFF"
+ "-DSPDLOG_BUILD_SHARED=OFF")
+
+ # Build project
+ ExternalProject_Add(
+ spdlog-external
+ URL "https://github.com/gabime/spdlog/archive/v1.7.0.zip"
Review comment:
Would you mind trying to bump this to 1.8.0?
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ set(BYPRODUCT "lib/spdlog.lib")
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
+ endif()
+
+ # Set build options
+ set(SPDLOG_SOURCE_DIR "${BINARY_DIR}/thirdparty/spdlog-src")
+ set(SPDLOG_INSTALL_DIR "${BINARY_DIR}/thirdparty/spdlog-install")
+ set(SPDLOG_LIBRARY "${SPDLOG_INSTALL_DIR}/${BYPRODUCT}")
+ set(SPDLOG_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${SPDLOG_INSTALL_DIR}"
+ "-DSPDLOG_BUILD_EXAMPLE=OFF"
+ "-DSPDLOG_BUILD_TESTS=OFF"
+ "-DSPDLOG_BUILD_TESTING=OFF"
+ "-DSPDLOG_BUILD_BENCH=OFF"
+ "-DSPDLOG_BUILD_SHARED=OFF")
+
+ # Build project
+ ExternalProject_Add(
+ spdlog-external
+ URL "https://github.com/gabime/spdlog/archive/v1.7.0.zip"
Review comment:
Would you mind trying to bump this to 1.8.0?
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ set(BYPRODUCT "lib/spdlog.lib")
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
+ endif()
+
+ # Set build options
+ set(SPDLOG_SOURCE_DIR "${BINARY_DIR}/thirdparty/spdlog-src")
+ set(SPDLOG_INSTALL_DIR "${BINARY_DIR}/thirdparty/spdlog-install")
+ set(SPDLOG_LIBRARY "${SPDLOG_INSTALL_DIR}/${BYPRODUCT}")
+ set(SPDLOG_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${SPDLOG_INSTALL_DIR}"
+ "-DSPDLOG_BUILD_EXAMPLE=OFF"
+ "-DSPDLOG_BUILD_TESTS=OFF"
+ "-DSPDLOG_BUILD_TESTING=OFF"
+ "-DSPDLOG_BUILD_BENCH=OFF"
+ "-DSPDLOG_BUILD_SHARED=OFF")
+
+ # Build project
+ ExternalProject_Add(
+ spdlog-external
+ URL "https://github.com/gabime/spdlog/archive/v1.7.0.zip"
Review comment:
Would you mind trying to bump this to 1.8.0?
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ set(BYPRODUCT "lib/spdlog.lib")
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
+ endif()
+
+ # Set build options
+ set(SPDLOG_SOURCE_DIR "${BINARY_DIR}/thirdparty/spdlog-src")
+ set(SPDLOG_INSTALL_DIR "${BINARY_DIR}/thirdparty/spdlog-install")
+ set(SPDLOG_LIBRARY "${SPDLOG_INSTALL_DIR}/${BYPRODUCT}")
+ set(SPDLOG_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${SPDLOG_INSTALL_DIR}"
+ "-DSPDLOG_BUILD_EXAMPLE=OFF"
+ "-DSPDLOG_BUILD_TESTS=OFF"
+ "-DSPDLOG_BUILD_TESTING=OFF"
+ "-DSPDLOG_BUILD_BENCH=OFF"
+ "-DSPDLOG_BUILD_SHARED=OFF")
+
+ # Build project
+ ExternalProject_Add(
+ spdlog-external
+ URL "https://github.com/gabime/spdlog/archive/v1.7.0.zip"
Review comment:
Would you mind trying to bump this to 1.8.0?
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ set(BYPRODUCT "lib/spdlog.lib")
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
+ endif()
+
+ # Set build options
+ set(SPDLOG_SOURCE_DIR "${BINARY_DIR}/thirdparty/spdlog-src")
+ set(SPDLOG_INSTALL_DIR "${BINARY_DIR}/thirdparty/spdlog-install")
+ set(SPDLOG_LIBRARY "${SPDLOG_INSTALL_DIR}/${BYPRODUCT}")
+ set(SPDLOG_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${SPDLOG_INSTALL_DIR}"
+ "-DSPDLOG_BUILD_EXAMPLE=OFF"
+ "-DSPDLOG_BUILD_TESTS=OFF"
+ "-DSPDLOG_BUILD_TESTING=OFF"
+ "-DSPDLOG_BUILD_BENCH=OFF"
+ "-DSPDLOG_BUILD_SHARED=OFF")
+
+ # Build project
+ ExternalProject_Add(
+ spdlog-external
+ URL "https://github.com/gabime/spdlog/archive/v1.7.0.zip"
Review comment:
Would you mind trying to bump this to 1.8.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
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r464237584
##########
File path: libminifi/src/core/logging/LoggerConfiguration.cpp
##########
@@ -294,26 +295,27 @@ 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() {
+spdlog::sink_ptr LoggerConfiguration::create_syslog_sink() {
#ifdef WIN32
return std::make_shared<internal::windowseventlog_sink>("ApacheNiFiMiNiFi");
#else
- return std::make_shared<spdlog::sinks::syslog_sink>("ApacheNiFiMiNiFi");
+ return std::dynamic_pointer_cast<spdlog::sinks::sink>(spdlog::syslog_logger_mt("ApacheNiFiMiNiFi", 0, LOG_USER, false));
#endif
}
-std::shared_ptr<spdlog::sinks::sink> LoggerConfiguration::create_fallback_sink() {
+spdlog::sink_ptr LoggerConfiguration::create_fallback_sink() {
if (utils::Environment::isRunningAsService()) {
return LoggerConfiguration::create_syslog_sink();
} else {
- return spdlog::sinks::stderr_sink_mt::instance();
+ return std::dynamic_pointer_cast<spdlog::sinks::sink>(std::make_shared<spdlog::sinks::stderr_sink_mt>());
}
}
std::shared_ptr<internal::LoggerNamespace> LoggerConfiguration::create_default_root() {
std::shared_ptr<internal::LoggerNamespace> result = std::make_shared<internal::LoggerNamespace>();
result->sinks = std::vector<std::shared_ptr<spdlog::sinks::sink>>();
- result->sinks.push_back(spdlog::sinks::stderr_sink_mt::instance());
+ auto default_sink = std::make_shared<spdlog::sinks::stderr_sink_mt>();
+ result->sinks.emplace_back(std::move(default_sink));
Review comment:
That is a valid and useful refactor. I did not check the context on doing the update.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.8.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r529487132
##########
File path: docker/debian/Dockerfile
##########
@@ -28,7 +28,11 @@ ENV MINIFI_BASE_DIR /opt/minifi
ENV MINIFI_HOME $MINIFI_BASE_DIR/nifi-minifi-cpp-$MINIFI_VERSION
ENV JAVA_HOME /usr/lib/jvm/java-8-openjdk-amd64/
-RUN apt-get update && apt-get install -y openjdk-8-jdk libpython3.5-dev openjdk-8-source sudo git maven
+RUN echo "deb http://deb.debian.org/debian stretch-backports main" | tee -a /etc/apt/sources.list \
+ && echo "deb http://deb.debian.org/debian stretch-backports-sloppy main" | tee -a /etc/apt/sources.list \
+ && apt-get update && apt-get install -y openjdk-8-jdk libpython3.5-dev openjdk-8-source sudo git maven \
+ && apt-get -t stretch-backports-sloppy install -y libarchive13\
Review comment:
It's a dependency of cmake. The maintainer incorrectly uploaded cmake 3.16 (instead of 3.13) to the amd64 stretch-backports, which depends on libarchive >=3.3.3. The version in the stretch repos is 3.2 and there is no libarchive in stretch-backports, so this is a workaround.
See this bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=954852
and see the inconsistent version near the bottom of this page: https://packages.debian.org/stretch-backports/cmake
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r456418735
##########
File path: LICENSE
##########
@@ -258,23 +258,27 @@ This product bundles 'spdlog' which is available under an MIT license.
Copyright (c) 2016 spdlog.
- Permission is hereby granted, free of charge, to any person obtaining a copy
- of this software and associated documentation files (the "Software"), to deal
- in the Software without restriction, including without limitation the rights
- to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- copies of the Software, and to permit persons to whom the Software is
- furnished to do so, subject to the following conditions:
-
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
-
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- THE SOFTWARE.
+ Permission is hereby granted, free of charge, to any person obtaining a copy
+ of this software and associated documentation files (the "Software"), to deal
+ in the Software without restriction, including without limitation the rights
+ to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ copies of the Software, and to permit persons to whom the Software is
+ furnished to do so, subject to the following conditions:
+
+ The above copyright notice and this permission notice shall be included in
+ all copies or substantial portions of the Software.
+
+ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ THE SOFTWARE.
+
+ -- NOTE: Third party dependency used by this software --
+ This software depends on the fmt lib (MIT License),
+ and users must comply to its license: https://github.com/fmtlib/fmt/blob/master/LICENSE.rst
Review comment:
Ah, I have seen those lines, but I do not know why we have them. At no point did spdlog have them listed in its `LICENSE` ([[see here]](https://github.com/gabime/spdlog/commits/v1.x/LICENSE)), but their names were present in the codebase as copyright holders. For some reason [[Aldrin asked us]](https://github.com/apache/nifi-minifi-cpp/pull/55) to provide these in our notice as well.
I updated the lines so that they are in correspondence with the current copyright notices in the spdlog files.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r463668961
##########
File path: libminifi/src/core/logging/LoggerConfiguration.cpp
##########
@@ -294,26 +295,27 @@ 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() {
+spdlog::sink_ptr LoggerConfiguration::create_syslog_sink() {
#ifdef WIN32
return std::make_shared<internal::windowseventlog_sink>("ApacheNiFiMiNiFi");
#else
- return std::make_shared<spdlog::sinks::syslog_sink>("ApacheNiFiMiNiFi");
+ return std::dynamic_pointer_cast<spdlog::sinks::sink>(spdlog::syslog_logger_mt("ApacheNiFiMiNiFi", 0, LOG_USER, false));
#endif
}
-std::shared_ptr<spdlog::sinks::sink> LoggerConfiguration::create_fallback_sink() {
+spdlog::sink_ptr LoggerConfiguration::create_fallback_sink() {
if (utils::Environment::isRunningAsService()) {
return LoggerConfiguration::create_syslog_sink();
} else {
- return spdlog::sinks::stderr_sink_mt::instance();
+ return std::dynamic_pointer_cast<spdlog::sinks::sink>(std::make_shared<spdlog::sinks::stderr_sink_mt>());
}
}
std::shared_ptr<internal::LoggerNamespace> LoggerConfiguration::create_default_root() {
std::shared_ptr<internal::LoggerNamespace> result = std::make_shared<internal::LoggerNamespace>();
result->sinks = std::vector<std::shared_ptr<spdlog::sinks::sink>>();
- result->sinks.push_back(spdlog::sinks::stderr_sink_mt::instance());
+ auto default_sink = std::make_shared<spdlog::sinks::stderr_sink_mt>();
+ result->sinks.emplace_back(std::move(default_sink));
Review comment:
Instead of the 3 lines above:
```
result->sinks = { std::make_shared<spdlog::sinks::stderr_sink_mt>(); };
```
And `result->sinks` should become a vector of sink_ptrs.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r462925415
##########
File path: libminifi/src/core/logging/LoggerConfiguration.cpp
##########
@@ -298,22 +299,23 @@ std::shared_ptr<spdlog::sinks::sink> LoggerConfiguration::create_syslog_sink() {
#ifdef WIN32
return std::make_shared<internal::windowseventlog_sink>("ApacheNiFiMiNiFi");
#else
- return std::make_shared<spdlog::sinks::syslog_sink>("ApacheNiFiMiNiFi");
+ return std::dynamic_pointer_cast<spdlog::sinks::sink>(spdlog::syslog_logger_mt("ApacheNiFiMiNiFi", 0, LOG_USER, false));
Review comment:
`spdlog::sink_ptr` seems to be the type to be used for type-erasing sink types, so I recommend changing the return type to that instead of casting. Applies to all create_* functions.
See the example at:
https://github.com/gabime/spdlog/wiki/2.-Creating-loggers#creating-loggers-with-multiple-sinks
##########
File path: libminifi/test/TestBase.cpp
##########
@@ -21,7 +21,8 @@
#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));
+ const fmt::basic_string_view<char> levelView(spdlog::level::to_string_view(level));
Review comment:
Use `auto` to be forward-compatible with `std::string_view` should the spdlog return type ever change.
https://github.com/gabime/spdlog/blob/eb23d505f86f01d61b10ef0899f2f7b2b377ec6d/include/spdlog/fmt/bundled/core.h#L190
##########
File path: LICENSE
##########
@@ -258,23 +258,27 @@ This product bundles 'spdlog' which is available under an MIT license.
Copyright (c) 2016 spdlog.
- Permission is hereby granted, free of charge, to any person obtaining a copy
- of this software and associated documentation files (the "Software"), to deal
- in the Software without restriction, including without limitation the rights
- to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- copies of the Software, and to permit persons to whom the Software is
- furnished to do so, subject to the following conditions:
-
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
-
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- THE SOFTWARE.
+ Permission is hereby granted, free of charge, to any person obtaining a copy
+ of this software and associated documentation files (the "Software"), to deal
+ in the Software without restriction, including without limitation the rights
+ to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ copies of the Software, and to permit persons to whom the Software is
+ furnished to do so, subject to the following conditions:
+
+ The above copyright notice and this permission notice shall be included in
+ all copies or substantial portions of the Software.
+
+ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ THE SOFTWARE.
+
+ -- NOTE: Third party dependency used by this software --
+ This software depends on the fmt lib (MIT License),
+ and users must comply to its license: https://github.com/fmtlib/fmt/blob/master/LICENSE.rst
Review comment:
I meant that we should keep the copyright notices, I take back my original first bullet.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r455632531
##########
File path: libminifi/src/utils/file/FileUtils.cpp
##########
@@ -40,7 +40,7 @@ uint64_t FileUtils::computeChecksum(const std::string &file_name, uint64_t up_to
while (stream && remaining_bytes_to_be_read > 0) {
// () around std::min are needed because Windows.h defines min (and max) as a macro
Review comment:
this comment is no longer needed
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.8.0
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#issuecomment-741612258
@szaszm rebased and updated again...
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.8.0
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r529354529
##########
File path: docker/debian/Dockerfile
##########
@@ -28,7 +28,11 @@ ENV MINIFI_BASE_DIR /opt/minifi
ENV MINIFI_HOME $MINIFI_BASE_DIR/nifi-minifi-cpp-$MINIFI_VERSION
ENV JAVA_HOME /usr/lib/jvm/java-8-openjdk-amd64/
-RUN apt-get update && apt-get install -y openjdk-8-jdk libpython3.5-dev openjdk-8-source sudo git maven
+RUN echo "deb http://deb.debian.org/debian stretch-backports main" | tee -a /etc/apt/sources.list \
+ && echo "deb http://deb.debian.org/debian stretch-backports-sloppy main" | tee -a /etc/apt/sources.list \
+ && apt-get update && apt-get install -y openjdk-8-jdk libpython3.5-dev openjdk-8-source sudo git maven \
+ && apt-get -t stretch-backports-sloppy install -y libarchive13\
Review comment:
Why do we need the system libarchive? We bundle that.
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -0,0 +1,74 @@
+# 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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "lib/spdlogd.lib")
+ else()
+ set(BYPRODUCT "lib/spdlog.lib")
+ endif()
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
Review comment:
What happened here? I think I remember a commit which applied these changes, but now the generator expressions are not there.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r456271769
##########
File path: LICENSE
##########
@@ -258,23 +258,27 @@ This product bundles 'spdlog' which is available under an MIT license.
Copyright (c) 2016 spdlog.
- Permission is hereby granted, free of charge, to any person obtaining a copy
- of this software and associated documentation files (the "Software"), to deal
- in the Software without restriction, including without limitation the rights
- to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- copies of the Software, and to permit persons to whom the Software is
- furnished to do so, subject to the following conditions:
-
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
-
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- THE SOFTWARE.
+ Permission is hereby granted, free of charge, to any person obtaining a copy
+ of this software and associated documentation files (the "Software"), to deal
+ in the Software without restriction, including without limitation the rights
+ to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ copies of the Software, and to permit persons to whom the Software is
+ furnished to do so, subject to the following conditions:
+
+ The above copyright notice and this permission notice shall be included in
+ all copies or substantial portions of the Software.
+
+ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ THE SOFTWARE.
+
+ -- NOTE: Third party dependency used by this software --
+ This software depends on the fmt lib (MIT License),
+ and users must comply to its license: https://github.com/fmtlib/fmt/blob/master/LICENSE.rst
Review comment:
I am not exactly sure how the first comments first bullet differs from the second comment. Updated the `NOTICE` and added a section for fmt in this `LICENSE` 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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r455658255
##########
File path: LICENSE
##########
@@ -258,23 +258,27 @@ This product bundles 'spdlog' which is available under an MIT license.
Copyright (c) 2016 spdlog.
- Permission is hereby granted, free of charge, to any person obtaining a copy
- of this software and associated documentation files (the "Software"), to deal
- in the Software without restriction, including without limitation the rights
- to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- copies of the Software, and to permit persons to whom the Software is
- furnished to do so, subject to the following conditions:
-
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
-
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- THE SOFTWARE.
+ Permission is hereby granted, free of charge, to any person obtaining a copy
+ of this software and associated documentation files (the "Software"), to deal
+ in the Software without restriction, including without limitation the rights
+ to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ copies of the Software, and to permit persons to whom the Software is
+ furnished to do so, subject to the following conditions:
+
+ The above copyright notice and this permission notice shall be included in
+ all copies or substantial portions of the Software.
+
+ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ THE SOFTWARE.
+
+ -- NOTE: Third party dependency used by this software --
+ This software depends on the fmt lib (MIT License),
+ and users must comply to its license: https://github.com/fmtlib/fmt/blob/master/LICENSE.rst
Review comment:
And please update our NOTICE file to include spdlog, fmt and their respective copyright notices.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r456351621
##########
File path: LICENSE
##########
@@ -258,23 +258,27 @@ This product bundles 'spdlog' which is available under an MIT license.
Copyright (c) 2016 spdlog.
- Permission is hereby granted, free of charge, to any person obtaining a copy
- of this software and associated documentation files (the "Software"), to deal
- in the Software without restriction, including without limitation the rights
- to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- copies of the Software, and to permit persons to whom the Software is
- furnished to do so, subject to the following conditions:
-
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
-
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- THE SOFTWARE.
+ Permission is hereby granted, free of charge, to any person obtaining a copy
+ of this software and associated documentation files (the "Software"), to deal
+ in the Software without restriction, including without limitation the rights
+ to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ copies of the Software, and to permit persons to whom the Software is
+ furnished to do so, subject to the following conditions:
+
+ The above copyright notice and this permission notice shall be included in
+ all copies or substantial portions of the Software.
+
+ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ THE SOFTWARE.
+
+ -- NOTE: Third party dependency used by this software --
+ This software depends on the fmt lib (MIT License),
+ and users must comply to its license: https://github.com/fmtlib/fmt/blob/master/LICENSE.rst
Review comment:
first comment first bullet: In the `LICENSE` file lines 248-260, there is an outdated list of copyright notices for spdlog. Please update those to reflect the current version of spdlog's [`LICENSE`](https://github.com/gabime/spdlog/blob/v1.7.0/LICENSE#L3) file.
Thanks for the updates. :)
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r455631883
##########
File path: libminifi/include/core/PropertyValidation.h
##########
@@ -202,7 +202,7 @@ class UnsignedIntValidator : public PropertyValidator {
class LongValidator : public PropertyValidator {
public:
- explicit LongValidator(const std::string &name, int64_t min = (std::numeric_limits<int64_t>::min)(), int64_t max = (std::numeric_limits<int64_t>::max)())
+ explicit LongValidator(const std::string &name, int64_t min = std::numeric_limits<int64_t>::min(), int64_t max = (std::numeric_limits<int64_t>::max)())
Review comment:
the `()` around `max` can be removed, too
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm closed pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.8.0
Posted by GitBox <gi...@apache.org>.
szaszm closed pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] fgerlits commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.8.0
Posted by GitBox <gi...@apache.org>.
fgerlits commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r529371181
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -0,0 +1,74 @@
+# 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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "lib/spdlogd.lib")
+ else()
+ set(BYPRODUCT "lib/spdlog.lib")
+ endif()
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
Review comment:
What happened here? I think I remember a commit which changed these to generator expressions, but now the generator expressions are not there.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r456351621
##########
File path: LICENSE
##########
@@ -258,23 +258,27 @@ This product bundles 'spdlog' which is available under an MIT license.
Copyright (c) 2016 spdlog.
- Permission is hereby granted, free of charge, to any person obtaining a copy
- of this software and associated documentation files (the "Software"), to deal
- in the Software without restriction, including without limitation the rights
- to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- copies of the Software, and to permit persons to whom the Software is
- furnished to do so, subject to the following conditions:
-
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
-
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- THE SOFTWARE.
+ Permission is hereby granted, free of charge, to any person obtaining a copy
+ of this software and associated documentation files (the "Software"), to deal
+ in the Software without restriction, including without limitation the rights
+ to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ copies of the Software, and to permit persons to whom the Software is
+ furnished to do so, subject to the following conditions:
+
+ The above copyright notice and this permission notice shall be included in
+ all copies or substantial portions of the Software.
+
+ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ THE SOFTWARE.
+
+ -- NOTE: Third party dependency used by this software --
+ This software depends on the fmt lib (MIT License),
+ and users must comply to its license: https://github.com/fmtlib/fmt/blob/master/LICENSE.rst
Review comment:
In the `LICENSE` file lines 248-260, there is an outdated list of copyright notices. Please update those to reflect the current version of spdlog's [`LICENSE`](https://github.com/gabime/spdlog/blob/v1.7.0/LICENSE#L3) 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
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r456418735
##########
File path: LICENSE
##########
@@ -258,23 +258,27 @@ This product bundles 'spdlog' which is available under an MIT license.
Copyright (c) 2016 spdlog.
- Permission is hereby granted, free of charge, to any person obtaining a copy
- of this software and associated documentation files (the "Software"), to deal
- in the Software without restriction, including without limitation the rights
- to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- copies of the Software, and to permit persons to whom the Software is
- furnished to do so, subject to the following conditions:
-
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
-
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- THE SOFTWARE.
+ Permission is hereby granted, free of charge, to any person obtaining a copy
+ of this software and associated documentation files (the "Software"), to deal
+ in the Software without restriction, including without limitation the rights
+ to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ copies of the Software, and to permit persons to whom the Software is
+ furnished to do so, subject to the following conditions:
+
+ The above copyright notice and this permission notice shall be included in
+ all copies or substantial portions of the Software.
+
+ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ THE SOFTWARE.
+
+ -- NOTE: Third party dependency used by this software --
+ This software depends on the fmt lib (MIT License),
+ and users must comply to its license: https://github.com/fmtlib/fmt/blob/master/LICENSE.rst
Review comment:
Ah, I have seen those lines, but I do not know where they are coming from. At no point did spdlog have them listed in its `LICENSE` ([[see here]](https://github.com/gabime/spdlog/commits/v1.x/LICENSE)), but their names were present in the codebase as copyright holders. For some reason [[Aldrin asked us]](https://github.com/apache/nifi-minifi-cpp/pull/55) to provide these in our notice as well.
I updated the lines so that they are in correspondence with the current copyright notices in the spdlog files.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.8.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#issuecomment-741723722
This is the issue: https://github.com/apache/nifi-minifi-cpp/pull/837/checks?check_run_id=1522646567#step:5:14323
Similar to ec40383, probably just missing includes.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r456473493
##########
File path: NOTICE
##########
@@ -44,11 +44,13 @@ Amazon Technologies, Inc (http://www.amazon.com/).
THIRD PARTY COMPONENTS
**********************
This software includes third party software subject to the following copyrights:
-- XML parsing and utility functions from TinyXml2 - Lee Thomason.
-- JSON parsing and utility functions from JsonCpp - Copyright (c) 2007-2010 Baptiste Lepilleur.
+- Very fast, header-only/compiled, C++ logging library from spdlog - Copyright (c) 2016 Gabi Melman
+- An open-source formatting library for C++ from fmt - Copyright (c) 2012 - present, Victor Zverovich
+- XML parsing and utility functions from TinyXml2 - Lee Thomason
+- JSON parsing and utility functions from JsonCpp - Copyright (c) 2007-2010 Baptiste Lepilleur
- OpenSSL build files for cmake used for Android Builds - Copyright (C) 2007-2012 LuaDist and Copyright (C) 2013 Brian Sidebotham
- Android tool chain cmake build files - Copyright (c) 2010-2011, Ethan Rublee and Copyright (c) 2011-2014, Andrey Kamaev
-- gsl-lite - Copyright (c) 2015 Martin Moene and Copyright (c) 2015 Microsoft Corporation. All rights reserved.
+- gsl-lite - Copyright (c) 2015 Martin Moene and Copyright (c) 2015 Microsoft Corporation. All rights reserved
Review comment:
I don't know whether they need to be literal, but I tried to modify as little as possible while keeping then on a single line (for 1 3rd party)
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#issuecomment-671353828
Could you rebase to main? Two reasons: 1. there are conflicts, 2. there are more CI failures than the usual flickers, but they don't seem to be related, so I would like to see another run.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r455647101
##########
File path: LICENSE
##########
@@ -258,23 +258,27 @@ This product bundles 'spdlog' which is available under an MIT license.
Copyright (c) 2016 spdlog.
- Permission is hereby granted, free of charge, to any person obtaining a copy
- of this software and associated documentation files (the "Software"), to deal
- in the Software without restriction, including without limitation the rights
- to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- copies of the Software, and to permit persons to whom the Software is
- furnished to do so, subject to the following conditions:
-
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
-
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- THE SOFTWARE.
+ Permission is hereby granted, free of charge, to any person obtaining a copy
+ of this software and associated documentation files (the "Software"), to deal
+ in the Software without restriction, including without limitation the rights
+ to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ copies of the Software, and to permit persons to whom the Software is
+ furnished to do so, subject to the following conditions:
+
+ The above copyright notice and this permission notice shall be included in
+ all copies or substantial portions of the Software.
+
+ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ THE SOFTWARE.
+
+ -- NOTE: Third party dependency used by this software --
+ This software depends on the fmt lib (MIT License),
+ and users must comply to its license: https://github.com/fmtlib/fmt/blob/master/LICENSE.rst
Review comment:
Could you also
1. update the copyright notices and
2. include the fmt license text 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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r463668961
##########
File path: libminifi/src/core/logging/LoggerConfiguration.cpp
##########
@@ -294,26 +295,27 @@ 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() {
+spdlog::sink_ptr LoggerConfiguration::create_syslog_sink() {
#ifdef WIN32
return std::make_shared<internal::windowseventlog_sink>("ApacheNiFiMiNiFi");
#else
- return std::make_shared<spdlog::sinks::syslog_sink>("ApacheNiFiMiNiFi");
+ return std::dynamic_pointer_cast<spdlog::sinks::sink>(spdlog::syslog_logger_mt("ApacheNiFiMiNiFi", 0, LOG_USER, false));
#endif
}
-std::shared_ptr<spdlog::sinks::sink> LoggerConfiguration::create_fallback_sink() {
+spdlog::sink_ptr LoggerConfiguration::create_fallback_sink() {
if (utils::Environment::isRunningAsService()) {
return LoggerConfiguration::create_syslog_sink();
} else {
- return spdlog::sinks::stderr_sink_mt::instance();
+ return std::dynamic_pointer_cast<spdlog::sinks::sink>(std::make_shared<spdlog::sinks::stderr_sink_mt>());
}
}
std::shared_ptr<internal::LoggerNamespace> LoggerConfiguration::create_default_root() {
std::shared_ptr<internal::LoggerNamespace> result = std::make_shared<internal::LoggerNamespace>();
result->sinks = std::vector<std::shared_ptr<spdlog::sinks::sink>>();
- result->sinks.push_back(spdlog::sinks::stderr_sink_mt::instance());
+ auto default_sink = std::make_shared<spdlog::sinks::stderr_sink_mt>();
+ result->sinks.emplace_back(std::move(default_sink));
Review comment:
Instead of the 3 lines above:
```
result->sinks = { std::make_shared<spdlog::sinks::stderr_sink_mt>(); };
```
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.8.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r529488056
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -0,0 +1,74 @@
+# 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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "lib/spdlogd.lib")
+ else()
+ set(BYPRODUCT "lib/spdlog.lib")
+ endif()
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
Review comment:
https://gitlab.kitware.com/cmake/cmake/-/issues/19522
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.8.0
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#issuecomment-732312187
> I got rid of the cmake issue in `make debian`.
>
> * by upgrading to debian 10 buster: [https://github.com/szaszm/nifi-minifi-cpp/tree/spdlog_upgrade_debian_buster](https://github.com/szaszm/nifi-minifi-cpp/tree/spdlog_upgrade_debian_buster?rgh-link-date=2020-11-20T12%3A42%3A17Z)
>
> * by using stretch-backports and stretch-backports-sloppy (due to an accidental upload to backports by the package maintainer): [https://github.com/szaszm/nifi-minifi-cpp/tree/MINIFICPP-1121_spdlog_upgrade](https://github.com/szaszm/nifi-minifi-cpp/tree/MINIFICPP-1121_spdlog_upgrade?rgh-link-date=2020-11-20T12%3A42%3A17Z)
Thanks, that is a great addition, I cherry picked the latter to this PR.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r456474581
##########
File path: LICENSE
##########
@@ -258,23 +258,27 @@ This product bundles 'spdlog' which is available under an MIT license.
Copyright (c) 2016 spdlog.
- Permission is hereby granted, free of charge, to any person obtaining a copy
- of this software and associated documentation files (the "Software"), to deal
- in the Software without restriction, including without limitation the rights
- to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
- copies of the Software, and to permit persons to whom the Software is
- furnished to do so, subject to the following conditions:
-
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
-
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
- OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
- THE SOFTWARE.
+ Permission is hereby granted, free of charge, to any person obtaining a copy
+ of this software and associated documentation files (the "Software"), to deal
+ in the Software without restriction, including without limitation the rights
+ to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ copies of the Software, and to permit persons to whom the Software is
+ furnished to do so, subject to the following conditions:
+
+ The above copyright notice and this permission notice shall be included in
+ all copies or substantial portions of the Software.
+
+ THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ THE SOFTWARE.
+
+ -- NOTE: Third party dependency used by this software --
+ This software depends on the fmt lib (MIT License),
+ and users must comply to its license: https://github.com/fmtlib/fmt/blob/master/LICENSE.rst
Review comment:
Good point from Aldrin, I take back that point. It's too bad that spdlog's LICENSE file is incomplete... :(
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.8.0
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r487698416
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ set(BYPRODUCT "lib/spdlog.lib")
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
+ endif()
+
+ # Set build options
+ set(SPDLOG_SOURCE_DIR "${BINARY_DIR}/thirdparty/spdlog-src")
+ set(SPDLOG_INSTALL_DIR "${BINARY_DIR}/thirdparty/spdlog-install")
+ set(SPDLOG_LIBRARY "${SPDLOG_INSTALL_DIR}/${BYPRODUCT}")
+ set(SPDLOG_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${SPDLOG_INSTALL_DIR}"
+ "-DSPDLOG_BUILD_EXAMPLE=OFF"
+ "-DSPDLOG_BUILD_TESTS=OFF"
+ "-DSPDLOG_BUILD_TESTING=OFF"
+ "-DSPDLOG_BUILD_BENCH=OFF"
+ "-DSPDLOG_BUILD_SHARED=OFF")
+
+ # Build project
+ ExternalProject_Add(
+ spdlog-external
+ URL "https://github.com/gabime/spdlog/archive/v1.7.0.zip"
Review comment:
Based on the changelog, I don't see anything that should be broken by the version update: done.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r464233574
##########
File path: libminifi/src/core/logging/LoggerConfiguration.cpp
##########
@@ -298,22 +299,23 @@ std::shared_ptr<spdlog::sinks::sink> LoggerConfiguration::create_syslog_sink() {
#ifdef WIN32
return std::make_shared<internal::windowseventlog_sink>("ApacheNiFiMiNiFi");
#else
- return std::make_shared<spdlog::sinks::syslog_sink>("ApacheNiFiMiNiFi");
+ return std::dynamic_pointer_cast<spdlog::sinks::sink>(spdlog::syslog_logger_mt("ApacheNiFiMiNiFi", 0, LOG_USER, false));
Review comment:
I tried doing the suggested change before updating the PR, but unfortunately it does not compile.
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r487110101
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ set(BYPRODUCT "lib/spdlog.lib")
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
+ endif()
+
+ # Set build options
+ set(SPDLOG_SOURCE_DIR "${BINARY_DIR}/thirdparty/spdlog-src")
+ set(SPDLOG_INSTALL_DIR "${BINARY_DIR}/thirdparty/spdlog-install")
+ set(SPDLOG_LIBRARY "${SPDLOG_INSTALL_DIR}/${BYPRODUCT}")
+ set(SPDLOG_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${SPDLOG_INSTALL_DIR}"
+ "-DSPDLOG_BUILD_EXAMPLE=OFF"
+ "-DSPDLOG_BUILD_TESTS=OFF"
+ "-DSPDLOG_BUILD_TESTING=OFF"
+ "-DSPDLOG_BUILD_BENCH=OFF"
+ "-DSPDLOG_BUILD_SHARED=OFF")
+
+ # Build project
+ ExternalProject_Add(
+ spdlog-external
+ URL "https://github.com/gabime/spdlog/archive/v1.7.0.zip"
Review comment:
Would you mind trying to bump this to 1.8.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
[GitHub] [nifi-minifi-cpp] szaszm commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.8.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r488153714
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -0,0 +1,74 @@
+# 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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "lib/spdlogd.lib")
+ else()
+ set(BYPRODUCT "lib/spdlog.lib")
+ endif()
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
Review comment:
Could you convert this to use generator expressions? Consistent usage of them would make multi-config build possible (e.g. switching config in visual studio after generation, before build). We're not there yet on the project level, but this could be a small step in the right direction.
https://cmake.org/cmake/help/v3.17/manual/cmake-generator-expressions.7.html
I know I was the one to commit the wrong version originally, but here's the fix:
```suggestion
set(BYPRODUCT "lib/spdlog$<$<CONFIG:Debug>:d>.lib")
else()
include(GNUInstallDirs)
set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog$<$<CONFIG:Debug>:d>.a")
```
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] adamdebreceni commented on a change in pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.8.0
Posted by GitBox <gi...@apache.org>.
adamdebreceni commented on a change in pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#discussion_r530898014
##########
File path: cmake/BundledSpdlog.cmake
##########
@@ -0,0 +1,74 @@
+# 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.
+
+function(use_bundled_spdlog SOURCE_DIR BINARY_DIR)
+ # Define byproducts
+ if (WIN32)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "lib/spdlogd.lib")
+ else()
+ set(BYPRODUCT "lib/spdlog.lib")
+ endif()
+ else()
+ include(GNUInstallDirs)
+ if ("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlogd.a")
+ else()
+ set(BYPRODUCT "${CMAKE_INSTALL_LIBDIR}/libspdlog.a")
+ endif()
+ endif()
+
+ # Set build options
+ set(SPDLOG_SOURCE_DIR "${BINARY_DIR}/thirdparty/spdlog-src")
+ set(SPDLOG_INSTALL_DIR "${BINARY_DIR}/thirdparty/spdlog-install")
+ set(SPDLOG_LIBRARY "${SPDLOG_INSTALL_DIR}/${BYPRODUCT}")
+ set(SPDLOG_CMAKE_ARGS ${PASSTHROUGH_CMAKE_ARGS}
+ "-DCMAKE_INSTALL_PREFIX=${SPDLOG_INSTALL_DIR}"
+ "-DSPDLOG_BUILD_EXAMPLE=OFF"
+ "-DSPDLOG_BUILD_TESTS=OFF"
+ "-DSPDLOG_BUILD_TESTING=OFF"
+ "-DSPDLOG_BUILD_BENCH=OFF"
+ "-DSPDLOG_BUILD_SHARED=OFF")
+
+ # Build project
+ ExternalProject_Add(
+ spdlog-external
+ URL "https://github.com/gabime/spdlog/archive/v1.8.0.zip"
+ SOURCE_DIR "${SPDLOG_SOURCE_DIR}"
+ CMAKE_ARGS ${SPDLOG_CMAKE_ARGS}
+ BUILD_BYPRODUCTS "${SPDLOG_LIBRARY}"
+ EXCLUDE_FROM_ALL TRUE
+ )
+
+ # Set variables
+ set(SPDLOG_FOUND "YES" CACHE STRING "" FORCE)
+ set(SPDLOG_INCLUDE_DIR "${SPDLOG_INSTALL_DIR}/include" CACHE STRING "" FORCE)
+ set(SPDLOG_LIBRARY "${SPDLOG_INSTALL_DIR}/${BYPRODUCT}" CACHE STRING "" FORCE)
+ set(SPDLOG_LIBRARIES ${SPDLOG_LIBRARY} CACHE STRING "" FORCE)
+
+ # Create imported targets
+ add_library(spdlog STATIC IMPORTED)
+ add_dependencies(spdlog spdlog-external)
+ file(MAKE_DIRECTORY ${SPDLOG_INCLUDE_DIR})
+ set_target_properties(spdlog PROPERTIES
+ IMPORTED_LOCATION "${SPDLOG_LIBRARY}"
+ INTERFACE_INCLUDE_DIRECTORIES "${SPDLOG_INCLUDE_DIR}")
+
+ if (NOT WIN32)
+ set_property(TARGET spdlog APPEND PROPERTY INTERFACE_COMPILE_DEFINITIONS "SPDLOG_ENABLE_SYSLOG")
Review comment:
to prevent increasing the compile time
```suggestion
set_property(TARGET spdlog APPEND PROPERTY INTERFACE_COMPILE_DEFINITIONS "SPDLOG_ENABLE_SYSLOG" "SPDLOG_COMPILED_LIB")
```
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] hunyadi-dev commented on pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.7.0
Posted by GitBox <gi...@apache.org>.
hunyadi-dev commented on pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#issuecomment-672813430
The slow speed on the builds are probably not only due to cache misses. Local builds (on a MacBook Pro 16", 2019) show similar clean build speeds when not using ccache:
```bash
cd .. && rm -rf build && mkdir build && cd build && cmake -G Ninja -DCMAKE_BUILD_TYPE=Debug -DFORCE_COLORED_OUTPUT=ON -DENABLE_LIBRDKAFKA=ON -DENABLE_PYTHON=ON -DENABLE_COAP=ON -DSKIP_TESTS= -DUSE_SHARED_LIBS=ON -DPORTABLE=ON -DBUILD_ROCKSDB=ON -DBUILD_IDENTIFIER= ../src && export CCACHE_DISABLE=1 && time ninja -j4
```
1. **On this branch:**
> ninja -j4 3536.57s user 429.61s system 442% cpu 14:56.78 total
1. **Reference (main):**
> ninja -j4 2823.80s user 439.10s system 457% cpu 11:53.96 total
Please review for potential loss of build speeds. Is it worth the effort of upgrading?
----------------------------------------------------------------
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
[GitHub] [nifi-minifi-cpp] szaszm commented on pull request #837: MINIFICPP-1121 - Upgrade spdlog to version 1.8.0
Posted by GitBox <gi...@apache.org>.
szaszm commented on pull request #837:
URL: https://github.com/apache/nifi-minifi-cpp/pull/837#issuecomment-740638136
There are some compilation errors caused by the last change
----------------------------------------------------------------
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