MINIFICPP-1121 - Upgrade spdlog to version 1.8.0 #837

hunyadi-dev · 2020-07-14T07:22:27Z

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

libminifi/include/core/PropertyValidation.h

libminifi/src/utils/file/FileUtils.cpp

LICENSE

NOTICE

CMakeLists.txt

libminifi/src/core/logging/LoggerConfiguration.cpp

libminifi/test/TestBase.cpp

LICENSE

libminifi/src/core/logging/LoggerConfiguration.cpp

szaszm · 2020-08-10T13:27:52Z

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.

hunyadi-dev · 2020-08-12T11:23:16Z

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:

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

On this branch:

ninja -j4 3536.57s user 429.61s system 442% cpu 14:56.78 total
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?

hunyadi-dev · 2020-11-23T17:33:06Z

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.

szaszm

I think this is ready for merge but let's wait for one approval besides mine because of the latest changes. @fgerlits @arpadboda

fgerlits

Minor, but the PR comment still says this is a draft PR.

fgerlits · 2020-11-24T09:36:38Z

docker/debian/Dockerfile

+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\


Why do we need the system libarchive? We bundle that.

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

fgerlits · 2020-11-24T09:49:51Z

cmake/BundledSpdlog.cmake

+        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()


What happened here? I think I remember a commit which changed these to generator expressions, but now the generator expressions are not there.

https://gitlab.kitware.com/cmake/cmake/-/issues/19522

cmake/BundledSpdlog.cmake

szaszm · 2020-12-08T14:03:45Z

There are some compilation errors caused by the last change

hunyadi-dev · 2020-12-09T08:18:34Z

@szaszm rebased and updated again...

szaszm · 2020-12-09T11:53:50Z

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.

…eplace incompatible interface Dependency directory is yet to be renamed in this commit.

…pgrade its license

…c_string_view<char>

…uto, change return types in LoggerConfiguration sink creation

chmod a+x debian using stretch-backports

…g compile times Co-authored-by: adamdebreceni <64783590+adamdebreceni@users.noreply.github.com>

…Utils

hunyadi-dev force-pushed the MINIFICPP-1121_spdlog_upgrade branch 3 times, most recently from d89f489 to 4e6909c Compare July 16, 2020 07:25

hunyadi-dev marked this pull request as ready for review July 16, 2020 07:56

fgerlits reviewed Jul 16, 2020

View reviewed changes

libminifi/include/core/PropertyValidation.h Outdated Show resolved Hide resolved

fgerlits reviewed Jul 16, 2020

View reviewed changes

libminifi/src/utils/file/FileUtils.cpp Outdated Show resolved Hide resolved

hunyadi-dev force-pushed the MINIFICPP-1121_spdlog_upgrade branch 2 times, most recently from b467ba8 to 8116851 Compare July 16, 2020 09:04

szaszm reviewed Jul 16, 2020

View reviewed changes

LICENSE Show resolved Hide resolved

szaszm reviewed Jul 17, 2020

View reviewed changes

NOTICE Outdated Show resolved Hide resolved

hunyadi-dev force-pushed the MINIFICPP-1121_spdlog_upgrade branch 5 times, most recently from 1d5ab27 to aa02d18 Compare July 20, 2020 11:15

fgerlits reviewed Jul 22, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

fgerlits mentioned this pull request Jul 23, 2020

MINIFICPP-1268 Fix compiler warnings on Windows (VS2017) #848

Closed

8 tasks

hunyadi-dev force-pushed the MINIFICPP-1121_spdlog_upgrade branch from aa02d18 to 1003bc5 Compare July 28, 2020 08:34

fgerlits approved these changes Jul 29, 2020

View reviewed changes

szaszm reviewed Jul 30, 2020

View reviewed changes

libminifi/src/core/logging/LoggerConfiguration.cpp Show resolved Hide resolved

libminifi/test/TestBase.cpp Outdated Show resolved Hide resolved

LICENSE Show resolved Hide resolved

szaszm reviewed Jul 31, 2020

View reviewed changes

libminifi/src/core/logging/LoggerConfiguration.cpp Outdated Show resolved Hide resolved

hunyadi-dev force-pushed the MINIFICPP-1121_spdlog_upgrade branch 3 times, most recently from 58ab0ea to 88f4425 Compare August 4, 2020 12:30

szaszm approved these changes Aug 5, 2020

View reviewed changes

hunyadi-dev force-pushed the MINIFICPP-1121_spdlog_upgrade branch 2 times, most recently from 4eb3541 to 2fe7f0a Compare August 12, 2020 09:25

