Page MenuHomePhabricator
Create Task
Maniphest T8404

Running the cpplint linter through arcanist no longer works
Open, Needs TriagePublic
Actions

Description

Our project had the cpplint linter configured and it was working really well. Then I updated arcanist a few days ago and things broke stopped working. It says the linter runs "Successfully", but it actually doesn't run at all. It seems like no files are actually being passed into cpplint so it just returns instantly.

If I revert the commit 4330b27 (https://secure.phabricator.com/D12696) by , the linter starts to work again.

Setup
arc version
arcanist 3ac80200e26115c9e390d84211bd1f0a24d49dce (27 May 2015)
libphutil c2cd90ee7aec642dcdf72de6119a9cac6e735cff (26 May 2015)

I was linting a C project. I changed a file and adding linting errors. With 4330b27, it was returned "No linting errors", with it reverted, it would return the actual linting errors. This is 100% reproducible.

Revisions and Commits

rARC Arcanist
D14960rARCaeb374b33348 Fix cpplint message regex

Related Objects

Event Timeline

tyhoff created this task.Jun 3 2015, 10:03 PM
tyhoff raised the priority of this task from to Needs Triage.
tyhoff updated the task description. (Show Details)
tyhoff added a project: Arcanist.
tyhoff added a subscriber: tyhoff.
tyhoff updated the task description. (Show Details)Jun 3 2015, 10:14 PM
avivey added a subscriber: avivey.Jun 3 2015, 10:26 PM

I'm guessing this is an instance of T8333: linters with deprecated configurations give hard-to-debug errors.

If the linter is configured in .arcconfig, it will no longer work properly. You should move the config to .arclint.

avivey added a comment.EditedJun 3 2015, 10:27 PM

... unless you actually triaged this to D12696, in which case it's something else.

avivey added a comment.Jun 3 2015, 10:28 PM

Can you paste here the relevant parts of .arcconfig and .arclint?

tyhoff added a comment.Jun 3 2015, 10:32 PM

I didn't necessarily "triage", but I did a git bisect over all recent commits and this is where things stopped working for me.

I have the linter configured in .arclint

"cpplint": {
  "type": "cpplint",
  "bin": ["tools/linting/linters/cpplint/cpplint.py"],
  "include": [ "(\\.(c|h)$)" ],
  "severity": {
    "runtime/int":            "advice",
    "whitespace/comments":    "advice",
    "whitespace/blank_line":  "advice"
  }
},

.arcconfig

{
  "load" : [
    "tools/linting/linters"
  ]
}
tyhoff added a comment.Jun 3 2015, 10:40 PM

I have reverted the entire commit from master and the linter works correctly.
I have also tested before and after 4330b27. Before the commit, things still work. After the commit, it stops working.

So I really do think it is this commit that is causing it not to work.

avivey added a comment.Jun 3 2015, 10:45 PM

I took your .arclint, changed the bin to point to one I have, and I see that it does try to run it, but the linter ignores it:

$ arc lint --trace *.c
....
>>> [4] <exec> $ '/home/aviv/tmp/cpplint.py'  '/home/aviv/tmp/listenurl/listenurl.c'
$ /home/aviv/tmp/cpplint.py /home/aviv/tmp/listenurl/listenurl.c
Ignoring /home/aviv/tmp/listenurl/listenurl.c; not a valid file name (cc, h, cpp, cu, cuh)
Done processing /home/aviv/tmp/listenurl/listenurl.c
Total errors found: 0

This is on the most recent arc and cpplint.

Possibly, before the change, the files were piped in, so cpplint couldn't ignore them by file name.

trying to lint .h files blows up because cpplint exits with non-0 because "No copyright message".

A workaround would be to define a wrapper that takes a any file and pipes it into cpplint.

tyhoff added a comment.Jun 3 2015, 10:56 PM

what happens if you change cpplint to accept the C extension? You can just edit the python script and the var _valid_extensions.

I get

[2015-06-03 22:54:00] EXCEPTION: (PhutilAggregateException) Some linters failed:
    - CommandException: Command failed with error #1!
      COMMAND
      'tools/linting/linters/cpplint/cpplint.py'  '/Users/thoffman/dev/tintin/src/fw/services/normal/test_file.c'

      STDOUT
      (empty)

      STDERR
      /Users/thoffman/dev/tintin/src/fw/services/normal/test_file.c:44:  Missing spaces around =  [whitespace/operators] [4]
      Done processing /Users/thoffman/dev/tintin/src/fw/services/normal/test_file.c
      Total errors found: 1
       at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:275]
