Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

shellcheck: Add CI check script #9063

Merged
merged 1 commit into from
Jul 30, 2018
Merged

shellcheck: Add CI check script #9063

merged 1 commit into from
Jul 30, 2018

Conversation

bergzand
Copy link
Member

@bergzand bergzand commented May 3, 2018

Contribution description

Check script for shellcheck in a similar way as the flake8 script. SC1090 is excluded from the checks.

Issues/PRs references

#8212

@bergzand bergzand added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: tools Area: Supplementary tools labels May 3, 2018
@aabadie aabadie added the Area: CI Area: Continuous Integration of RIOT components label May 3, 2018
aabadie
aabadie previously requested changes May 3, 2018
View reviewed changes
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very useful, thanks!

Do you think it would make sense to add this script to the list of static tests (in dist/tools/ci/build_and_test.sh) ?
I tested it locally and since another script with issues is touched, shellcheck reports errors.
You may also want to install shellcheck riotbuild docker image and travis.yml

dist/tools/shellcheck/check.sh Outdated
fi

${SHELLCHECK_CMD} --version &> /dev/null || {
printf "%s%s: cannot execute \"%s\"!%s\n" "${CERROR}" "$0" "${SHELLCHECK_CMD}" "${CRESET}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, $0 and ${SHELLCHECK_CMD} are the same thing ? In flake checker, $0 corresponds to python because the command is python -m flake8 ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use the following:

printf "%sError: \"%s\" is missing%s\n" "${CERROR}" "${SHELLCHECK_CMD}" "${CRESET}"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

dist/tools/shellcheck/check.sh Outdated
exit 1
}

ERRORS=$("${SHELLCHECK_CMD}" --format=gcc ${FILES})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is reporting the following error for me: dist/tools/shellcheck/check.sh:38:43: note: Double quote to prevent globbing and word splitting. [SC2086]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way at the moment to fix this is to make FILES an bash array, which requires modifications of the dist/tools/ci/changed_files.sh. Those modification would cascade to many other scripts which is out of scope for this PR.

@cladmi
Copy link
Contributor

cladmi commented May 4, 2018 •
edited
Loading

Now that the 'RIOTTOLS' variable has been added, could you try replacing 'dist/tools' by 'RIOTTOLS' as done in some of the commits here: https://github.com/RIOT-OS/RIOT/pull/8821 ?

Edit: or more precisely this one now: #9070

@bergzand bergzand force-pushed the pr/shellcheck/initial branch from 6ae813a to b3d8970 Compare May 24, 2018 11:01
@bergzand
Copy link
Member Author

Now that the 'RIOTTOLS' variable has been added, could you try replacing 'dist/tools' by 'RIOTTOLS' as done in some of the commits here: #8821 ?

Done!

@bergzand bergzand force-pushed the pr/shellcheck/initial branch from b3d8970 to b5cd1f6 Compare May 24, 2018 11:04
@tcschmidt tcschmidt requested a review from smlng June 21, 2018 11:24
smlng
smlng suggested changes Jun 22, 2018
View reviewed changes
dist/tools/shellcheck/check.sh Outdated
fi

${SHELLCHECK_CMD} --version &> /dev/null || {
printf "%sError: \"%s\" is missing%s\n" "${CERROR}" "${SHELLCHECK_CMD}" "${CRESET}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't work when spellcheck is not installed this error message reads: Error: "" is missing, which is no surprise. Hence, you need to write shellcheck in the error string because the SHELLCHECK_CMD variable is empty

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, fixed!

dist/tools/shellcheck/check.sh Outdated
: "${RIOTTOOLS:=${PWD}/dist/tools}"
. "${RIOTTOOLS}"/ci/changed_files.sh

FILES=$(FILEREGEX='(?=*.sh$)' changed_files)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this regex doesn't work with macOS, and also is no proper/standard? regex anyway. Using FILES=$(FILEREGEX='(.*\.sh$)' changed_files) it works, allowing any file name which ends on .sh.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for testing this, I've replaced it with your suggestion.

@bergzand
Copy link
Member Author

@smlng thanks for the review, I've adapted your suggestions

@cladmi
Copy link
Contributor

cladmi commented Jul 17, 2018

@aabadie @smlng do you like the changes can he squash ?

After discussing IRL about the changes, I personally just want referenced in the commit message why it is not passing its own test right now #9063 (comment)

I would merge after this.

@cladmi cladmi added this to the Release 2018.10 milestone Jul 17, 2018
@smlng
Copy link
Member

smlng commented Jul 18, 2018

re-tested on macOS, works as expected: ACK.

@smlng
Copy link
Member

smlng commented Jul 18, 2018

clear to squash

miri64
miri64 reviewed Jul 19, 2018
View reviewed changes
dist/tools/shellcheck/check.sh Outdated
# directory for more details.
#

SHELLCHECK_CMD="$(which shellcheck)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the agreed upon and portable way to check if a command is available was determined to be command -v.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, oversaw that one

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So remove the which shellcheck and assume that executing shellcheck works?

Copy link
Member

@miri64 miri64 Jul 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, just use command -v instead of which ;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like that is already here :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then you don't have the absolute path (in case that is required).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, I was not aware that command is an actual command and that it can be used as an alternative for which 😑

@cladmi
Copy link
Contributor

cladmi commented Jul 24, 2018

You can squash when you want.

@bergzand bergzand force-pushed the pr/shellcheck/initial branch from 8340f76 to c5c3903 Compare July 29, 2018 13:28
@bergzand
Copy link
Member Author

squashed!

@cladmi cladmi added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 30, 2018
@cladmi cladmi dismissed aabadie’s stale review July 30, 2018 11:52

Outdated and adding by default will be another PR

@cladmi cladmi merged commit 5284849 into RIOT-OS:master Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 left review comments

@smlng smlng smlng approved these changes

@cladmi cladmi cladmi approved these changes

@aabadie aabadie aabadie left review comments

Assignees
No one assigned
Labels
Area: CI Area: Continuous Integration of RIOT components Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2018.10
Development

Successfully merging this pull request may close these issues.
5 participants
@bergzand @cladmi @smlng @miri64 @aabadie