Alexey Serbin has posted comments on this change. Change subject: [build-support] IWYU build configuration for Jenkins ......................................................................
Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/7750/3/build-support/iwyu/iwyu_tool.py File build-support/iwyu/iwyu_tool.py: > So this is totally unmodified, then? Right -- I only added the license into the header. http://gerrit.cloudera.org:8080/#/c/7750/3/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: Line 197: elif [ "$BUILD_TYPE" = "IWYU" ]; then > Shouldn't this set USE_CLANG=1? Or does that not matter? I think it makes sense. For some reason I always do that for my builds, so I forgot to do that here. Thanks! PS3, Line 242: # Create empty test logs or else Jenkins fails to archive artifacts, which : # results in the build failing. : mkdir -p Testing/Temporary : mkdir -p $TEST_LOGDIR > We have a number of these strewn around the script; can we just do this onc It's a good idea, specially given the fact it's all idempotent; I'll update it as needed. Line 248: file_list_tmp=$(git show --format='' --name-only | grep -E '\.(c|cc|h)$') > Maybe this should follow the style of 'make ilint', which looks for the las Done PS3, Line 256: if [ -z "$IWYU_FILE_LIST" ]; then : # Nothing to do: declare success. : exit 0 : fi > Maybe do this on file_list_tmp to short circuit faster? Yep, that's a way to go as well. Line 267: --mapping_file=$IWYU_ARGS/gtest.imp \ > Nit: after glog Done Line 274: IWYU_FILTERED_LOG=$TEST_LOGDIR/iwyu-filtered.log > I'd prefer either: This sounds reasonable. I'll update it, implementing the latter. Line 275: awk -f ../../build-support/iwyu-filter.awk $IWYU_LOG | tee $IWYU_FILTERED_LOG > Just curious - is this post-processing step with awk a common thing to do w This is a workaround specific for Kudu. It's here becase: 1) It seemed not feasible to address everything in the auto-generated files. Yes, they are not to appear in the list of files fed to this run, but there is also cmake-related use case for IWYU. 2) There might be some imported files which we would not like to update in regard of 'IWYU accuracy'. 3) Right now there is a small list of files in the 'mute' list because it requires some additional work on IWYU-related quirks. I don't want to continue spending time on that now, hoping I can empty it eventually (working in the background). 4) The filtering might be the last resort in some scenarios when there isn't fast way to fix another IWYU quirk we stumble upon. -- To view, visit http://gerrit.cloudera.org:8080/7750 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I45f2e8529891dd7da18c9fa6383e0c41e6b4182e Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
