#28058 closed enhancement (implemented)

Run shellcheck as part of "make check"

Reported by: teor Owned by: rl1987
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: technical-debt
Cc: rl1987 Actual Points:
Parent ID: Points:
Reviewer: mikeperry Sponsor:

Description

To avoid introducing shell script issues into Tor, we could run shellcheck (https://github.com/koalaman/shellcheck) as part of "make check".

Since this is a technical debt check, I am tentatively assigning it to 0.3.6.

Child Tickets

Change History (12)

comment:1 Changed 13 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:2 Changed 12 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:3 Changed 12 months ago by rl1987

Status: acceptedneeds_review

https://github.com/torproject/tor/pull/594

Currently let's only cover scripts/ directory, as all scripts in there has been cleaned up to make shellcheck pass without any warnings. In tor codebase there are still quite a bit of scripts that shellcheck finds objectionable. I will open some tickets later.

comment:4 Changed 12 months ago by nickm

I think this needs to be something that only happens when shellcheck is installed. Unless we really want to require all developers and distributors to install shellcheck?

comment:5 Changed 12 months ago by rl1987

It does check if we have shellcheck before trying to use it:

shellcheck:
	if command -v shellcheck; then \

comment:6 Changed 12 months ago by nickm

Oh. Yeah, you're right. Sorry, I missed that.

comment:7 Changed 12 months ago by dgoulet

Reviewer: mikeperry

comment:8 Changed 11 months ago by mikeperry

Status: needs_reviewmerge_ready

Seems simple enough to me. Looks good.

comment:9 Changed 11 months ago by mikeperry

Status: merge_readyneeds_revision

Actually, you know what.. Can we fix the formatting in that makefile rule, and maybe comment that the command -v thing is checking for shellcheck before running it? The fact that Nick missed that is probably a sign this code is too dense right now.

comment:10 Changed 11 months ago by rl1987

Status: needs_revisionneeds_review

Cleaned up and commented in 49f7951ac9afae1fd67d721b77d4b766b59db3bc.

comment:11 Changed 11 months ago by mikeperry

Status: needs_reviewmerge_ready

Ok this looks good.

comment:12 Changed 11 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Okay; squashed and merged!

Note: See TracTickets for help on using tickets.