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.

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

tools/ci: correcly report flake8 version. #10252

Merged
merged 2 commits into from
Aug 14, 2019

Conversation

jcarrano
Copy link
Contributor

@jcarrano jcarrano commented Oct 25, 2018

This PR is rebased on top if #10251 for easy testing, but it is not required.

Contribution description

This PR contains two commits. I can split it if necessary.

Change the way version detection works

Detect command line tool versions without using "command". "command" may be a builting in some shells, leading to unportability.

The new version uses the status code to correctly detect a non-existent command. This allows it to differentiate between error in the tool and not-found errors.

It also works with compound commands, for example `python -m callable_module".

Fix flake8 detection in some systems (for example, riotdocker)

If the flake8 executable is not found, the static test script reports the tool as missing. It may happen that the flake8 module is installed, but the console entry point is not.

In the flake8 shell script, flake is invoked via python -m. The result is a confusing error message where static-test reports the tools as missing, yet the flake8 tests are run.

This patch makes the toolchain version script use the same command as the flake8 script.

Testing procedure

The easiest way to see the issue is to look at Murdock output from any PR (example) and see that flake8 is reported as missing, but the test that uses flake8 passes.

Travis uses a different image that does not show the problem, that's why I rebased on top of #10251. That PR's Travis output has the issue, while this one should not.

With this PR, the static-tests can detect flake8 without problems.

Why is this important?

Because if we have tests it is because we want to run them and if the output from the static test is contradictory, how do we know if the tests are running or not?

@jcarrano jcarrano added Area: CI Area: Continuous Integration of RIOT components Area: tools Area: Supplementary tools labels Oct 25, 2018
@jcarrano jcarrano changed the title Flake8 report existence tools/ci: correcly report flake8 version. Oct 25, 2018
Copy link
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

See inline comment

@@ -6,11 +6,11 @@ get_cmd_version() {
fi

local cmd="$1"
if command -v "$cmd" 2>&1 >/dev/null; then
ver=$("$cmd" --version 2> /dev/null | head -n 1)
if command -v $cmd 2>&1 >/dev/null; then
Copy link
Member

Choose a reason for hiding this comment

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

This way is not portable when the command is python -m flake8. It works in dash, but in bash this will look for the commands python, -m, and flake8 and print the paths found and return zero if any command is found. In zsh, it looks for the commands like in bash, but additionally, returns a non zero return code if any command is not found.

Shell test locally:

% command -v python3 -m flake8; echo return: $?
/usr/bin/python3
/usr/bin/flake8
return: 1

Copy link
Member

Choose a reason for hiding this comment

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

How about adding optional extra arguments to this function instead, which can be used to pass arguments?

get_cmd_version() {
    if [ -z "$1" ]; then
        return
    fi

    local cmd="$1"
    shift 1
    local args=--version
    if [ $# -gt 0 ]; then
        args="$@"
    fi
    if command -v $cmd 2>&1 >/dev/null; then
        ver=$($cmd $args 2> /dev/null | head -n 1)
        # some tools (eg. openocd) print version info to stderr
        if [ -z "$ver" ]; then
            ver=$($cmd $args 2>&1 | head -n 1)
        fi
        if [ -z "$ver" ]; then
            ver="error"
        fi
    else
        ver="missing"
    fi

    printf "%s" "$ver"
}

and

printf "%23s: %s\n" "flake8" "$(get_cmd_version python3 -m flake8 --version)"

Though, on failure we get this error message instead (because of the captured stderr output):

flake8: /usr/lib/python-exec/python3.6/python3: No module named fake8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I fixed it now.

@jcarrano jcarrano force-pushed the flake8-report-existence branch from c454afb to af83954 Compare November 22, 2018 14:08
@jcarrano jcarrano added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Nov 22, 2018
Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

works for me now, i.e., on macOS:

 ./dist/tools/ci/print_toolchain_versions.sh 

Operating System Environment
-----------------------------
       Operating System: Mac OS X 10.14.2
                 Kernel: Darwin 18.2.0 x86_64 i386

Installed compiler toolchains
-----------------------------
             native gcc: gcc (Homebrew GCC 8.2.0) 8.2.0
      arm-none-eabi-gcc: arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 7-2017-q4-major) 7.2.1 20170904 (release) [ARM/embedded-7-branch revision 255204]
                avr-gcc: avr-gcc (GCC) 8.2.0
       mips-mti-elf-gcc: missing
             msp430-gcc: missing
   riscv-none-embed-gcc: missing
                  clang: Apple LLVM version 10.0.0 (clang-1000.10.44.4)

Installed compiler libs
-----------------------
   arm-none-eabi-newlib: "2.5.0"
    mips-mti-elf-newlib: missing
riscv-none-embed-newlib: missing
               avr-libc: "2.0.0" ("20150208")

Installed development tools
---------------------------
                  cmake: cmake version 3.13.2
               cppcheck: Cppcheck 1.86
                doxygen: 1.8.15
                    git: git version 2.20.1
                   make: GNU Make 3.81
                openocd: Open On-Chip Debugger 0.10.0
                 python: Python 2.7.15
                python2: Python 2.7.15
                python3: Python 3.7.2
                 flake8: 3.5.0 (mccabe: 0.6.1, pycodestyle: 2.3.1, pyflakes: 1.6.0) CPython 3.7.2 on Darwin
             coccinelle: spatch version 1.0.6 compiled with OCaml version 4.05.0

Detect command line tool versions without using "command".
Command may be a builting in some shells, leading to unportability.

The new version uses the status code to correctly detect a non-existent
command. This allows it to differentiate between error in the tool and
not-found errors.

It also works with compound commands, for example `python -m callable_module".
If the flake8 executable is not found, the static test script reports
the tool as missing. It may happen that the flake8 module is installed,
but the console entry point is not.