hunyadi-dev marked this pull request as draft September 8, 2020 10:01

szaszm approved these changes Nov 23, 2020

View reviewed changes

fgerlits reviewed Nov 24, 2020

View reviewed changes

fgerlits approved these changes Nov 24, 2020

View reviewed changes

adamdebreceni requested changes Nov 26, 2020

View reviewed changes

cmake/BundledSpdlog.cmake Outdated Show resolved Hide resolved

hunyadi-dev force-pushed the MINIFICPP-1121_spdlog_upgrade branch from c223df9 to ec40383 Compare December 9, 2020 08:18

szaszm force-pushed the MINIFICPP-1121_spdlog_upgrade branch from f89a896 to 0dd5c9f Compare December 10, 2020 11:50

hunyadi-dev force-pushed the MINIFICPP-1121_spdlog_upgrade branch from 0dd5c9f to d2b1388 Compare December 10, 2020 12:32

hunyadi-dev and others added 15 commits December 10, 2020 14:13

MINIFICPP-1121 - Switch speedlog implementaion to version 1.7.0 and r…

e3a6967

…eplace incompatible interface Dependency directory is yet to be renamed in this commit.

MINIFICPP-1121 - Add changes for windows support for spdlog upgrade

99498c7

MINIFICPP-1121 - Rename spdlog to correspond to its new version and u…

229f409

…pgrade its license

MINIFICPP-1121 - Remove leftover Windows minmax macro workarounds

c0bbb81

MINIFICPP-1121 - Add further notices on spdlog and fmt licenses

a538a73

MINIFICPP-1121 - Correct signature for the %s printouts for fmt::basi…

bc48e8b

…c_string_view<char>

MINIFICPP-1121 - Move -DNOMINMAX up in the CXX and C flags list

7f4d85c

MINIFICPP-1121 - Change fmt::string_view returned from spdlog to be a…

d5f8861

…uto, change return types in LoggerConfiguration sink creation

MINIFICPP-1121 - Convert spdlog to static lib

3a209bf

MINIFICPP-1121 - Correct shared library paths for bundled spdlog

ce2fa14

MINIFICPP-1121 - Upgrade spdlog version (1.7.0 -> 1.8.0)

ebe9df9

fix cat debian.sh >debian

ac08f9f

chmod a+x debian using stretch-backports

MINIFICPP-1121 - Prevent multiple inclusions of spdlib from increasin…

18f185a

…g compile times Co-authored-by: adamdebreceni <64783590+adamdebreceni@users.noreply.github.com>

MINIFICPP-1121 - Include missing headers previously included by spdlog

54eb9c0

MINIFICPP-1121 - Include sys/stat.h when using boost on linux in File…

71e3ce1

…Utils

hunyadi-dev force-pushed the MINIFICPP-1121_spdlog_upgrade branch from d2b1388 to 71e3ce1 Compare December 10, 2020 13:15

adamdebreceni approved these changes Dec 10, 2020

View reviewed changes

szaszm closed this in 8f7a946 Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINIFICPP-1121 - Upgrade spdlog to version 1.8.0 #837

MINIFICPP-1121 - Upgrade spdlog to version 1.8.0 #837

hunyadi-dev commented Jul 14, 2020

szaszm commented Aug 10, 2020

hunyadi-dev commented Aug 12, 2020

hunyadi-dev commented Nov 23, 2020

szaszm left a comment

fgerlits left a comment

fgerlits Nov 24, 2020

szaszm Nov 24, 2020

fgerlits Nov 24, 2020

szaszm Nov 24, 2020

szaszm commented Dec 8, 2020

hunyadi-dev commented Dec 9, 2020

szaszm commented Dec 9, 2020

MINIFICPP-1121 - Upgrade spdlog to version 1.8.0 #837

MINIFICPP-1121 - Upgrade spdlog to version 1.8.0 #837

Conversation

hunyadi-dev commented Jul 14, 2020

For all changes:

For code changes:

For documentation related changes:

Note:

szaszm commented Aug 10, 2020

hunyadi-dev commented Aug 12, 2020

hunyadi-dev commented Nov 23, 2020

szaszm left a comment

Choose a reason for hiding this comment

fgerlits left a comment

Choose a reason for hiding this comment

fgerlits Nov 24, 2020

Choose a reason for hiding this comment

szaszm Nov 24, 2020

Choose a reason for hiding this comment

fgerlits Nov 24, 2020

Choose a reason for hiding this comment

szaszm Nov 24, 2020

Choose a reason for hiding this comment

szaszm commented Dec 8, 2020

hunyadi-dev commented Dec 9, 2020

szaszm commented Dec 9, 2020