diff options
| -rw-r--r-- | .gitignore | 18 | ||||
| -rw-r--r-- | docs/bash_code_style.rst | 712 | ||||
| -rw-r--r-- | resources/libraries/bash/entry/bootstrap_verify_perf.sh | 6 | ||||
| -rwxr-xr-x | resources/libraries/bash/entry/bootstrap_vpp_device.sh | 4 | ||||
| -rw-r--r-- | resources/libraries/bash/entry/check/README.txt | 27 | ||||
| -rw-r--r-- | resources/libraries/bash/entry/check/autogen.sh | 61 | ||||
| -rw-r--r-- | resources/libraries/bash/entry/check/line.sh | 49 | ||||
| -rw-r--r-- | resources/libraries/bash/entry/check/pylint.sh | 44 | ||||
| -rwxr-xr-x | resources/libraries/bash/entry/tox.sh | 32 | ||||
| -rw-r--r-- | resources/libraries/bash/function/common.sh | 37 | ||||
| -rw-r--r-- | resources/libraries/python/autogen/Copyright.py | 29 | ||||
| -rw-r--r-- | resources/libraries/python/autogen/Regenerator.py | 29 | ||||
| -rw-r--r-- | tox-requirements.txt | 24 | ||||
| -rw-r--r-- | tox.ini | 70 |
14 files changed, 1082 insertions, 60 deletions
diff --git a/.gitignore b/.gitignore index e12f651b29..9989133ada 100644 --- a/.gitignore +++ b/.gitignore @@ -1,12 +1,26 @@ +# Copyright (c) 2019 Cisco and/or its affiliates. +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at: +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + /env /download_dir /archive_dir +/dmm +/.tox outputs output.xml log.html report.html +*.log *.pyc *~ -*.log .idea -dmm/** diff --git a/docs/bash_code_style.rst b/docs/bash_code_style.rst new file mode 100644 index 0000000000..e5dbc9c06e --- /dev/null +++ b/docs/bash_code_style.rst @@ -0,0 +1,712 @@ +.. + Copyright (c) 2019 Cisco and/or its affiliates. + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at: +.. + http://www.apache.org/licenses/LICENSE-2.0 +.. + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. + + +The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", +"SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", +"MAY", and "OPTIONAL" in this document are to be interpreted as +described in `BCP 14 <https://tools.ietf.org/html/bcp14>`_ +`[RFC2119] <https://tools.ietf.org/html/rfc2119>`_ +`[RFC8174] <https://tools.ietf.org/html/rfc8174>`_ +when, and only when, they appear in all capitals, as shown here. + +This document SHALL describe guidelines for writing reliable, maintainable, +reusable and readable code for CSIT. + +Motivation +^^^^^^^^^^ + +TODO: List reasons why we need code style document for Bash. + +Proposed style +^^^^^^^^^^^^^^ + +File types +~~~~~~~~~~ + +Bash files SHOULD NOT be monolithic. Generally, this document +considers two types of bash files: + ++ Entry script: Assumed to be called by user, + or a script "external" in some way. + + + Sources bash libraries and calls functions defined there. + ++ Library file: To be sourced by entry scipts, possibly also by other libraries. + + + Sources other libraries for functions it needs. + + + Or relies on a related file already having sourced that. + + + Documentation SHALL imply which case it is. + + + Defines multiple functions other scripts can call. + +Safety +~~~~~~ + ++ Variable expansions MUST be quoted, to prevent word splitting. + + + This includes special "variables" such as "${1}". + + + RECOMMENDED even if the value is safe, as in "$?" and "$#". + + + It is RECOMMENDED to quote strings in general, + so text editors can syntax-highlight them. + + + Even if the string is a numeric value. + + + Commands and known options can get their own highlight, no need to quote. + + + Example: You do not need to quote every word of + "pip install --upgrade virtualenv". + + + Code SHALL NOT quote glob characters you need to expand (obviously). + + + OPTIONALLY do not quote adjacent characters (such as dot or fore-slash), + so that syntax highlighting makes them stand out compared to surrounding + ordinary strings. + + + Example: cp "logs"/*."log" "."/ + + + TODO: Consider giving examples both for good and bad usage. + + + Command substitution on right hand side of assignment should be safe + without quotes. + + + But still, quotes are RECOMMENDED, unless line length is a concern. + + + Note that command substitution limits the scope for quotes, + so it is NOT REQUIRED to escape the quotes in deeper levels. + + + TODO: Do we prefer backtics, or "dollar round-bracket"? + + + Backticks are more readable, especially when there are + round brackets in the surrounding text. + + + Backticks do not allow nested command substitution. + + + But we might want to avoid nested command substitution anyway? + + + Code SHOULD NOT be structured in a way where + word splitting is intended. + + + Example: Variable holding string of multiple command lines arguments. + + + Solution: Array variable should be used in this case. + + + Expansion MUST use quotes then: "${name[@]}". + + + Word splitting MAY be used when creating arrays from command substitution. + ++ Code MUST always check the exit code of commands. + + + Traditionally, error code checking is done either by "set -e" + or by appending "|| die" after each command. + The first is unreliable, due to many rules affecting "set -e" behavior + (see <https://mywiki.wooledge.org/BashFAQ/105>), but "|| die" + relies on humans identifying each command, which is also unreliable. + When was the last time you checked error code of "echo" command, + for example? + + + Another example: "set -e" in your function has no effect + if any ancestor call is done with logical or, + for example in "func || code=$?" construct. + + + As there is no reliable method of error detection, and there are two + largely independent unreliable methods, the best what we can do + is to apply both. So, code SHOULD explicitly + check each command (with "|| die" and similar) AND have "set -e" applied. + + + Code MUST explicitly check each command, unless the command is well known, + and considered safe (such as the aforementioned "echo"). + + + The well known commands MUST still be checked implicitly via "set -e". + + + See below for specific "set -e" recommendations. + ++ Code SHOULD use "readlink -e" (or "-f" if target does not exist yet) + to normalize any path value to absolute path without symlinks. + It helps with debugging and identifies malformed paths. + ++ Code SHOULD use such normalized paths for sourcing. + ++ When exiting on a known error, code MUST print a longer, helpful message, + in order for the user to fix their situation if possible. + ++ When error happens at an unexpected place, it is RECOMMENDED for the message + to be short and generic, instead of speculative. + +Bash options +~~~~~~~~~~~~ + ++ Code MUST apply "-x" to make debugging easier. + + + Code MAY temporarily supress such output in order to avoid spam + (e.g. in long busy loops), but it is still NOT RECOMMENDED to do so. + ++ Code MUST apply "-e" for early error detection. + + + But code still SHOULD use "|| die" for most commands, + as "-e" has numerous rules and exceptions. + + + Code MAY apply "+e" temporarily for commands which (possibly nonzero) + exit code it interested in. + + + Code MUST to store "$?" and call "set -e" immediatelly afterwards. + + + Code MUST NOT use this approach when calling functions. + + + That is because functions are instructed to apply "set -e" on their own + which (when triggered) will exit the whole entry script. + + + Unless overriden by ERR trap. + But code SHOULD NOT set any ERR trap. + + + If code needs exit code of a function, it is RECOMMENDED to use + pattern 'code="0"; called_function || code="${?}"'. + + + In this case, contributor MUST make sure nothing in the + called_function sub-graph relies on "set -e" behavior, + because the call being part of "or construct" disables it. + + + Code MAY append "|| true" for benign commands, + when it is clear non-zero exit codes make no difference. + + + Also in this case, the contributor MUST make sure nothing within + the called sub-graph depends on "set -e", as it is disabled. + ++ Code MUST apply "-u" as unset variable is generally a typo, thus an error. + + + Code MAY temporarily apply "+u" if a command needs that to pass. + + + Virtualenv activation is the only known example so far. + ++ Code MUST apply "-o pipefail" to make sure "-e" picks errors + inside piped construct. + + + Code MAY use "|| true" inside a pipe construct, in the (inprobable) case + when non-zero exit code still results in a meaningful pipe output. + ++ All together: "set -exuo pipefail". + + + Code MUST put that line near start of every file, so we are sure + the options are applied no matter what. + + + "Near start" means "before any nontrivial code". + + + Basically only copyright is RECOMMENDED to appear before. + + + Also code MUST put the line near start of function bodies + and subshell invocations. + +Functions +~~~~~~~~~ + +There are (at least) two possibilities how a code from an external file +can be executed. Either the file contains a code block to execute +on each "source" invocation, or the file just defines functions +which have to be called separately. + +This document considers the "function way" to be better, +here are some pros and cons: + ++ Cons: + + + The function way takes more space. Files have more lines, + and the code in function body is one indent deeper. + + + It is not easy to create functions for low-level argument manipulation, + as "shift" command in the function code does not affect the caller context. + + + Call sites frequently refer to code two times, + when sourcing the definition and when executing the function. + + + It is not clear when a library can rely on its relative + to have performed the sourcing already. + + + Ideally, each library should detect if it has been sourced already + and return early, which takes even more space. + ++ Pros: + + + Some code blocks are more useful when used as function, + to make call site shorter. + + + Examples: Trap functions, "die" function. + + + The "import" part and "function" part usually have different side effects, + making the documentation more focused (even if longer overall). + + + There is zero risk of argument-less invocation picking arguments + from parent context. + + + This safety feature is the main reason for chosing the "function way". + + + This allows code blocks to support optional arguments. + ++ Rules: + + + Library files MUST be only "source"d. For example if "tox" calls a script, + it is an entry script. + + + Library files (upon sourcing) MUST minimize size effect. + + + The only permitted side effects MUST by directly related to: + + + Defining functions (without executing them). + + + Sourcing sub-library files. + + + If a bash script indirectly call another bash script, + it is not a "source" operation, variables are not shared, + so the called script MUST be considered an entry script, + even if it implements logic fitting into a single function. + + + Entry scripts SHOULD avoid duplicating any logic. + + + Clear duplicated blocks MUST be moved into libraries as functions. + + + Blocks with low amount of duplication MAY remain in entry scripts. + + + Usual motives for not creating functions are: + + + The extracted function would have too much logic for processing + arguments (instead of hardcoding values as in entry script). + + + The arguments needed would be too verbose. + + + And using "set +x" would take too much vertical space + (when compared to entry script implementation). + +Variables +~~~~~~~~~ + +This document describes two kinds of variables: called "local" and "global". + +TODO: Find better adjectives for the two kinds defined here, +if the usual bash meaning makes reader forget other specifics. +For example, variable with lowercase name and not marked by "local" builtin, +is cosidered "global" from bash point of view, but "local" from this document +point of view. + ++ Local variables: + + + Variable name MUST contain only lower case letters, digits and underscores. + + + Code MUST NOT export local variables. + + + Code MUST NOT rely on local variables set in different contexts. + + + Documentation is NOT REQUIRED. + + + Variable name SHOULD be descriptive enough. + + + Local variable MUST be initialized before first use. + + + Code SHOULD have a comment if a reader might have missed + the initialization. + + + TODO: Agree on level of defensiveness (against local values + being influenced by other functions) needed. + Possible alternatives / additions to the "always initialize" rule: + + + Unset local variables when leaving the function. + + + Explicitly typeset by "local" builtin command. + + + Require strict naming convention, e.g. function_name__variable_name. + ++ Global variables: + + + Variable name MUST contain only upper case letters, digits and underscores. + + + They SHOULD NOT be exported, unless external commands need them + (e.g. PYTHONPATH). + + + TODO: Propose a strict naming convention, or a central document + of all used global variables, to prevent contributors + from causing variable name conflicts. + + + Code MUST document if a function (or its inner call) + reads a global variable. + + + Code MUST document if a function (or its inner call) + sets or rewrites a global variable. + + + If a function "wants to return a value", it SHOULD be implemented + as the function setting (or rewriting) a global variable, + and the call sites reading that variable. + + + If a function "wants to accept an argument", it IS RECOMMENDED + to be implemented as the call sites setting or rewriting global variables, + and the function reading that variables. + But see below for direct arguments. + ++ Code MUST use curly brackets when referencing variables, + e.g. "${my_variable}". + + + It makes related constructs (such as ${name:-default}) less surprising. + + + It looks more similar to Robot Framework variables (which is good). + +Arguments +~~~~~~~~~ + +Bash scripts and functions MAY accept arguments, named "${1}", "${2}" and so on. +As a whole available via "$@". +You MAY use "shift" command to consume an argument. + +Contexts +```````` + +Functions never have access to parent arguments, but they can read and write +variables set or read by parent contexts. + +Arguments or variables +`````````````````````` + ++ Both arguments and global variables MAY act as an input. + ++ In general, if the caller is likely to supply the value already placed + in a global variable of known name, it is RECOMMENDED + to use that global variable. + ++ Construct "${NAME:-value}" can be used equally well for arguments, + so default values are possible for both input methods. + ++ Arguments are positional, so there are restrictions on which input + is optional. + ++ Functions SHOULD either look at arguments (possibly also + reading global variables to use as defaults), or look at variables only. + ++ Code MUST NOT rely on "${0}", it SHOULD use "${BASH_SOURCE[0]}" instead + (and apply "readlink -e") to get the current block location. + ++ For entry scripts, it is RECOMMENDED to use standard parsing capabilities. + + + For most Linux distros, "getopt" is RECOMMENDED. + +Working directory handling +~~~~~~~~~~~~~~~~~~~~~~~~~~ + ++ Functions SHOULD act correctly without neither assuming + what the currect working directory is, nor changing it. + + + That is why global variables and arguments SHOULD contain + (normalized) full paths. + + + Motivation: Different call sites MAY rely on different working directories. + ++ A function MAY return (also with nonzero exit code) when working directory + is changed. + + + In this case the function documentation MUST clearly state where (and when) + is the working directory changed. + + + Exception: Functions with undocumented exit code. + + + Those functions MUST return nonzero code only on "set -e" or "die". + + + Note that both "set -e" and "die" by default result in exit of the whole + entry script, but the caller MAY have altered that behavior + (by registering ERR trap, or redefining die function). + + + Any callers which use "set +e" or "|| true" MUST make sure + their (and their caller ancestors') assumption on working directory + are not affected. + + + Such callers SHOULD do that by restoring the original working directory + either in their code, + + + or contributors SHOULD do such restoration in the function code, + (see below) if that is more convenient. + + + Motivation: Callers MAY rely on this side effect to simplify their logic. + ++ A function MAY assume a particular directory is already set + as the working directory (to save space). + + + In this case function documentation MUST clearly state what the assumed + working directory is. + + + Motivation: Callers MAY call several functions with common + directory of interest. + + + Example: Several dowload actions to execute in sequence, + implemented as functions assuming ${DOWNLOAD_DIR} + is the working directory. + ++ A function MAY change the working directory transiently, + before restoring it back before return. + + + Such functions SHOULD use command "pushd" to change the working directory. + + + Such functions SHOULD use "trap 'trap - RETURN; popd' RETURN" + imediately after the pushd. + + + In that case, the "trap - RETURN" part MUST be included, + to restore any trap set by ancestor. + + + Functions MAY call "trap - RETURN; popd" exlicitly. + + + Such functions MUST NOT call another pushd (before an explicit popd), + as traps do not stack within a function. + ++ If entry scripts also use traps to restore working directory (or other state), + they SHOULD use EXIT traps instead. + + + That is because "exit" command, as well as the default behavior + of "die" or "set -e" cause direct exit (without skipping function returns). + +Function size +~~~~~~~~~~~~~ + ++ In general, code SHOULD follow reasoning similar to how pylint + limits code complexity. + ++ It is RECOMMENDED to have functions somewhat simpler than Python functions, + as Bash is generally more verbose and less readable. + ++ If code contains comments in order to partition a block + into sub-blocks, the sub-blocks SHOULD be moved into separate functions. + + + Unless the sub-blocks are essentially one-liners, + not readable just because external commands do not have + obvious enough parameters. Use common sense. + +Documentation +~~~~~~~~~~~~~ + ++ The library path and filename is visible from source sites. It SHOULD be + descriptive enough, so reader do not need to look inside to determine + how and why is the sourced file used. + + + If code would use several functions with similar names, + it is RECOMMENDED to create a (well-named) sub-library for them. + + + Code MAY create deep library trees if needed, it SHOULD store + common path prefixes into global variables to make sourcing easier. + + + Contributors, look at other files in the subdirectory. You SHOULD + improve their filenames when adding-removing other filenames. + + + Library files SHOULD NOT have executable flag set. + + + Library files SHOULD have an extension .sh (or perhaps .bash). + + + It is RECOMMENDED for entry scripts to also have executable flag unset + and have .sh extension. + ++ Each entry script MUST start with a shebang. + + + "#!/bin/usr/env bash" is RECOMMENDED. + + + Code SHOULD put an empty line after shebang. + + + Library files SHOULD NOT contain a shebang, as "source" is the primary + method to include them. + ++ Following that, there SHOULD be a block of comment lines with copyright. + + + It is a boilerplate, but human eyes are good at ignoring it. + + + Overhead for git is also negligible. + ++ Following that, there MUST be "set -exuo pipefail". + + + It acts as an anchor for humans to start paying attention. + +Then it depends on script type. + +Library documentation +````````````````````` + ++ Following "set -exuo pipefail" SHALL come the "import part" documentation. + ++ Then SHALL be the import code + ("source" commands and a bare minimum they need). + ++ Then SHALL be the function definitions, and inside: + + + "set -exuo pipefail" again. + + + Following that SHALL be the function documentation explaining API contract. + Similar to Robot [Documentation] or Python function-level docstring. + + + See below. + + + Following that SHALL be varius TODOs, FIXMEs and code itself. + + + "Code itself" SHALL include comment lines + explaining any non-obvious logic. + + + TODO: Document (in an appropriate place) how TODOs differ from FIXMEs. + + + There SHALL be two empty lines before next function definition. + +More details on function documentation: + +Generally, code SHOULD use comments to explain anything +not obvious from the funtion name. + ++ Function documentation SHOULD start with short description of function + operation or motivation, but only if not obvious from function name. + ++ Documentation SHOULD continue with listing any non-obvious side effect: + + + Documentation MUST list all read global variables. + + + Documentation SHOULD include descriptions of semantics + of global variable values. + It is RECOMMENDED to mention which function is supposed to set them. + + + The "include descriptions" part SHOULD apply to other items as well. + + + Documentation MUST list all global variables set, unset, reset, + or otherwise updated. + + + It is RECOMMENDED to list all hardcoded values used in code. + + + Not critical, but can hint at future improvements. + + + Documentation MUST list all files or directories read + (so caller can make sure their content is ready). + + + Documentation MUST list all files or directories updated + (created, deleted, emptied, otherwise edited). + + + Documentation SHOULD list all functions called (so reader can look them up). + + + Documentation SHOULD mention where are the functions defined, + if not in the current file. + + + Documentation SHOULD list all external commands executed. + + + Because their behavior can change "out of bounds", meaning + the contributor changing the implementation of the extrenal command + can be unaware of this particular function interested in its side effects. + + + Documentation SHOULD explain exit code (coming from + the last executed command). + + + Usually, most functions SHOULD be "pass or die", + but some callers MAY be interested in nonzero exit codes + without using global variables to store them. + + + Remember, "exit 1" ends not only the function, but all scripts + in the source chain, so code MUST NOT use it for other purposes. + + + Code SHOULD call "die" function instead. This way the caller can + redefine that function, if there is a good reason for not exiting + on function failure. + + + TODO: Programs installed, services started, URLs downloaded from, ... + + + TODO: Add more items when you spot them. + + + TODO: Is the current order recommended? + +Entry script documentation +`````````````````````````` + ++ After "set -exuo pipefail", high-level description SHALL come. + + + Then TODOs and FIXMEs SHALL be placed (if any). + + + Entry scripts are rarely reused, so detailed side effects + are OPTIONAL to document. + + + But code SHOULD document the primary side effects. + ++ Then SHALL come few commented lines to import the library with "die" function. + ++ Then block of "source" commands for sourcing other libraries needed SHALL be. + + + In alphabetical order, any "special" library SHOULD be + in the previous block (for "die"). + ++ Then block os commands processing arguments SHOULD be (if needed). + ++ Then SHALL come block of function calls (with parameters as needed). + +Other general recommendations +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + ++ Code SHOULD NOT not repeat itself, even in documentation: + + + For hardcoded values, a general description SHOULD be written + (instead of copying the value), so when someone edits the value + in the code, the description still applies. + + + If affected directory name is taken from a global variable, + documentation MAY distribute the directory description + over the two items. + + + If most of side effects come from an inner call, + documentation MAY point the reader to the documentation + of the called function (instead of listing all the side effects). + + + TODO: Composite functions can have large effects. Should we require + intermediate functions to actively hide them whenever possible? + ++ But documentation SHOULD repeat it if the information crosses functions. + + + Item description MUST NOT be skipped just because the reader + should have read parent/child documentation already. + + + Frequently it is RECOMMENDED to copy&paste item descriptions + between functions. + + + But sometimes it is RECOMMENDED to vary the descriptions. For example: + + + A global variable setter MAY document how does it figure out the value + (without caring about what it will be used for by other functions). + + + A global variable reader MAY document how does it use the value + (without caring about how has it been figured out by the setter). + ++ When possible, Bash code SHOULD be made to look like Python + (or Robot Framework). Those are three primary languages CSIT code relies on, + so it is nicer for the readers to see similar expressions when possible. + Examples: + + + Code MUST use indentation, 1 level is 4 spaces. + + + Code SHOULD use "if" instead of "&&" constructs. + + + For comparisons, code SHOULD use operators such as "!=" (needs "[["). + ++ Code MUST NOT use more than 80 characters per line. + + + If long external command invocations are needed, + code SHOULD use array variables to shorten them. + + + If long strings (or arrays) are needed, code SHOULD use "+=" operator + to grow the value over multiple lines. + + + If "|| die" does not fit with the command, code SHOULD use curly braces: + + + Current line has "|| {", + + + Next line has the die commands (indented one level deeper), + + + Final line closes with "}" at original intent level. + + + TODO: Recommend what to do with other constructs. + + + For example multiple piped commands. + + + No, "eval" is too unsafe to use. diff --git a/resources/libraries/bash/entry/bootstrap_verify_perf.sh b/resources/libraries/bash/entry/bootstrap_verify_perf.sh index 1a0d638da1..aad2969ecc 100644 --- a/resources/libraries/bash/entry/bootstrap_verify_perf.sh +++ b/resources/libraries/bash/entry/bootstrap_verify_perf.sh @@ -1,6 +1,4 @@ -#!/usr/bin/env bash - -# Copyright (c) 2018 Cisco and/or its affiliates. +# Copyright (c) 2019 Cisco and/or its affiliates. # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at: @@ -39,7 +37,7 @@ get_test_tag_string || die select_topology || die gather_build || die check_download_dir || die -activate_virtualenv "${CSIT_DIR}" || die +activate_virtualenv || die reserve_testbed || die select_tags || die compose_pybot_arguments || die diff --git a/resources/libraries/bash/entry/bootstrap_vpp_device.sh b/resources/libraries/bash/entry/bootstrap_vpp_device.sh index 6d389d6baf..139e1e99ab 100755 --- a/resources/libraries/bash/entry/bootstrap_vpp_device.sh +++ b/resources/libraries/bash/entry/bootstrap_vpp_device.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash -# Copyright (c) 2018 Cisco and/or its affiliates. +# Copyright (c) 2019 Cisco and/or its affiliates. # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at: @@ -37,7 +37,7 @@ get_test_tag_string || die select_topology || die gather_build || die check_download_dir || die -activate_virtualenv "${CSIT_DIR}" || die +activate_virtualenv || die activate_docker_topology || die select_vpp_device_tags || die compose_pybot_arguments || die diff --git a/resources/libraries/bash/entry/check/README.txt b/resources/libraries/bash/entry/check/README.txt new file mode 100644 index 0000000000..a72274464c --- /dev/null +++ b/resources/libraries/bash/entry/check/README.txt @@ -0,0 +1,27 @@ +# Copyright (c) 2019 Cisco and/or its affiliates. +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at: +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +This directory contains checker scripts and other files they need. +Each checker script is assumed to be run from tox, +when working directory is set to ${CSIT_DIR}. +Each script should: ++ Return nonzero exit code when it fails. +++ The tox might ignore the code when the check is not blocking. ++ Write less verbose output to stderr. ++ Write (to stderr) PASSED or FAILED to help with debugging. ++ Direct more verbose output to appropriately named .log file. ++ Only the output suitable for automated processing by an external caller + should be written to stdout. +++ The level of "less verbose" depends on check and state of codebase. ++ TODO: Should we carefully document which files are + whitelisted/blacklisted for a particulat check? diff --git a/resources/libraries/bash/entry/check/autogen.sh b/resources/libraries/bash/entry/check/autogen.sh new file mode 100644 index 0000000000..35ecc281ee --- /dev/null +++ b/resources/libraries/bash/entry/check/autogen.sh @@ -0,0 +1,61 @@ +# Copyright (c) 2019 Cisco and/or its affiliates. +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at: +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -exuo pipefail + +# This file should be executed from tox, as the assumend working directory +# is different from where this file is located. +# This file does not have executable flag nor shebang, +# to dissuade non-tox callers. + +# This script run every executable *.py script anywhere within tests/ dir, +# the working directory temporarily changed to where the *.py file is. +# Proper virtualenv is assumed to be active. +# If "git diff" sees any change, this script fails. +# The diff output stored to autogen.log (overwriting). +# The *.py files are assumed to be robot suite generators, +# any change means the contribution does not match the generated code. + +# "set -eu" handles failures from the following two lines. +BASH_CHECKS_DIR="$(dirname $(readlink -e "${BASH_SOURCE[0]}"))" +BASH_FUNCTION_DIR="$(readlink -e "${BASH_CHECKS_DIR}/../../function")" +source "${BASH_FUNCTION_DIR}/common.sh" || { + echo "Source failed." >&2 + exit 1 +} + +work_dir="$(pwd)" || die +trap "cd '${work_dir}'" EXIT || die +file_list="$(find ./tests -type f -executable -name '*.py')" || die + +for gen in ${file_list}; do + directory="$(dirname "${gen}")" || die + filename="$(basename "${gen}")" || die + pushd "${directory}" || die + ./"${filename}" || die + popd || die +done + +lines="$(git diff | tee "autogen.log" | wc -l)" || die +if [ "${lines}" != "0" ]; then + # TODO: Decide which text goes to stdout and which to stderr. + warn "Autogen conflict diff nonzero lines: ${lines}" + # TODO: Disable if output size does more harm than good. + cat "autogen.log" >&2 + warn + warn "Autogen checker: FAIL" + exit 1 +fi + +warn +warn "Autogen checker: PASS" diff --git a/resources/libraries/bash/entry/check/line.sh b/resources/libraries/bash/entry/check/line.sh new file mode 100644 index 0000000000..c58c7d0126 --- /dev/null +++ b/resources/libraries/bash/entry/check/line.sh @@ -0,0 +1,49 @@ +# Copyright (c) 2019 Cisco and/or its affiliates. +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at: +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -exuo pipefail + +# This file should be executed from tox, as the assumend working directory +# is different from where this file is located. +# This file does not have executable flag nor shebang, +# to dissuade non-tox callers. + +# This script runs a grep-based command and fails if it detects any lines +# longer than 80 characters. +# The grep output stored to lines.log (overwriting). + +# "set -eu" handles failures from the following two lines. +BASH_CHECKS_DIR="$(dirname $(readlink -e "${BASH_SOURCE[0]}"))" +BASH_FUNCTION_DIR="$(readlink -e "${BASH_CHECKS_DIR}/../../function")" +source "${BASH_FUNCTION_DIR}/common.sh" || { + echo "Source failed." >&2 + exit 1 +} + +# docs contains too many wide formatted tables. +# .txt contains lines with wide URLs. +piped_command='set -exuo pipefail && grep -rn ".\{81\}" "resources/" "tests/"' +piped_command+=' | fgrep -v .svg | fgrep -v .txt | tee "lines.log" | wc -l' +lines="$(bash -c "${piped_command}")" || die +if [ "${lines}" != "0" ]; then + # TODO: Decide which text goes to stdout and which to stderr. + warn "Long lines detected: ${lines}" + ## TODO: Enable when output size does more good than harm. + # cat "lines.log" >&2 + warn + warn "Line length checker: FAIL" + exit 1 +fi + +warn +warn "Line length checker: PASS" diff --git a/resources/libraries/bash/entry/check/pylint.sh b/resources/libraries/bash/entry/check/pylint.sh new file mode 100644 index 0000000000..a3cad9a0d7 --- /dev/null +++ b/resources/libraries/bash/entry/check/pylint.sh @@ -0,0 +1,44 @@ +# Copyright (c) 2019 Cisco and/or its affiliates. +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at: +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -exuo pipefail + +# This file should be executed from tox, as the assumend working directory +# is different from where this file is located. +# This file does not have executable flag nor shebang, +# to dissuade non-tox callers. + +# This script runs pylint and propagates its exit code. +# Config is taken from pylint.cfg, and proper virtualenv is assumed to be active. +# The pylint output stored to pylint.log (overwriting). + +# "set -eu" handles failures from the following two lines. +BASH_CHECKS_DIR="$(dirname $(readlink -e "${BASH_SOURCE[0]}"))" +BASH_FUNCTION_DIR="$(readlink -e "${BASH_CHECKS_DIR}/../../function")" +source "${BASH_FUNCTION_DIR}/common.sh" || { + echo "Source failed." >&2 + exit 1 +} +pylint_args=("--rcfile=pylint.cfg" "resources/" "tests/") +if pylint "${pylint_args[@]}" > "pylint.log"; then + warn + warn "Pylint checker: PASS" +else + # TODO: Decide which text goes to stdout and which to stderr. + warn "Pylint exited with nonzero status." + ## TODO: Enable when output size does more good than harm. + # cat "pylint.log" >&2 + warn + warn "Pylint checker: FAIL" + exit 1 +fi diff --git a/resources/libraries/bash/entry/tox.sh b/resources/libraries/bash/entry/tox.sh new file mode 100755 index 0000000000..82b7ef5307 --- /dev/null +++ b/resources/libraries/bash/entry/tox.sh @@ -0,0 +1,32 @@ +#!/usr/bin/env bash + +# Copyright (c) 2019 Cisco and/or its affiliates. +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at: +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -exuo pipefail + +# This is a launcher script, to be called from Jenkins. +# It runs tox at $CSIT_DIR (after activating virtualenv with its requirements). +# Exit code of tox is propagated. + +# "set -eu" handles failures from the following two lines. +BASH_ENTRY_DIR="$(dirname $(readlink -e "${BASH_SOURCE[0]}"))" +BASH_FUNCTION_DIR="$(readlink -e "${BASH_ENTRY_DIR}/../function")" +source "${BASH_FUNCTION_DIR}/common.sh" || { + echo "Source failed." >&2 + exit 1 +} +common_dirs || die +cd "${CSIT_DIR}" || die +activate_virtualenv "${CSIT_DIR}" "${CSIT_DIR}/tox-requirements.txt" || die +tox # Return code is turned into Jenkins job vote. diff --git a/resources/libraries/bash/function/common.sh b/resources/libraries/bash/function/common.sh index 5982af9427..34aa44f338 100644 --- a/resources/libraries/bash/function/common.sh +++ b/resources/libraries/bash/function/common.sh @@ -1,4 +1,4 @@ -# Copyright (c) 2018 Cisco and/or its affiliates. +# Copyright (c) 2019 Cisco and/or its affiliates. # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at: @@ -90,40 +90,43 @@ function activate_virtualenv () { set -exuo pipefail + # Update virtualenv pip package, delete and create virtualenv directory, + # activate the virtualenv, install requirements, set PYTHONPATH. + # Arguments: - # - ${1} - Non-empty path to existing directory for creating virtualenv in. + # - ${1} - Path to existing directory for creating virtualenv in. + # If missing or empty, ${CSIT_DIR} is used. + # - ${2} - Path to requirements file, ${CSIT_DIR}/requirements.txt if empty. # Variables read: # - CSIT_DIR - Path to existing root of local CSIT git repository. - # Variables set: - # - ENV_DIR - Path to the created virtualenv subdirectory. # Variables exported: # - PYTHONPATH - CSIT_DIR, as CSIT Python scripts usually need this. # Functions called: # - die - Print to stderr and exit. - # TODO: Do we really need to have ENV_DIR available as a global variable? - - if [[ "${1-}" == "" ]]; then - die "Root location of virtualenv to create is not specified." - fi - ENV_DIR="${1}/env" - rm -rf "${ENV_DIR}" || die "Failed to clean previous virtualenv." + # TODO: Do we want the callers to be able to set the env dir name? + # TODO: + In that case, do we want to support env switching? + # TODO: + In that case we want to make env_dir global. + # TODO: Do we want the callers to override PYTHONPATH loaction? + root_path="${1-$CSIT_DIR}" + env_dir="${root_path}/env" + req_path=${2-$CSIT_DIR/requirements.txt} + rm -rf "${env_dir}" || die "Failed to clean previous virtualenv." pip install --upgrade virtualenv || { die "Virtualenv package install failed." } - virtualenv "${ENV_DIR}" || { + virtualenv "${env_dir}" || { die "Virtualenv creation failed." } set +u - source "${ENV_DIR}/bin/activate" || die "Virtualenv activation failed." + source "${env_dir}/bin/activate" || die "Virtualenv activation failed." set -u - pip install -r "${CSIT_DIR}/requirements.txt" || { - die "CSIT requirements installation failed." + pip install --upgrade -r "${req_path}" || { + die "Requirements installation failed." } - # Most CSIT Python scripts assume PYTHONPATH is set and exported. - export PYTHONPATH="${CSIT_DIR}" || die "Export failed." + export PYTHONPATH="${root_path}" || die "Export failed." } diff --git a/resources/libraries/python/autogen/Copyright.py b/resources/libraries/python/autogen/Copyright.py deleted file mode 100644 index e8c72f0624..0000000000 --- a/resources/libraries/python/autogen/Copyright.py +++ /dev/null @@ -1,29 +0,0 @@ -# Copyright (c) 2018 Cisco and/or its affiliates. -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at: -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -"""Module defining sole constant holding copyright text for current year.""" - - -COPYRIGHT = '''# Copyright (c) 2018 Cisco and/or its affiliates. -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at: -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -'''
\ No newline at end of file diff --git a/resources/libraries/python/autogen/Regenerator.py b/resources/libraries/python/autogen/Regenerator.py index 8bfe054e5e..c822b16d10 100644 --- a/resources/libraries/python/autogen/Regenerator.py +++ b/resources/libraries/python/autogen/Regenerator.py @@ -1,4 +1,4 @@ -# Copyright (c) 2018 Cisco and/or its affiliates. +# Copyright (c) 2019 Cisco and/or its affiliates. # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at: @@ -13,16 +13,25 @@ """Module defining utilities for test directory regeneration.""" +from __future__ import print_function + from glob import glob from os import getcwd +import sys from .DefaultTestcase import DefaultTestcase +# Copied from https://stackoverflow.com/a/14981125 +def eprint(*args, **kwargs): + """Print to stderr.""" + print(*args, file=sys.stderr, **kwargs) + + class Regenerator(object): """Class containing file generating methods.""" - def __init__(self, testcase_class=DefaultTestcase): + def __init__(self, testcase_class=DefaultTestcase, quiet=True): """Initialize Testcase class to use. TODO: See the type doc for testcase_class? @@ -31,9 +40,12 @@ class Regenerator(object): :param testcase_class: Subclass of DefaultTestcase for generation. Default: DefaultTestcase + :param quiet: Reduce log prints (to stderr) when True (default). :type testcase_class: subclass of DefaultTestcase accepting suite_id + :type quiet: boolean """ self.testcase_class = testcase_class + self.quiet = quiet def regenerate_glob(self, pattern, protocol="ip4", tc_kwargs_list=None): """Regenerate files matching glob pattern based on arguments. @@ -43,6 +55,8 @@ class Regenerator(object): test cases, autonumbering them, taking arguments from list. If the list is None, use default list, which depends on ip6 usage. + Log-like prints are emited to sys.stderr. + :param pattern: Glob pattern to select files. Example: *-ndrpdr.robot :param is_ip6: Flag determining minimal frame size. Default: False :param tc_kwargs_list: Arguments defining the testcases. Default: None @@ -129,7 +143,8 @@ class Regenerator(object): num = add_testcase(testcase, iface, suite_id, file_out, num, **tc_kwargs) - print "Regenerator starts at {cwd}".format(cwd=getcwd()) + if not self.quiet: + eprint("Regenerator starts at {cwd}".format(cwd=getcwd())) min_framesize = protocol_to_min_framesize[protocol] kwargs_list = tc_kwargs_list if tc_kwargs_list else [ {"framesize": min_framesize, "phy_cores": 1}, @@ -146,7 +161,8 @@ class Regenerator(object): {"framesize": "IMIX_v4_1", "phy_cores": 4} ] for filename in glob(pattern): - print "Regenerating filename:", filename + if not self.quiet: + eprint("Regenerating filename:", filename) with open(filename, "r") as file_in: text = file_in.read() text_prolog = "".join(text.partition("*** Test Cases ***")[:-1]) @@ -155,5 +171,6 @@ class Regenerator(object): with open(filename, "w") as file_out: file_out.write(text_prolog) add_testcases(testcase, iface, suite_id, file_out, kwargs_list) - print "Regenerator ends." - print # To make autogen check output more readable. + if not self.quiet: + eprint("Regenerator ends.") + eprint() # To make autogen check output more readable. diff --git a/tox-requirements.txt b/tox-requirements.txt new file mode 100644 index 0000000000..6f56438bcf --- /dev/null +++ b/tox-requirements.txt @@ -0,0 +1,24 @@ +# Copyright (c) 2019 Cisco and/or its affiliates. +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at: +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Bump regularly. +tox==3.7.0 + +# Tox dependencies. Consult "pip freeze" after installing +# bumped tox into an empty virtualenv. +filelock==3.0.10 +pluggy==0.8.1 +py==1.7.0 +six==1.12.0 +toml==0.10.0 +virtualenv==16.4.0 diff --git a/tox.ini b/tox.ini new file mode 100644 index 0000000000..0fd8645328 --- /dev/null +++ b/tox.ini @@ -0,0 +1,70 @@ +# Copyright (c) 2019 Cisco and/or its affiliates. +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at: +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Idea of this file is motivated by OpenDylight, +# especially its Integration/Test sub-project. + +# This file requires active virtualenv with tox package installed, +# or python-tox system package installed. + +# Usage: +# cd to CSIT root (other directories might use different tox.ini) +# $ tox +# will execute all checks. +# $ tox -e pylint +# will execute only checks defined in "pylint" tox environment. + +[tox] +envlist = linelength, autogen, pylint +# The following is needed as tox requires setup.py by default. +skipsdist = true + +# TODO: Tox prints various warnings. +# Figure out what they are about and fix them. + +[testenv:pylint] +deps = + pylint==1.5.4 + -r ./requirements.txt +whitelist_externals = /bin/bash +setenv = PYTHONPATH = {toxinidir} +# Run pylint, but hide its return value until python warnings are cleared. +commands = bash -c "bash resources/libraries/bash/entry/check/pylint.sh || true" + +# TODO: See FIXME in https://gerrit.fd.io/r/16423 + +[testenv:linelength] +whitelist_externals = /bin/bash +# Fix all transgressions and remove the " || true" workaround. +commands = bash -c "bash resources/libraries/bash/entry/check/line.sh || true" + +# It would be possible to add a check which fails +# if number of long lines increases (from parent commit value), +# and have it voting. +# But that would basically prevent us from adding new suites righ now. :( + +[testenv:autogen] +whitelist_externals = /bin/bash +setenv = PYTHONPATH = {toxinidir} +commands = bash resources/libraries/bash/entry/check/autogen.sh + +# TODO: Migrate current docs check here. +# TODO: Create license checker. +# TODO: Create voting "pylint violations should not increase" checker. +# TODO: Create voting "linelength violations should not increase" checker. +# TODO: Create Robot suite Documentation checker (backslash if not next mark). +# TODO: Create .yaml specific checker, so people can override long line check. +# TODO: Create .rst specific checker, if there is one allowing +# to override line check. +# TODO: You get the idea, replace line check with something smarter +# wherever possible.
\ No newline at end of file |