Page MenuHomePhabricator

build: Add format string linter
ClosedPublic

Authored by Fabien on Nov 7 2018, 21:31.

Details

Summary

This linter checks that the number of arguments passed to each variadic format
string function matches the number of format specifiers in the format string.

Backport of core PR13705

Test Plan
./test/lint/lint-format-strings.sh

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D2025
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4348
Build 6761: Bitcoin ABC Buildbot (legacy)
Build 6760: arc lint + arc unit

Event Timeline

schancel requested changes to this revision.Nov 7 2018, 22:30
schancel added a subscriber: schancel.
➜  bitcoin-abc git:(arcpatch-D2025) ✗ ./test/lint/lint-format-strings.sh
./test/lint/lint-format-strings.sh: line 35: mapfile: command not found
./test/lint/lint-format-strings.sh: line 35: mapfile: command not found
./test/lint/lint-format-strings.sh: line 35: mapfile: command not found
./test/lint/lint-format-strings.sh: line 35: mapfile: command not found
./test/lint/lint-format-strings.sh: line 35: mapfile: command not found
./test/lint/lint-format-strings.sh: line 35: mapfile: command not found
./test/lint/lint-format-strings.sh: line 35: mapfile: command not found
./test/lint/lint-format-strings.sh: line 35: mapfile: command not found
./test/lint/lint-format-strings.sh: line 35: mapfile: command not found
./test/lint/lint-format-strings.sh: line 35: mapfile: command not found
./test/lint/lint-format-strings.sh: line 35: mapfile: command not found
./test/lint/lint-format-strings.sh: line 35: mapfile: command not found
./test/lint/lint-format-strings.sh: line 35: mapfile: command not found
test/lint/lint-format-strings.sh
1 ↗(On Diff #5707)

#!/bin/bash

This revision now requires changes to proceed.Nov 7 2018, 22:30

I follows that arc lint should run this automatically.

Fabien marked an inline comment as done.Nov 8 2018, 08:40

Running it through arc lint is a good idea. I propose to do this in another diff to keep the backport clean

test/lint/lint-format-strings.sh
1 ↗(On Diff #5707)

What is the reason for this change ? Is env causing an issue on Mac ?

@schancel Is readarray available on your machine ?

Fabien marked an inline comment as done.Nov 9 2018, 13:41
Fabien added inline comments.
test/lint/lint-format-strings.sh
1 ↗(On Diff #5707)

I read the article, and I think using env is the best solution for this specific script.
The solutions this article describe are better, but suitable to scripts that are intended to be installed on user's machine, which is not the case for this one. Using configure to detect the user configuration and change the shebang seems overkill to me for an helper script.
Using env for searching the bin in PATH has less drawbacks that using the #!/bin/bash hardcoded one.

test/lint/lint-format-strings.sh
35 ↗(On Diff #5707)

What does this do? And why is this script-to-run-a-script necessary? I would prefer not to have shell running python if possible.

It errors out on FreeBSD variants as mapfile isn't included.

Fabien marked an inline comment as done.Nov 10 2018, 11:40
Fabien added inline comments.
test/lint/lint-format-strings.sh
35 ↗(On Diff #5707)

It builds an array from the input. mapfile (or readarray, this is equivalent) is part of bash, but is also built in sh, what shell are you using ?

I agree that this shell script should be avoided, the idea is to end up calling the python script from arc lint. But I don't know how to do this (yet), and I prefer include it in a future diff to keep the backport consistent.

Fabien planned changes to this revision.Dec 21 2018, 16:18

Integrate the linter with arcanist

Fabien edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Dec 21 2018, 19:03

It's be good if we could run the linter on specified files rather than on everything.

.arclint
23 ↗(On Diff #6396)

Why ?

test/lint/lint-format-strings.sh
40 ↗(On Diff #6396)

???

This revision now requires changes to proceed.Dec 21 2018, 19:03

So I arch patched this and this is CLEARLY not working.

$ time ./test/lint/lint-format-strings.sh
[...]
real    0m1.811s
user    0m1.928s
sys     0m0.616s

$ time arc lint
 OKAY  No lint warnings.

real    0m0.544s
user    0m0.448s
sys     0m0.094s

The linter actually runs on a single file if called via the bash script ./test/lint/lint-format-strings.sh file_to_check.
This is required for arcanist integration. When it is called via arc lint every file from the diff is processed individually (you can run arc lint --trace to see the behavior).
This is different from the core version which checked all the files.

.arclint
23 ↗(On Diff #6396)

I thought that errors from libraries should not be checked as it may not be code we want to maintain. These will also require to manually add exceptions in the python script, and re-check every time the library gets updated. The clang-format linter also excludes these files.
Do you think they should be checked ?

Fix exit in bash script

test/lint/lint-format-strings.sh
39 ↗(On Diff #6400)

Why the change ? If the script failed to run, it should not pretend it's all good.

Split in two diffs. This one is the backport of PR13705.
The arcanist integration will be made in another diff.

Fabien edited the test plan for this revision. (Show Details)
This revision is now accepted and ready to land.Dec 28 2018, 09:32
This revision was automatically updated to reflect the committed changes.