arcanist(head=master, ref.master=8c589f1f759f), linters(), phutil(head=master, ref.master=afc05a9a7f00)
  #0 <#2> ExecFuture::resolvex() called at [<arcanist>/src/lint/linter/ArcanistExternalLinter.php:366]
  #1 <#2> ArcanistExternalLinter::resolveFuture(string, ExecFuture) called at [<arcanist>/src/lint/linter/ArcanistFutureLinter.php:34]
  #2 <#2> ArcanistFutureLinter::didLintPaths(array) called at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:602]
  #3 <#2> ArcanistLintEngine::executeDidLintOnPaths(ArcanistCpplintLinter, array) called at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:553]
  #4 <#2> ArcanistLintEngine::executeLintersOnChunk(array, array) called at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:481]
  #5 <#2> ArcanistLintEngine::executeLinters(array) called at [<arcanist>/src/lint/engine/ArcanistLintEngine.php:219]
  #6 ArcanistLintEngine::run() called at [<arcanist>/src/workflow/ArcanistLintWorkflow.php:331]
  #7 ArcanistLintWorkflow::run() called at [<arcanist>/scripts/arcanist.php:382]

Is arcanist not expecting cpplint to return errors on STDERR?

avivey added a comment.Jun 3 2015, 11:00 PM

It doesn't expect it to give a non-zero exit code, but it should (https://secure.phabricator.com/diffusion/ARC/browse/master/src/lint/linter/ArcanistExternalLinter.php;8c589f1f759f0913135b8cc6959a6c1589e14ae4$58-60).

joshuaspence added a project: Lint.Jun 3 2015, 11:29 PM
joshuaspence added a subscriber: joshuaspence.
tyhoff added a comment.Jun 18 2015, 7:04 PM

Also looks like the output format for cpplint has changed due to 4330b27

Before:

-:163:  Missing space before {  [whitespace/braces] [5]

After:

/src/fw/process_management/app_install_manager.c:163:  Missing space before {  [whitespace/braces] [5]

Changing the regular expression for cpplint to the following fixes my issues with cpplint linting.

$regex = '/(\d+):\s*(.*)\s*\[(.*)\] \[(\d+)\]$/';

In conclusion, my fix was:

  1. Update regular expression in ArcanistCpplintLinter.php with the one listed above.
  2. I also had to force cpplint.py to return 0 every time.
  3. I had to include 'c' in the list of accepted files of cpplint.py.
epriestley merged a task: T8713: cpplint no longer works.Jun 29 2015, 10:26 PM
epriestley added a subscriber: • sshannin.
jra3 added a subscriber: jra3.Sep 13 2015, 5:09 AM

Actually, all I needed to do to fix this was to use @tyhoff's regex. The issue has nothing to do with the non-zero return code. It has to do with not being able to parse any messages from the output.

https://secure.phabricator.com/diffusion/ARC/browse/master/src/lint/linter/ArcanistExternalLinter.php;8c589f1f759f0913135b8cc6959a6c1589e14ae4$357

andl added a subscriber: andl.Nov 25 2015, 3:10 PM

lastest cpplint + arcanist HEAD (9e78d15) still have this issue, using @tyhoff's regex fix the issue.

michaeloa added a subscriber: michaeloa.Dec 11 2015, 1:40 PM
michaeloa added a comment.Dec 14 2015, 12:48 PM

We've added a linter with this fix to our library of arcanist "extras" (https://github.com/metno/arcanist-support). Also updated the install information for cpplint, as discussed in T9957.

If we can help expedite a fix for this by submitting a patch, let us know.

epriestley moved this task from Backlog to Bindings on the Lint board.Dec 22 2015, 12:46 PM
epriestley mentioned this in T10038: Plan the mid-term pathway for unit-test/linter bindings.Dec 22 2015, 12:59 PM
jra3 added a revision: D14960: Fix cpplint message regex.Jan 6 2016, 5:59 PM
epriestley added a commit: rARCaeb374b33348: Fix cpplint message regex.Jan 6 2016, 9:15 PM
avivey mentioned this in T10118: regex for parsing severity codes in ArcanistCppLinter is wrong.Jan 11 2016, 8:58 PM
epriestley moved this task from Backlog to Unit/Lint Bindings on the Arcanist board.Nov 14 2016, 5:58 PM
Phabricator · Built by Phacility