dist/tools: add Python style check in static tests #8078

aabadie · 2017-11-17T16:20:02Z

As required in #8036 (see comment), I'm trying to split the PR in smaller pieces.

This PR is about adding a new checker in the static-test for common Python style issues. I think it's important to keep the automatic test scripts consistent and Python provide very well described rules for that (see PEP8 for instance).

The style checks are performed using Flake8. I also integrated this into Travis (~~that will obviously fail~~).

The static-test target doesn't fail for now but just displays the errors found.

We could also go further with linter (using pylint) but I'm still not convinced for the moment.

aabadie · 2017-11-17T16:53:19Z

Maybe @cladmi you want to look at that one ?

kaspar030 · 2017-11-17T17:11:00Z

dist/tools/flake8/check.sh

@@ -0,0 +1,35 @@
+#!/usr/bin/env bash


because of the colored output when errors are found (using warning color). I couldn't make it work with sh without having tput result in the terminal... Maybe you have an idea ?

Its the same as pr_check/pr_check.sh.

kaspar030 · 2017-11-17T17:11:48Z

dist/tools/flake8/check.sh

+
+RIOTBASE=$(readlink -f "$(dirname $(realpath $0))/../../..")
+
+CHECK_PATHS="${RIOTBASE}/tests \


please use "changed_files.sh" as all the other static checks do

Thanks ! That's much better indeed. I pushed an update that adds this.

aabadie · 2017-11-18T18:40:27Z

I pushed ba3d717 just to make sure errors are raised on modified files and the build remains successful (just a warning). 5a06800 shows that everything is fine after fixing the detected issues.

cladmi · 2017-11-24T17:27:52Z

Sorry, I wanted to look into this but did not have time, I will do next week.

aabadie · 2017-11-27T08:00:33Z

@cladmi, I can also raise an error (exit !0) when problems are found on modified files. What do you prefer ?

aabadie · 2017-11-27T08:04:45Z

I opened #8141 to keep track of the problems raised in the automatic test scripts.

cladmi · 2017-11-28T15:24:38Z

dist/tools/flake8/check.sh

@@ -0,0 +1,35 @@
+#!/usr/bin/env bash


Its the same as pr_check/pr_check.sh.

cladmi · 2017-11-28T15:39:57Z

dist/tools/flake8/check.sh

+    CRESET=
+fi
+
+. ${RIOTBASE:+${RIOTBASE}/}dist/tools/ci/changed_files.sh


This ${RIOTBASE:+${RIOTBASE}/} does nothing, maybe more ${RIOTBASE:+.}/.

It does add a slash, shout riotbase be set, right? So we don't end up with neither "/home/foo/bardist/tools/..." nor "/dist/tools/.../".

cladmi · 2017-11-28T15:43:56Z

dist/tools/flake8/check.sh

+fi
+
+# Allow 119 characters width per line to simplify test output parsing
+ERRORS=$(flake8 --max-line-length=119 ${FILES})


I would prefer changing to using a config file here.

kaspar030 · 2017-11-29T10:17:35Z

dist/tools/flake8/check.sh

+
+if [ -n "${ERRORS}" ]
+then
+    echo -e "${CWARNING}There are style issues in the following Python scripts:${CRESET}"


AFAIR "echo -e" causes trouble on some platforms (OSX?).

printf is better, more portable

aabadie · 2017-11-29T17:02:37Z

@cladmi, I addressed your comments

cladmi · 2017-11-30T17:10:58Z

dist/tools/flake8/check.sh

+    CRESET=
+fi
+
+. ${RIOTBASE:+./}dist/tools/ci/changed_files.sh


Every makefiles is using $(RIOTBASE)/ so I think the '/' can be put after, but maybe I'm wrong.
You could add a DIST_TOOLS variable so the${RIOTBASE} with default value is only done once in the file.

After testing a bit, putting the / outside was giving /dist/tools/... with an empty RIOTBASE. The problem was the + that should be a -:

${RIOTBASE:-.}/dist/tools/

works when RIOTBASE is empty and also when RIOTBASE is a valid path.

And there is the same problem in the ccpcheck.sh script I think

cladmi · 2017-11-30T17:11:17Z

dist/tools/flake8/check.sh

+    exit 0
+fi
+
+# Allow 119 characters width per line to simplify test output parsing


The comment can be moved to the config file I think.

miri64 · 2017-11-30T19:23:35Z

dist/tools/flake8/check.sh

+# directory for more details.
+#
+
+if tput colors &> /dev/null && [ $(tput colors) -ge 8 ]; then


To answer https://github.com/RIOT-OS/RIOT/pull/8078/files#r151843512: The &> here is a bash-ism. The portable way would be > /dev/null 2> /dev/null.

miri64 · 2017-12-11T14:39:20Z

Is there still a dependency, that needs to be merged, before we can merge this one?

aabadie · 2017-12-11T14:45:27Z

Is there still a dependency, that needs to be merged, before we can merge this one?

Murdock workers should now be up-to-date and travis dependency is added by this PR.

Now I'm just wondering if we should consider returning an error (and have the static-test failed) instead of just a warning.

comments addressed

aabadie · 2017-12-14T17:00:26Z

@cladmi do you still see things to change here ?

smlng

looks good to me, and having this check will (hopefully) enforce good python on our test scripts 😉

aabadie · 2017-12-15T10:40:36Z

