clang-tidy support followup (actually error travis job with warnings/errors) #105

Activity

1. springmeyer commented on Jan 27, 2018

   

   springmeyer

   on Jan 27, 2018

   ContributorAuthor

   More actions

   /cc @daniel-j-h @kkaefer in case either of you have ideas.
2. sssoleileraaa commented on Jun 13, 2018

   

   sssoleileraaa

   on Jun 13, 2018

   More actions

   @springmeyer - I believe this is the bug you are referring to. You can see the last comment from a couple months ago says "Should be fixed in r329579" where r329579 refers to a code revision which you can see here. The fix is to have LLVM return a non-zero exit code when it sees a compiler error.

   Since this fix is not part of LLVM 6.0.0 we are still coming across this issue. We can test the fix by pointing node-cpp-skel with a dev version of LLVM and its latest changes.
3. sssoleileraaa commented on Jun 13, 2018

   

   sssoleileraaa

   on Jun 13, 2018

   Last edited by sssoleileraaa

   More actions

   After taking a closer look at the LLVM code change, I see that our issue will not be fixed because we use the -fix-errors flag. And you can see here that if FixErrors is set, clang-tidy will return zero. You can also take a look at that TODO (my vote is for b).

     if (FoundErrors) {
       // TODO: Figure out when zero exit code should be used with -fix-errors:
       //   a. when a fix has been applied for an error
       //   b. when a fix has been applied for all errors
       //   c. some other condition.
       // For now always returning zero when -fix-errors is used.
       if (FixErrors)
         return 0;
       if (!Quiet)
         llvm::errs() << "Found compiler error(s).\n";
       return 1;
     }

   Here's a solution proposal while we wait to see if LLVM will change clang-tidy so that a zero return code is only returned if all errors have been fixed:

   We can pipe output from cmake to a file and then scan it for strings indicating an error.
4. sssoleileraaa commented on Jun 13, 2018

   

   sssoleileraaa

   on Jun 13, 2018

   More actions

   Looks like we actually pass -fix not -fix-errors: https://github.com/mapbox/node-cpp-skel/blob/master/scripts/clang-tidy.sh#L47 so perhaps the issue is fixed in llvm-dev. We can test that pretty easily but I want to double-check my understanding of the problem first. I think the issue isn't that there should be a diff when this is run in Travis but that we should have a way of detecting and notifying clang-tidy errors that were not automatically fixed. Does this sound correct?

   http://clang.llvm.org/extra/clang-tidy/ docs:

     -fix                          -
                                     Apply suggested fixes. Without -fix-errors
                                     clang-tidy will bail out if any compilation
                                     errors were found.
     -fix-errors                   -
                                     Apply suggested fixes even if compilation
                                     errors were found. If compiler errors have
                                     attached fix-its, clang-tidy will apply them as
                                     well.
   
5. springmeyer commented on Jun 13, 2018

   

   springmeyer

   on Jun 13, 2018

   ContributorAuthor

   More actions

   @AllieOop wow, incredible research and digging. Your findings sounds really promising. Overall I think what we ideally want is:

   • clang-tidy to return non-zero if there are any errors (either fixable or not)
   • we want to capture the clang-tidy return code and then dump some helpful output (including any diffs) before exiting the overall clang-tidy.sh script with that same error code that run-clang-tidy.py
   • this way we'll ensure that if any clang tidy warnings or errors (even those automatically fixable) are hit then travis will fail. Then the developer will know that they need to run make tidy locally and commit any changes needed to fix things.

   Note: the fact that the current script uses the presence of a diff to decide on return code I consider a workaround that we do not need to keep (Although spitting out a diff still feels valuable).

   So it seems like next steps are:

   • figure out what version of clang-tidy/llvm the fix you found landed in
   • figure out if we can upgrade to that version (is it released yet?)
   • start capturing the return code of the run-clang-tidy.py command here using the same bash approach that we do to capture and handle the return code from make here.