2 Copyright (c) 2019 Cisco and/or its affiliates.
3 Licensed under the Apache License, Version 2.0 (the "License");
4 you may not use this file except in compliance with the License.
5 You may obtain a copy of the License at:
7 http://www.apache.org/licenses/LICENSE-2.0
9 Unless required by applicable law or agreed to in writing, software
10 distributed under the License is distributed on an "AS IS" BASIS,
11 WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12 See the License for the specific language governing permissions and
13 limitations under the License.
16 The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
17 "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED",
18 "MAY", and "OPTIONAL" in this document are to be interpreted as
19 described in `BCP 14 <https://tools.ietf.org/html/bcp14>`_
20 `[RFC2119] <https://tools.ietf.org/html/rfc2119>`_
21 `[RFC8174] <https://tools.ietf.org/html/rfc8174>`_
22 when, and only when, they appear in all capitals, as shown here.
24 This document SHALL describe guidelines for writing reliable, maintainable,
25 reusable and readable code for CSIT.
30 TODO: List reasons why we need code style document for Bash.
38 Bash files SHOULD NOT be monolithic. Generally, this document
39 considers two types of bash files:
41 + Entry script: Assumed to be called by user,
42 or a script "external" in some way.
44 + Sources bash libraries and calls functions defined there.
46 + Library file: To be sourced by entry scipts, possibly also by other libraries.
48 + Sources other libraries for functions it needs.
50 + Or relies on a related file already having sourced that.
52 + Documentation SHALL imply which case it is.
54 + Defines multiple functions other scripts can call.
59 + Variable expansions MUST be quoted, to prevent word splitting.
61 + This includes special "variables" such as "${1}".
63 + RECOMMENDED even if the value is safe, as in "$?" and "$#".
65 + It is RECOMMENDED to quote strings in general,
66 so text editors can syntax-highlight them.
68 + Even if the string is a numeric value.
70 + Commands and known options can get their own highlight, no need to quote.
72 + Example: You do not need to quote every word of
73 "pip install --upgrade virtualenv".
75 + Code SHALL NOT quote glob characters you need to expand (obviously).
77 + OPTIONALLY do not quote adjacent characters (such as dot or fore-slash),
78 so that syntax highlighting makes them stand out compared to surrounding
81 + Example: cp "logs"/*."log" "."/
83 + TODO: Consider giving examples both for good and bad usage.
85 + Command substitution on right hand side of assignment are safe
88 + Note that command substitution limits the scope for quotes,
89 so it is NOT REQUIRED to escape the quotes in deeper levels.
91 + Both backtics and "dollar round-bracket" provide command substitution.
92 The folowing rules are RECOMMENDED:
94 + For simple constructs, use "dollar round-bracket".
96 + If there are round brackets in the surrounding text, use backticks,
97 as some editor highlighting logic can get confused.
99 + Avoid nested command substitution.
101 + Put intermediate results into local variables,
102 use "|| die" on each step of command substitution.
104 + Code SHOULD NOT be structured in a way where
105 word splitting is intended.
107 + Example: Variable holding string of multiple command lines arguments.
109 + Solution: Array variable should be used in this case.
111 + Expansion MUST use quotes then: "${name[@]}".
113 + Word splitting MAY be used when creating arrays from command substitution.
115 + Code MUST always check the exit code of commands.
117 + Traditionally, error code checking is done either by "set -e"
118 or by appending "|| die" after each command.
119 The first is unreliable, due to many rules affecting "set -e" behavior
120 (see <https://mywiki.wooledge.org/BashFAQ/105>), but "|| die"
121 relies on humans identifying each command, which is also unreliable.
122 When was the last time you checked error code of "echo" command,
125 + Another example: "set -e" in your function has no effect
126 if any ancestor call is done with logical or,
127 for example in "func || code=$?" construct.
129 + As there is no reliable method of error detection, and there are two
130 largely independent unreliable methods, the best what we can do
131 is to apply both. So, code SHOULD explicitly
132 check each command (with "|| die" and similar) AND have "set -e" applied.
134 + Code MUST explicitly check each command, unless the command is well known,
135 and considered safe (such as the aforementioned "echo").
137 + The well known commands MUST still be checked implicitly via "set -e".
139 + See below for specific "set -e" recommendations.
141 + Code SHOULD use "readlink -e" (or "-f" if target does not exist yet)
142 to normalize any path value to absolute path without symlinks.
143 It helps with debugging and identifies malformed paths.
145 + Code SHOULD use such normalized paths for sourcing.
147 + When exiting on a known error, code MUST print a longer, helpful message,
148 in order for the user to fix their situation if possible.
150 + When error happens at an unexpected place, it is RECOMMENDED for the message
151 to be short and generic, instead of speculative.
156 + Code MUST apply "-x" to make debugging easier.
158 + Code MAY temporarily supress such output in order to avoid spam
159 (e.g. in long busy loops), but it is still NOT RECOMMENDED to do so.
161 + Code MUST apply "-e" for early error detection.
163 + But code still SHOULD use "|| die" for most commands,
164 as "-e" has numerous rules and exceptions.
166 + Code MAY apply "+e" temporarily for commands which (possibly nonzero)
167 exit code it interested in.
169 + Code MUST to store "$?" and call "set -e" immediatelly afterwards.
171 + Code MUST NOT use this approach when calling functions.
173 + That is because functions are instructed to apply "set -e" on their own
174 which (when triggered) will exit the whole entry script.
176 + Unless overriden by ERR trap.
177 But code SHOULD NOT set any ERR trap.
179 + If code needs exit code of a function, it is RECOMMENDED to use
180 pattern 'code="0"; called_function || code="${?}"'.
182 + In this case, contributor MUST make sure nothing in the
183 called_function sub-graph relies on "set -e" behavior,
184 because the call being part of "or construct" disables it.
186 + Code MAY append "|| true" for benign commands,
187 when it is clear non-zero exit codes make no difference.
189 + Also in this case, the contributor MUST make sure nothing within
190 the called sub-graph depends on "set -e", as it is disabled.
192 + Code MUST apply "-u" as unset variable is generally a typo, thus an error.
194 + Code MAY temporarily apply "+u" if a command needs that to pass.
196 + Virtualenv activation is the only known example so far.
198 + Code MUST apply "-o pipefail" to make sure "-e" picks errors
199 inside piped construct.
201 + Code MAY use "|| true" inside a pipe construct, in the (inprobable) case
202 when non-zero exit code still results in a meaningful pipe output.
204 + All together: "set -exuo pipefail".
206 + Code MUST put that line near start of every file, so we are sure
207 the options are applied no matter what.
209 + "Near start" means "before any nontrivial code".
211 + Basically only copyright is RECOMMENDED to appear before.
213 + Also code MUST put the line near start of function bodies
214 and subshell invocations.
219 There are (at least) two possibilities how a code from an external file
220 can be executed. Either the file contains a code block to execute
221 on each "source" invocation, or the file just defines functions
222 which have to be called separately.
224 This document considers the "function way" to be better,
225 here are some pros and cons:
229 + The function way takes more space. Files have more lines,
230 and the code in function body is one indent deeper.
232 + It is not easy to create functions for low-level argument manipulation,
233 as "shift" command in the function code does not affect the caller context.
235 + Call sites frequently refer to code two times,
236 when sourcing the definition and when executing the function.
238 + It is not clear when a library can rely on its relative
239 to have performed the sourcing already.
241 + Ideally, each library should detect if it has been sourced already
242 and return early, which takes even more space.
246 + Some code blocks are more useful when used as function,
247 to make call site shorter.
249 + Examples: Trap functions, "die" function.
251 + The "import" part and "function" part usually have different side effects,
252 making the documentation more focused (even if longer overall).
254 + There is zero risk of argument-less invocation picking arguments
257 + This safety feature is the main reason for chosing the "function way".
259 + This allows code blocks to support optional arguments.
263 + Library files MUST be only "source"d. For example if "tox" calls a script,
264 it is an entry script.
266 + Library files (upon sourcing) MUST minimize size effect.
268 + The only permitted side effects MUST by directly related to:
270 + Defining functions (without executing them).
272 + Sourcing sub-library files.
274 + If a bash script indirectly call another bash script,
275 it is not a "source" operation, variables are not shared,
276 so the called script MUST be considered an entry script,
277 even if it implements logic fitting into a single function.
279 + Entry scripts SHOULD avoid duplicating any logic.
281 + Clear duplicated blocks MUST be moved into libraries as functions.
283 + Blocks with low amount of duplication MAY remain in entry scripts.
285 + Usual motives for not creating functions are:
287 + The extracted function would have too much logic for processing
288 arguments (instead of hardcoding values as in entry script).
290 + The arguments needed would be too verbose.
292 + And using "set +x" would take too much vertical space
293 (when compared to entry script implementation).
298 This document describes two kinds of variables: called "local" and "global".
300 TODO: Find better adjectives for the two kinds defined here,
301 if the usual bash meaning makes reader forget other specifics.
302 For example, variable with lowercase name and not marked by "local" builtin,
303 is cosidered "global" from bash point of view, but "local" from this document
308 + Variable name MUST contain only lower case letters, digits and underscores.
310 + Code MUST NOT export local variables.
312 + Code MUST NOT rely on local variables set in different contexts.
314 + Documentation is NOT REQUIRED.
316 + Variable name SHOULD be descriptive enough.
318 + Local variable MUST be initialized before first use.
320 + Code SHOULD have a comment if a reader might have missed
323 + TODO: Agree on level of defensiveness (against local values
324 being influenced by other functions) needed.
325 Possible alternatives / additions to the "always initialize" rule:
327 + Unset local variables when leaving the function.
329 + Explicitly typeset by "local" builtin command.
331 + Require strict naming convention, e.g. function_name__variable_name.
335 + Variable name MUST contain only upper case letters, digits and underscores.
337 + They SHOULD NOT be exported, unless external commands need them
340 + TODO: Propose a strict naming convention, or a central document
341 of all used global variables, to prevent contributors
342 from causing variable name conflicts.
344 + Code MUST document if a function (or its inner call)
345 reads a global variable.
347 + Code MUST document if a function (or its inner call)
348 sets or rewrites a global variable.
350 + If a function "wants to return a value", it SHOULD be implemented
351 as the function setting (or rewriting) a global variable,
352 and the call sites reading that variable.
354 + If a function "wants to accept an argument", it IS RECOMMENDED
355 to be implemented as the call sites setting or rewriting global variables,
356 and the function reading that variables.
357 But see below for direct arguments.
359 + Code MUST use curly brackets when referencing variables,
360 e.g. "${my_variable}".
362 + It makes related constructs (such as ${name:-default}) less surprising.
364 + It looks more similar to Robot Framework variables (which is good).
369 Bash scripts and functions MAY accept arguments, named "${1}", "${2}" and so on.
370 As a whole available via "$@".
371 You MAY use "shift" command to consume an argument.
376 Functions never have access to parent arguments, but they can read and write
377 variables set or read by parent contexts.
379 Arguments or variables
380 ``````````````````````
382 + Both arguments and global variables MAY act as an input.
384 + In general, if the caller is likely to supply the value already placed
385 in a global variable of known name, it is RECOMMENDED
386 to use that global variable.
388 + Construct "${NAME:-value}" can be used equally well for arguments,
389 so default values are possible for both input methods.
391 + Arguments are positional, so there are restrictions on which input
394 + Functions SHOULD either look at arguments (possibly also
395 reading global variables to use as defaults), or look at variables only.
397 + Code MUST NOT rely on "${0}", it SHOULD use "${BASH_SOURCE[0]}" instead
398 (and apply "readlink -e") to get the current block location.
400 + For entry scripts, it is RECOMMENDED to use standard parsing capabilities.
402 + For most Linux distros, "getopt" is RECOMMENDED.
404 Working directory handling
405 ~~~~~~~~~~~~~~~~~~~~~~~~~~
407 + Functions SHOULD act correctly without neither assuming
408 what the currect working directory is, nor changing it.
410 + That is why global variables and arguments SHOULD contain
411 (normalized) full paths.
413 + Motivation: Different call sites MAY rely on different working directories.
415 + A function MAY return (also with nonzero exit code) when working directory
418 + In this case the function documentation MUST clearly state where (and when)
419 is the working directory changed.
421 + Exception: Functions with undocumented exit code.
423 + Those functions MUST return nonzero code only on "set -e" or "die".
425 + Note that both "set -e" and "die" by default result in exit of the whole
426 entry script, but the caller MAY have altered that behavior
427 (by registering ERR trap, or redefining die function).
429 + Any callers which use "set +e" or "|| true" MUST make sure
430 their (and their caller ancestors') assumption on working directory
433 + Such callers SHOULD do that by restoring the original working directory
434 either in their code,
436 + or contributors SHOULD do such restoration in the function code,
437 (see below) if that is more convenient.
439 + Motivation: Callers MAY rely on this side effect to simplify their logic.
441 + A function MAY assume a particular directory is already set
442 as the working directory (to save space).
444 + In this case function documentation MUST clearly state what the assumed
445 working directory is.
447 + Motivation: Callers MAY call several functions with common
448 directory of interest.
450 + Example: Several dowload actions to execute in sequence,
451 implemented as functions assuming ${DOWNLOAD_DIR}
452 is the working directory.
454 + A function MAY change the working directory transiently,
455 before restoring it back before return.
457 + Such functions SHOULD use command "pushd" to change the working directory.
459 + Such functions SHOULD use "trap 'trap - RETURN; popd' RETURN"
460 imediately after the pushd.
462 + In that case, the "trap - RETURN" part MUST be included,
463 to restore any trap set by ancestor.
465 + Functions MAY call "trap - RETURN; popd" exlicitly.
467 + Such functions MUST NOT call another pushd (before an explicit popd),
468 as traps do not stack within a function.
470 + If entry scripts also use traps to restore working directory (or other state),
471 they SHOULD use EXIT traps instead.
473 + That is because "exit" command, as well as the default behavior
474 of "die" or "set -e" cause direct exit (without skipping function returns).
479 + In general, code SHOULD follow reasoning similar to how pylint
480 limits code complexity.
482 + It is RECOMMENDED to have functions somewhat simpler than Python functions,
483 as Bash is generally more verbose and less readable.
485 + If code contains comments in order to partition a block
486 into sub-blocks, the sub-blocks SHOULD be moved into separate functions.
488 + Unless the sub-blocks are essentially one-liners,
489 not readable just because external commands do not have
490 obvious enough parameters. Use common sense.
495 + The library path and filename is visible from source sites. It SHOULD be
496 descriptive enough, so reader do not need to look inside to determine
497 how and why is the sourced file used.
499 + If code would use several functions with similar names,
500 it is RECOMMENDED to create a (well-named) sub-library for them.
502 + Code MAY create deep library trees if needed, it SHOULD store
503 common path prefixes into global variables to make sourcing easier.
505 + Contributors, look at other files in the subdirectory. You SHOULD
506 improve their filenames when adding-removing other filenames.
508 + Library files SHOULD NOT have executable flag set.
510 + Library files SHOULD have an extension .sh (or perhaps .bash).
512 + It is RECOMMENDED for entry scripts to also have executable flag unset
513 and have .sh extension.
515 + Each entry script MUST start with a shebang.
517 + "#!/bin/usr/env bash" is RECOMMENDED.
519 + Code SHOULD put an empty line after shebang.
521 + Library files SHOULD NOT contain a shebang, as "source" is the primary
522 method to include them.
524 + Following that, there SHOULD be a block of comment lines with copyright.
526 + It is a boilerplate, but human eyes are good at ignoring it.
528 + Overhead for git is also negligible.
530 + Following that, there MUST be "set -exuo pipefail".
532 + It acts as an anchor for humans to start paying attention.
534 Then it depends on script type.
536 Library documentation
537 `````````````````````
539 + Following "set -exuo pipefail" SHALL come the "import part" documentation.
541 + Then SHALL be the import code
542 ("source" commands and a bare minimum they need).
544 + Then SHALL be the function definitions, and inside:
546 + The body SHALL sart with the function documentation explaining API contract.
547 Similar to Robot [Documentation] or Python function-level docstring.
551 + Following that SHALL be various top-level TODOs and FIXMEs.
553 + TODO: Document (in an appropriate place) how TODOs differ from FIXMEs.
555 + "set -exuo pipefail" SHALL be the first executable line
556 in the function body, except functions which legitimely need
557 different flags. Those SHALL also start with appropriate "set" command(s).
559 + Lines containing code itself SHALL follow.
561 + "Code itself" SHALL include comment lines
562 explaining any non-obvious logic.
564 + There SHALL be two empty lines between function definitions.
566 More details on function documentation:
568 Generally, code SHOULD use comments to explain anything
569 not obvious from the funtion name.
571 + Function documentation SHOULD start with short description of function
572 operation or motivation, but only if not obvious from function name.
574 + Documentation SHOULD continue with listing any non-obvious side effect:
576 + Documentation MUST list all read global variables.
578 + Documentation SHOULD include descriptions of semantics
579 of global variable values.
580 It is RECOMMENDED to mention which function is supposed to set them.
582 + The "include descriptions" part SHOULD apply to other items as well.
584 + Documentation MUST list all global variables set, unset, reset,
585 or otherwise updated.
587 + It is RECOMMENDED to list all hardcoded values used in code.
589 + Not critical, but can hint at future improvements.
591 + Documentation MUST list all files or directories read
592 (so caller can make sure their content is ready).
594 + Documentation MUST list all files or directories updated
595 (created, deleted, emptied, otherwise edited).
597 + Documentation SHOULD list all functions called (so reader can look them up).
599 + Documentation SHOULD mention where are the functions defined,
600 if not in the current file.
602 + Documentation SHOULD list all external commands executed.
604 + Because their behavior can change "out of bounds", meaning
605 the contributor changing the implementation of the extrenal command
606 can be unaware of this particular function interested in its side effects.
608 + Documentation SHOULD explain exit code (coming from
609 the last executed command).
611 + Usually, most functions SHOULD be "pass or die",
612 but some callers MAY be interested in nonzero exit codes
613 without using global variables to store them.
615 + Remember, "exit 1" ends not only the function, but all scripts
616 in the source chain, so code MUST NOT use it for other purposes.
618 + Code SHOULD call "die" function instead. This way the caller can
619 redefine that function, if there is a good reason for not exiting
622 + TODO: Programs installed, services started, URLs downloaded from, ...
624 + TODO: Add more items when you spot them.
626 + TODO: Is the current order recommended?
628 Entry script documentation
629 ``````````````````````````
631 + After "set -exuo pipefail", high-level description SHALL come.
633 + Then TODOs and FIXMEs SHALL be placed (if any).
635 + Entry scripts are rarely reused, so detailed side effects
636 are OPTIONAL to document.
638 + But code SHOULD document the primary side effects.
640 + Then SHALL come few commented lines to import the library with "die" function.
642 + Then block of "source" commands for sourcing other libraries needed SHALL be.
644 + In alphabetical order, any "special" library SHOULD be
645 in the previous block (for "die").
647 + Then block os commands processing arguments SHOULD be (if needed).
649 + Then SHALL come block of function calls (with parameters as needed).
651 Other general recommendations
652 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
654 + Code SHOULD NOT not repeat itself, even in documentation:
656 + For hardcoded values, a general description SHOULD be written
657 (instead of copying the value), so when someone edits the value
658 in the code, the description still applies.
660 + If affected directory name is taken from a global variable,
661 documentation MAY distribute the directory description
664 + If most of side effects come from an inner call,
665 documentation MAY point the reader to the documentation
666 of the called function (instead of listing all the side effects).
668 + TODO: Composite functions can have large effects. Should we require
669 intermediate functions to actively hide them whenever possible?
671 + But documentation SHOULD repeat it if the information crosses functions.
673 + Item description MUST NOT be skipped just because the reader
674 should have read parent/child documentation already.
676 + Frequently it is RECOMMENDED to copy&paste item descriptions
679 + But sometimes it is RECOMMENDED to vary the descriptions. For example:
681 + A global variable setter MAY document how does it figure out the value
682 (without caring about what it will be used for by other functions).
684 + A global variable reader MAY document how does it use the value
685 (without caring about how has it been figured out by the setter).
687 + When possible, Bash code SHOULD be made to look like Python
688 (or Robot Framework). Those are three primary languages CSIT code relies on,
689 so it is nicer for the readers to see similar expressions when possible.
692 + Code MUST use indentation, 1 level is 4 spaces.
694 + Code SHOULD use "if" instead of "&&" constructs.
696 + For comparisons, code SHOULD use operators such as "!=" (needs "[[").
698 + Code MUST NOT use more than 80 characters per line.
700 + If long external command invocations are needed,
701 code SHOULD use array variables to shorten them.
703 + If long strings (or arrays) are needed, code SHOULD use "+=" operator
704 to grow the value over multiple lines.
706 + If "|| die" does not fit with the command, code SHOULD use curly braces:
708 + Current line has "|| {",
710 + Next line has the die commands (indented one level deeper),
712 + Final line closes with "}" at original intent level.
714 + TODO: Recommend what to do with other constructs.
716 + For example multiple piped commands.
718 + No, "eval" is too unsafe to use.