dist/tools/flake8/check.sh

+then
+    printf "${CWARNING}There are style issues in the following Python scripts:${CRESET}\n\n"
+    printf "${ERRORS}\n"
+    exit 0


just to make this more obvious: this check script never fails but just list problematic files with a warning yellow color. I can change to a more strict rule if desired: red error color + exit 1.

mhm, did you run it all over RIOT checking all python in the current master? If that's the case and all current python stuff sticks with rules, I'd says lets be more strict and exit 1 because most people don't look into build logs if they succeed.

did you run it all over RIOT checking all python in the current master?

Yes and the scripts in test should be fine but not the one in the dist/tools directory. But I agree we should be more strict now and open PRs if problems occur later.

aabadie · 2017-12-15T14:10:02Z

@smlng this PR is now strict with python style issues. The CI will fail if there are problems in a python file touched by a PR.

aabadie · 2017-12-18T11:50:27Z

@cladmi can you have another look at this one ?

aabadie added Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools labels Nov 17, 2017

aabadie requested review from cladmi and miri64 November 17, 2017 16:20

aabadie force-pushed the pr/python_style_check branch 4 times, most recently from c60d709 to 35c6207 Compare November 17, 2017 16:49

aabadie removed request for cladmi and miri64 November 17, 2017 16:52

kaspar030 previously requested changes Nov 17, 2017

View reviewed changes

aabadie force-pushed the pr/python_style_check branch 4 times, most recently from a515d89 to ba3d717 Compare November 18, 2017 18:35

aabadie mentioned this pull request Nov 28, 2017

Fix common python style issues found in automatic test scripts #8141

Closed

12 tasks

cladmi suggested changes Nov 28, 2017

View reviewed changes

kaspar030 reviewed Nov 29, 2017

View reviewed changes

aabadie force-pushed the pr/python_style_check branch 2 times, most recently from f803ab5 to 5023717 Compare November 29, 2017 17:02

aabadie mentioned this pull request Nov 29, 2017

add flake8 dependency RIOT-OS/riotdocker#36

Merged

cladmi reviewed Nov 30, 2017

View reviewed changes

aabadie force-pushed the pr/python_style_check branch 2 times, most recently from 6c8e55b to d185771 Compare November 30, 2017 19:31

miri64 reviewed Nov 30, 2017

View reviewed changes

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 11, 2017

smlng approved these changes Dec 15, 2017

View reviewed changes

aabadie commented Dec 15, 2017

View reviewed changes

aabadie added 2 commits December 15, 2017 14:32

travis: add flake8 dependency

cde171a

dist/tools: add PEP8 check to static tests

035bdf7

aabadie force-pushed the pr/python_style_check branch from d185771 to 035bdf7 Compare December 15, 2017 14:08

cladmi approved these changes Dec 19, 2017

View reviewed changes

cladmi merged commit a40e9ba into RIOT-OS:master Dec 19, 2017

aabadie added this to the Release 2018.01 milestone Jan 18, 2018

aabadie deleted the pr/python_style_check branch June 8, 2018 20:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dist/tools: add Python style check in static tests #8078

dist/tools: add Python style check in static tests #8078

aabadie commented Nov 17, 2017 •

edited

Loading

aabadie commented Nov 17, 2017

kaspar030 Nov 17, 2017

aabadie Nov 18, 2017

cladmi Nov 28, 2017

kaspar030 Nov 29, 2017

kaspar030 Nov 17, 2017

aabadie Nov 18, 2017

aabadie commented Nov 18, 2017 •

edited

Loading

cladmi commented Nov 24, 2017

aabadie commented Nov 27, 2017

aabadie commented Nov 27, 2017

cladmi Nov 28, 2017

cladmi Nov 28, 2017

kaspar030 Nov 29, 2017

cladmi Nov 28, 2017

kaspar030 Nov 29, 2017

jnohlgard Nov 29, 2017

aabadie commented Nov 29, 2017

cladmi Nov 30, 2017

aabadie Nov 30, 2017

aabadie Nov 30, 2017

cladmi Nov 30, 2017

miri64 Nov 30, 2017

miri64 commented Dec 11, 2017

aabadie commented Dec 11, 2017 •

edited

Loading

aabadie commented Dec 14, 2017

smlng left a comment

aabadie Dec 15, 2017

smlng Dec 15, 2017

aabadie Dec 15, 2017

aabadie commented Dec 15, 2017

aabadie commented Dec 18, 2017


		RIOTBASE=$(readlink -f "$(dirname $(realpath $0))/../../..")

		CHECK_PATHS="${RIOTBASE}/tests \

dist/tools: add Python style check in static tests #8078

dist/tools: add Python style check in static tests #8078

Conversation

aabadie commented Nov 17, 2017 • edited Loading

aabadie commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabadie commented Nov 18, 2017 • edited Loading

cladmi commented Nov 24, 2017

aabadie commented Nov 27, 2017

aabadie commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabadie commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 commented Dec 11, 2017

aabadie commented Dec 11, 2017 • edited Loading

aabadie commented Dec 14, 2017

smlng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabadie commented Dec 15, 2017

aabadie commented Dec 18, 2017

aabadie commented Nov 17, 2017 •

edited

Loading

aabadie commented Nov 18, 2017 •

edited

Loading

aabadie commented Dec 11, 2017 •

edited

Loading