Details

Reviewers

jasonbcox
deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGINGf867af275c65: build: Add format string linter
rABCf867af275c65: build: Add format string linter

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

PR13705

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 3812
Build 5697: Bitcoin ABC Buildbot (legacy)
Build 5696: arc lint + arc unit

Event Timeline

Fabien created this revision.Nov 7 2018, 21:31

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 7 2018, 21:31

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3812: Diff 5707.Nov 7 2018, 21:41

Fabien added a child revision: D2026: Add a lint-format-strings false positive filter for seeder help.Nov 7 2018, 22:03

➜  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	#!/bin/bash

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

I follows that arc lint should run this automatically.

Fabien edited the summary of this revision. (Show Details)Nov 8 2018, 08:30

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	What is the reason for this change ? Is env causing an issue on Mac ?

@schancel Is readarray available on your machine ?

schancel added inline comments.Nov 8 2018, 22:06

test/lint/lint-format-strings.sh
1	http://julio.meroh.net/2016/09/env-considered-harmful.html

Fabien marked an inline comment as done.Nov 9 2018, 13:41

Fabien added inline comments.

test/lint/lint-format-strings.sh
1	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.

schancel added inline comments.Nov 9 2018, 23:11

test/lint/lint-format-strings.sh
35	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	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 summary of this revision. (Show Details)Dec 21 2018, 17:59

Fabien edited the test plan for this revision. (Show Details)

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B4346: Diff 6396!Dec 21 2018, 18:05

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 ?