In the flake8 shell script, flake is invoked via `python -m`. The result
is a confusing error message where static-test reports the tools as missing,
yet the flake8 tests are run.

This patch makes the toolchain version script use the same command as the
flake8 script.
@jcarrano jcarrano force-pushed the flake8-report-existence branch from a704a73 to 2209435 Compare January 15, 2019 19:11
@jcarrano
Copy link
Contributor Author

I rebased-squashed-fixed conflicts.

@gebart Are you OK with this PR?

Copy link
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

See comments, untested though

fi
else
ver="missing"
VERSION_RAW=$( ($@ --version) 2>&1)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be "$@" instead of $@?
(combined with removing the quotes below at the get_cmd_version call for flake8

Suggested change
VERSION_RAW=$( ($@ --version) 2>&1)
VERSION_RAW=$( ("$@" --version) 2>&1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"$@" will treat "a b" as the name of a command instead of command a with argument b.

ver="missing"
VERSION_RAW=$( ($@ --version) 2>&1)
ERR=$?
VERSION=$(echo "$VERSION_RAW" | head -n 1)
Copy link
Member

Choose a reason for hiding this comment

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

it is more reliable to use printf %s "$VERSION_RAW" instead of echo to avoid getting strange outputs in certain situations, especially since $VERSION_RAW may contain anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has the head to make sure we get the fist line in commands that have more than one.

done
printf "%23s: %s\n" "flake8" "$(get_cmd_version "python3 -Wignore -m flake8")"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
printf "%23s: %s\n" "flake8" "$(get_cmd_version "python3 -Wignore -m flake8")"
printf "%23s: %s\n" "flake8" "$(get_cmd_version python3 -Wignore -m flake8)"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment above for $@.

@stale
Copy link

stale bot commented Aug 10, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@miri64 miri64 removed the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019
@smlng
Copy link
Member

smlng commented Aug 12, 2019

@jcarrano can you have another look at @gebart comments, maybe we can get this in quick?!

@smlng smlng added the State: don't stale State: Tell state-bot to ignore this issue label Aug 12, 2019
@jcarrano
Copy link
Contributor Author

@smlng I responded to @gebart's comment. He does not seem active at the moment, so if you think I answered satisfactorily dismiss his review and we merge.

@smlng
Copy link
Member

smlng commented Aug 12, 2019

@cladmi can you have a quick look on this one, thx!

@smlng smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 12, 2019
@smlng smlng merged commit 950b83e into RIOT-OS:master Aug 14, 2019
@cladmi
Copy link
Contributor

cladmi commented Aug 14, 2019

Oops, sorry I missed your message, it got lost in the stale bot flood.

@kb2ma kb2ma added this to the Release 2019.10 milestone Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 State: don't stale State: Tell state-bot to ignore this issue Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants