martinzink commented on a change in pull request #1069: URL: https://github.com/apache/nifi-minifi-cpp/pull/1069#discussion_r629293827
########## File path: CMakeLists.txt ########## @@ -847,21 +847,17 @@ else() include(DockerConfig) endif() -if(NOT WIN32) # Create a custom build target that will run the linter. get_property(extensions GLOBAL PROPERTY EXTENSION-LINTERS) add_custom_target(linter - COMMAND ${CMAKE_SOURCE_DIR}/thirdparty/google-styleguide/run_linter.sh - ${CMAKE_SOURCE_DIR}/libminifi/include/ -- - ${CMAKE_SOURCE_DIR}/libminifi/src/ - COMMAND ${CMAKE_SOURCE_DIR}/thirdparty/google-styleguide/run_linter.sh - ${CMAKE_SOURCE_DIR}/libminifi/include/ -- - ${CMAKE_SOURCE_DIR}/libminifi/test/ - COMMAND ${CMAKE_SOURCE_DIR}/thirdparty/google-styleguide/run_linter.sh - ${CMAKE_SOURCE_DIR}/encrypt-config/ -- - ${CMAKE_SOURCE_DIR}/encrypt-config/ + COMMAND python ${CMAKE_SOURCE_DIR}/thirdparty/google-styleguide/run_linter.py -i Review comment: Since the run_linter.py only checked the cpp files in the first argument and only the h files in the second, we didnt check any h files in the libminifi/test folder. It also needlesly double checked the libminifi/include folder. The cpplint.py doesnt require the filelist to be separated by header/source, so its easier to just have one argument. ########## File path: .github/workflows/ci.yml ########## @@ -24,15 +24,20 @@ jobs: brew update brew install ossp-uuid boost flex lua@5.3 ccache sqliteodbc automake autoconf - id: setup_env + name: setup enviroment run: | echo "PATH=/usr/lib/ccache:/usr/local/opt/ccache/bin:/usr/local/opt/ccache/libexec:$PATH" >> $GITHUB_ENV echo -e "127.0.0.1\t$HOSTNAME" | sudo tee -a /etc/hosts > /dev/null sudo xcode-select -switch /Applications/Xcode_11.2.1.app - - id: build + - name: build run: | export PATH="/usr/local/opt/lua@5.3/lib:/usr/local/opt/lua@5.3/include:/usr/local/opt/lua@5.3/bin:$PATH" export PKG_CONFIG_PATH="/usr/local/opt/lua@5.3/lib/pkgconfig" - ./bootstrap.sh -e -t && cd build && cmake -DCMAKE_BUILD_TYPE=Release -DENABLE_LUA_SCRIPTING=1 -DENABLE_AWS=ON -DENABLE_AZURE=ON -DENABLE_SQL=ON -DCMAKE_VERBOSE_MAKEFILE=ON -DCMAKE_RULE_MESSAGES=OFF -DSTRICT_GSL_CHECKS=AUDIT -DFAIL_ON_WARNINGS=ON .. && cmake --build . --parallel 4 && make test ARGS="--timeout 300 -j4 --output-on-failure" && make linter + ./bootstrap.sh -e -t && cd build && cmake -DCMAKE_BUILD_TYPE=Release -DENABLE_LUA_SCRIPTING=1 -DENABLE_AWS=ON -DENABLE_AZURE=ON -DENABLE_SQL=ON -DCMAKE_VERBOSE_MAKEFILE=ON -DCMAKE_RULE_MESSAGES=OFF -DSTRICT_GSL_CHECKS=AUDIT -DFAIL_ON_WARNINGS=ON .. && cmake --build . --parallel 4 + - name: test Review comment: separating the build/test/linter checks into steps, and naming them accordingly makes the github ci output more readable ########## File path: libminifi/test/KamikazeProcessor.h ########## @@ -18,13 +18,15 @@ * limitations under the License. */ +#include <string> Review comment: Linter issues previously libminifi/test/*.h files were ignored by the linter. -- 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: us...@infra.apache.org