docs/bash_code_style.rst

   1 ..
   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:
   6 ..
   7        http://www.apache.org/licenses/LICENSE-2.0
   8 ..
   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.
  14
  15
  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.
  23
  24 This document SHALL describe guidelines for writing reliable, maintainable,
  25 reusable and readable code for CSIT.
  26
  27 Motivation
  28 ^^^^^^^^^^
  29
  30 TODO: List reasons why we need code style document for Bash.
  31
  32 Proposed style
  33 ^^^^^^^^^^^^^^
  34
  35 File types
  36 ~~~~~~~~~~
  37
  38 Bash files SHOULD NOT be monolithic. Generally, this document
  39 considers two types of bash files:
  40
  41 + Entry script: Assumed to be called by user,
  42   or a script "external" in some way.
  43
  44   + Sources bash libraries and calls functions defined there.
  45
  46 + Library file: To be sourced by entry scipts, possibly also by other libraries.
  47
  48   + Sources other libraries for functions it needs.
  49
  50     + Or relies on a related file already having sourced that.
  51
  52     + Documentation SHALL imply which case it is.
  53
  54   + Defines multiple functions other scripts can call.
  55
  56 Safety
  57 ~~~~~~
  58
  59 + Variable expansions MUST be quoted, to prevent word splitting.
  60
  61   + This includes special "variables" such as "${1}".
  62
  63     + RECOMMENDED even if the value is safe, as in "$?" and "$#".
  64
  65   + It is RECOMMENDED to quote strings in general,
  66     so text editors can syntax-highlight them.
  67
  68     + Even if the string is a numeric value.
  69
  70     + Commands and known options can get their own highlight, no need to quote.
  71
  72       + Example: You do not need to quote every word of
  73         "pip install --upgrade virtualenv".
  74
  75   + Code SHALL NOT quote glob characters you need to expand (obviously).
  76
  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
  79       ordinary strings.
  80
  81     + Example: cp "logs"/*."log" "."/
  82
  83     + TODO: Consider giving examples both for good and bad usage.
  84
  85   + Command substitution on right hand side of assignment should be safe
  86     without quotes.
  87
  88     + But still, quotes are RECOMMENDED, unless line length is a concern.
  89
  90     + Note that command substitution limits the scope for quotes,
  91       so it is NOT REQUIRED to escape the quotes in deeper levels.
  92
  93     + TODO: Do we prefer backtics, or "dollar round-bracket"?
  94
  95       + Backticks are more readable, especially when there are
  96         round brackets in the surrounding text.
  97
  98       + Backticks do not allow nested command substitution.
  99
 100         + But we might want to avoid nested command substitution anyway?
 101
 102   + Code SHOULD NOT be structured in a way where
 103     word splitting is intended.
 104
 105     + Example: Variable holding string of multiple command lines arguments.
 106
 107     + Solution: Array variable should be used in this case.
 108
 109     + Expansion MUST use quotes then: "${name[@]}".
 110
 111     + Word splitting MAY be used when creating arrays from command substitution.
 112
 113 + Code MUST always check the exit code of commands.
 114
 115   + Traditionally, error code checking is done either by "set -e"
 116     or by appending "|| die" after each command.
 117     The first is unreliable, due to many rules affecting "set -e" behavior
 118     (see <https://mywiki.wooledge.org/BashFAQ/105>), but "|| die"
 119     relies on humans identifying each command, which is also unreliable.
 120     When was the last time you checked error code of "echo" command,
 121     for example?
 122
 123   + As there is no reliable method of error detection, and there are two
 124     largely independent unreliable methods, the best what we can do
 125     is to apply both. So, code SHOULD explicitly
 126     check each command (with "|| die" and similar) AND have "set -e" applied.
 127
 128   + Code MUST explicitly check each command, unless the command is well known,
 129     and considered safe (such as the aforementioned "echo").
 130
 131     + The well known commands MUST still be checked implicitly via "set -e".
 132
 133   + See below for specific "set -e" recommendations.
 134
 135 + Code SHOULD use "readlink -e" (or "-f" if target does not exist yet)
 136   to normalize any path value to absolute path without symlinks.
 137   It helps with debugging and identifies malformed paths.
 138
 139 + Code SHOULD use such normalized paths for sourcing.
 140
 141 + When exiting on a known error, code MUST print a longer, helpful message,
 142   in order for the user to fix their situation if possible.
 143
 144 + When error happens at an unexpected place, it is RECOMMENDED for the message
 145   to be short and generic, instead of speculative.
 146
 147 Bash options
 148 ~~~~~~~~~~~~
 149
 150 + Code MUST apply "-x" to make debugging easier.
 151
 152   + Code MAY temporarily supress such output in order to avoid spam
 153     (e.g. in long busy loops), but it is still NOT RECOMMENDED to do so.
 154
 155 + Code MUST apply "-e" for early error detection.
 156
 157   + But code still SHOULD use "|| die" for most commands,
 158     as "-e" has numerous rules and exceptions.
 159
 160   + Code MAY apply "+e" temporarily for commands which (possibly nonzero)
 161     exit code it interested in.
 162
 163     + Code MUST to store "$?" and call "set -e" immediatelly afterwards.
 164
 165   + Code MAY append "|| true" for benign commands,
 166     when it is clear non-zero exit codes make no difference.
 167
 168 + Code MUST apply "-u" as unset variable is generally a typo, thus an error.
 169
 170   + Code MAY temporarily apply "+u" if a command needs that to pass.
 171
 172     + Virtualenv activation is the only known example so far.
 173
 174 + Code MUST apply "-o pipefail" to make sure "-e" picks errors
 175   inside piped construct.
 176
 177   + Code MAY use "|| true" inside a pipe construct, in the (inprobable) case
 178     when non-zero exit code still results in a meaningful pipe output.
 179
 180 + All together: "set -exuo pipefail".
 181
 182   + Code MUST put that line near start of every file, so we are sure
 183     the options are applied no matter what.
 184
 185     + "Near start" means "before any nontrivial code".
 186
 187     + Basically only copyright is RECOMMENDED to appear before.
 188
 189   + Also code MUST put the line near start of function bodies
 190     and subshell invocations.
 191
 192 Functions
 193 ~~~~~~~~~
 194
 195 There are (at least) two possibilities how a code from an external file
 196 can be executed. Either the file contains a code block to execute
 197 on each "source" invocation, or the file just defines functions
 198 which have to be called separately.
 199
 200 This document considers the "function way" to be better,
 201 here are some pros and cons:
 202
 203 + Cons:
 204
 205   + The function way takes more space. Files have more lines,
 206     and the code in function body is one indent deeper.
 207
 208   + It is not easy to create functions for low-level argument manipulation,
 209     as "shift" command in the function code does not affect the caller context.
 210
 211   + Call sites frequently refer to code two times,
 212     when sourcing the definition and when executing the function.
 213
 214   + It is not clear when a library can rely on its relative
 215     to have performed the sourcing already.
 216
 217   + Ideally, each library should detect if it has been sourced already
 218     and return early, which takes even more space.
 219
 220 + Pros:
 221
 222   + Some code blocks are more useful when used as function,
 223     to make call site shorter.
 224
 225     + Examples: Trap functions, "die" function.
 226
 227   + The "import" part and "function" part usually have different side effects,
 228     making the documentation more focused (even if longer overall).
 229
 230   + There is zero risk of argument-less invocation picking arguments
 231     from parent context.
 232
 233     + This safety feature is the main reason for chosing the "function way".
 234
 235     + This allows code blocks to support optional arguments.
 236
 237 + Rules:
 238
 239   + Library files MUST be only "source"d. For example if "tox" calls a script,
 240     it is an entry script.
 241
 242   + Library files (upon sourcing) MUST minimize size effect.
 243
 244     + The only permitted side effects MUST by directly related to:
 245
 246       + Defining functions (without executing them).
 247
 248       + Sourcing sub-library files.
 249
 250   + If a bash script indirectly call another bash script,
 251     it is not a "source" operation, variables are not shared,
 252     so the called script MUST be considered an entry script,
 253     even if it implements logic fitting into a single function.
 254
 255   + Entry scripts SHOULD avoid duplicating any logic.
 256
 257     + Clear duplicated blocks MUST be moved into libraries as functions.
 258
 259     + Blocks with low amount of duplication MAY remain in entry scripts.
 260
 261     + Usual motives for not creating functions are:
 262
 263       + The extracted function would have too much logic for processing
 264         arguments (instead of hardcoding values as in entry script).
 265
 266       + The arguments needed would be too verbose.
 267
 268         + And using "set +x" would take too much vertical space
 269           (when compared to entry script implementation).
 270
 271 Variables
 272 ~~~~~~~~~
 273
 274 This document describes two kinds of variables: called "local" and "global".
 275
 276 TODO: Find better adjectives for the two kinds defined here,
 277 if the usual bash meaning makes reader forget other specifics.
 278 For example, variable with lowercase name and not marked by "local" builtin,
 279 is cosidered "global" from bash point of view, but "local" from this document
 280 point of view.
 281
 282 + Local variables:
 283
 284   + Variable name MUST contain only lower case letters, digits and underscores.
 285
 286   + Code MUST NOT export local variables.
 287
 288   + Code MUST NOT rely on local variables set in different contexts.
 289
 290   + Documentation is NOT REQUIRED.
 291
 292     + Variable name SHOULD be descriptive enough.
 293
 294   + Local variable MUST be initialized before first use.
 295
 296     + Code SHOULD have a comment if a reader might have missed
 297       the initialization.
 298
 299   + TODO: Agree on level of defensiveness (against local values
 300     being influenced by other functions) needed.
 301     Possible alternatives / additions to the "always initialize" rule:
 302
 303     + Unset local variables when leaving the function.
 304
 305     + Explicitly typeset by "local" builtin command.
 306
 307     + Require strict naming convention, e.g. function_name__variable_name.
 308
 309 + Global variables:
 310
 311   + Variable name MUST contain only upper case letters, digits and underscores.
 312
 313   + They SHOULD NOT be exported, unless external commands need them
 314     (e.g. PYTHONPATH).
 315
 316   + TODO: Propose a strict naming convention, or a central document
 317     of all used global variables, to prevent contributors
 318     from causing variable name conflicts.
 319
 320   + Code MUST document if a function (or its inner call)
 321     reads a global variable.
 322
 323   + Code MUST document if a function (or its inner call)
 324     sets or rewrites a global variable.
 325
 326   + If a function "wants to return a value", it SHOULD be implemented
 327     as the function setting (or rewriting) a global variable,
 328     and the call sites reading that variable.
 329
 330   + If a function "wants to accept an argument", it IS RECOMMENDED
 331     to be implemented as the call sites setting or rewriting global variables,
 332     and the function reading that variables.
 333     But see below for direct arguments.
 334
 335 + Code MUST use curly brackets when referencing variables,
 336   e.g. "${my_variable}".
 337
 338   + It makes related constructs (such as ${name:-default}) less surprising.
 339
 340   + It looks more similar to Robot Framework variables (which is good).
 341
 342 Arguments
 343 ~~~~~~~~~
 344
 345 Bash scripts and functions MAY accept arguments, named "${1}", "${2}" and so on.
 346 As a whole available via "$@".
 347 You MAY use "shift" command to consume an argument.
 348
 349 Contexts
 350 ````````
 351
 352 Functions never have access to parent arguments, but they can read and write
 353 variables set or read by parent contexts.
 354
 355 Arguments or variables
 356 ``````````````````````
 357
 358 + Both arguments and global variables MAY act as an input.
 359
 360 + In general, if the caller is likely to supply the value already placed
 361   in a global variable of known name, it is RECOMMENDED
 362   to use that global variable.
 363
 364 + Construct "${NAME:-value}" can be used equally well for arguments,
 365   so default values are possible for both input methods.
 366
 367 + Arguments are positional, so there are restrictions on which input
 368   is optional.
 369
 370 + Functions SHOULD either look at arguments (possibly also
 371   reading global variables to use as defaults), or look at variables only.
 372
 373 + Code MUST NOT rely on "${0}", it SHOULD use "${BASH_SOURCE[0]}" instead
 374   (and apply "readlink -e") to get the current block location.
 375
 376 + For entry scripts, it is RECOMMENDED to use standard parsing capabilities.
 377
 378   + For most Linux distros, "getopt" is RECOMMENDED.
 379
 380 Function size
 381 ~~~~~~~~~~~~~
 382
 383 + In general, code SHOULD follow reasoning similar to how pylint
 384   limits code complexity.
 385
 386 + It is RECOMMENDED to have functions somewhat simpler than Python functions,
 387   as Bash is generally more verbose and less readable.
 388
 389 + If code contains comments in order to partition a block
 390   into sub-blocks, the sub-blocks SHOULD be moved into separate functions.
 391
 392   + Unless the sub-blocks are essentially one-liners,
 393     not readable just because external commands do not have
 394     obvious enough parameters. Use common sense.
 395
 396 Documentation
 397 ~~~~~~~~~~~~~
 398
 399 + The library path and filename is visible from source sites. It SHOULD be
 400   descriptive enough, so reader do not need to look inside to determine
 401   how and why is the sourced file used.
 402
 403   + If code would use several functions with similar names,
 404     it is RECOMMENDED to create a (well-named) sub-library for them.
 405
 406   + Code MAY create deep library trees if needed, it SHOULD store
 407     common path prefixes into global variables to make sourcing easier.
 408
 409   + Contributors, look at other files in the subdirectory. You SHOULD
 410     improve their filenames when adding-removing other filenames.
 411
 412   + Library files SHOULD NOT have executable flag set.
 413
 414   + Library files SHOULD have an extension .sh (or perhaps .bash).
 415
 416   + It is RECOMMENDED for entry scripts to also have executable flag unset
 417     and have .sh extension.
 418
 419 + Each entry script MUST start with a shebang.
 420
 421   + "#!/bin/usr/env bash" is RECOMMENDED.
 422
 423   + Code SHOULD put an empty line after shebang.
 424
 425   + Library files SHOULD NOT contain a shebang, as "source" is the primary
 426     method to include them.
 427
 428 + Following that, there SHOULD be a block of comment lines with copyright.
 429
 430   + It is a boilerplate, but human eyes are good at ignoring it.
 431
 432   + Overhead for git is also negligible.
 433
 434 + Following that, there MUST be "set -exuo pipefail".
 435
 436   + It acts as an anchor for humans to start paying attention.
 437
 438 Then it depends on script type.
 439
 440 Library documentation
 441 `````````````````````
 442
 443 + Following "set -exuo pipefail" SHALL come the "import part" documentation.
 444
 445 + Then SHALL be the import code
 446   ("source" commands and a bare minimum they need).
 447
 448 + Then SHALL be the function definitions, and inside:
 449
 450   + "set -exuo pipefail" again.
 451
 452   + Following that SHALL be the function documentation explaining API contract.
 453     Similar to Robot [Documentation] or Python function-level docstring.
 454
 455     + See below.
 456
 457   + Following that SHALL be varius TODOs, FIXMEs and code itself.
 458
 459     + "Code itself" SHALL include comment lines
 460       explaining any non-obvious logic.
 461
 462     + TODO: Document (in an appropriate place) how TODOs differ from FIXMEs.
 463
 464   + There SHALL be two empty lines before next function definition.
 465
 466 More details on function documentation:
 467
 468 Generally, code SHOULD use comments to explain anything
 469 not obvious from the funtion name.
 470
 471 + Function documentation SHOULD start with short description of function
 472   operation or motivation, but only if not obvious from function name.
 473
 474 + Documentation SHOULD continue with listing any non-obvious side effect:
 475
 476   + Documentation MUST list all read global variables.
 477
 478     + Documentation SHOULD include descriptions of semantics
 479       of global variable values.
 480       It is RECOMMENDED to mention which function is supposed to set them.
 481
 482     + The "include descriptions" part SHOULD apply to other items as well.
 483
 484   + Documentation MUST list all global variables set, unset, reset,
 485     or otherwise updated.
 486
 487   + It is RECOMMENDED to list all hardcoded values used in code.
 488
 489     + Not critical, but can hint at future improvements.
 490
 491   + Documentation MUST list all files or directories read
 492     (so caller can make sure their content is ready).
 493
 494   + Documentation MUST list all files or directories updated
 495     (created, deleted, emptied, otherwise edited).
 496
 497   + Documentation SHOULD list all functions called (so reader can look them up).
 498
 499     + Documentation SHOULD mention where are the functions defined,
 500       if not in the current file.
 501
 502   + Documentation SHOULD list all external commands executed.
 503
 504     + Because their behavior can change "out of bounds", meaning
 505       the contributor changing the implementation of the extrenal command
 506       can be unaware of this particular function interested in its side effects.
 507
 508   + Documentation SHOULD explain exit code (coming from
 509     the last executed command).
 510
 511     + Usually, most functions SHOULD be "pass or die",
 512       but some callers MAY be interested in nonzero exit codes
 513       without using global variables to store them.
 514
 515     + Remember, "exit 1" ends not only the function, but all scripts
 516       in the source chain, so code MUST NOT use it for other purposes.
 517
 518       + Code SHOULD call "die" function instead. This way the caller can
 519         redefine that function, if there is a good reason for not exiting
 520         on function failure.
 521
 522   + TODO: Programs installed, services started, URLs downloaded from, ...
 523
 524   + TODO: Add more items when you spot them.
 525
 526   + TODO: Is the current order recommended?
 527
 528 Entry script documentation
 529 ``````````````````````````
 530
 531 + After "set -exuo pipefail", high-level description SHALL come.
 532
 533   + Then TODOs and FIXMEs SHALL be placed (if any).
 534
 535   + Entry scripts are rarely reused, so detailed side effects
 536     are OPTIONAL to document.
 537
 538   + But code SHOULD document the primary side effects.
 539
 540 + Then SHALL come few commented lines to import the library with "die" function.
 541
 542 + Then block of "source" commands for sourcing other libraries needed SHALL be.
 543
 544   + In alphabetical order, any "special" library SHOULD be
 545     in the previous block (for "die").
 546
 547 + Then block os commands processing arguments SHOULD be (if needed).
 548
 549 + Then SHALL come block of function calls (with parameters as needed).
 550
 551 Other general recommendations
 552 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 553
 554 + Code SHOULD NOT not repeat itself, even in documentation:
 555
 556   + For hardcoded values, a general description SHOULD be written
 557     (instead of copying the value), so when someone edits the value
 558     in the code, the description still applies.
 559
 560   + If affected directory name is taken from a global variable,
 561     documentation MAY distribute the directory description
 562     over the two items.
 563
 564   + If most of side effects come from an inner call,
 565     documentation MAY point the reader to the documentation
 566     of the called function (instead of listing all the side effects).
 567
 568     + TODO: Composite functions can have large effects. Should we require
 569       intermediate functions to actively hide them whenever possible?
 570
 571 + But documentation SHOULD repeat it if the information crosses functions.
 572
 573   + Item description MUST NOT be skipped just because the reader
 574     should have read parent/child documentation already.
 575
 576   + Frequently it is RECOMMENDED to copy&paste item descriptions
 577     between functions.
 578
 579   + But sometimes it is RECOMMENDED to vary the descriptions. For example:
 580
 581     + A global variable setter MAY document how does it figure out the value
 582       (without caring about what it will be used for by other functions).
 583
 584     + A global variable reader MAY document how does it use the value
 585       (without caring about how has it been figured out by the setter).
 586
 587 + When possible, Bash code SHOULD be made to look like Python
 588   (or Robot Framework). Those are three primary languages CSIT code relies on,
 589   so it is nicer for the readers to see similar expressions when possible.
 590   Examples:
 591
 592   + Code MUST use indentation, 1 level is 4 spaces.
 593
 594   + Code SHOULD use "if" instead of "&&" constructs.
 595
 596   + For comparisons, code SHOULD use operators such as "!=" (needs "[[").
 597
 598 + Code MUST NOT use more than 80 characters per line.
 599
 600   + If long external command invocations are needed,
 601     code SHOULD use array variables to shorten them.
 602
 603   + If long strings (or arrays) are needed, code SHOULD use "+=" operator
 604     to grow the value over multiple lines.
 605
 606   + If "|| die" does not fit with the command, code SHOULD use curly braces:
 607
 608     + Current line has "|| {",
 609
 610     + Next line has the die commands (indented one level deeper),
 611
 612     + Final line closes with "}" at original intent level.
 613
 614   + TODO: Recommend what to do with other constructs.
 615
 616     + For example multiple piped commands.
 617
 618     + No, "eval" is too unsafe to use.