diff options
Diffstat (limited to 'HACKING')
-rw-r--r-- | HACKING/build-system.md | 24 | ||||
-rw-r--r-- | HACKING/code-quality.md | 135 | ||||
-rw-r--r-- | HACKING/code-style.md | 62 | ||||
-rw-r--r-- | HACKING/contributing.md | 26 | ||||
-rw-r--r-- | HACKING/licensing.md | 19 | ||||
-rw-r--r-- | HACKING/testing.md | 18 |
6 files changed, 284 insertions, 0 deletions
diff --git a/HACKING/build-system.md b/HACKING/build-system.md new file mode 100644 index 0000000..93770cc --- /dev/null +++ b/HACKING/build-system.md @@ -0,0 +1,24 @@ +The build system is built around an automake-like system for GNU Make +that I wrote. It is documented in `automake.txt`. It provides all of +the standard targets and such; you tell it what to do by setting a +series of `am_whatever` variables. I'm just going to call it +"automake" here. + +I also hook into non-exposed parts of automake with a couple of +`_am_whatever` variables. Hopefully I will come up with good ways to +expose the needed functionality in the future. + +There are a couple of variables that get automatically set from git. +This happens by `include`ing a hidden makefile that sets them; if +`$(topsrcdir)/.git` exists, it knows to use git to generate it; +otherwise it expects the file to exist: + +| variable | file | +|--------------------+----------------------------------------| +| srcfiles | $(srcdir)/.srcfiles.mk | +| LIBRETOOLS_VERSION | $(topsrcdir)/.srcversion-libretools.mk | +| LIBRETOOLS_COMMIT | $(topsrcdir)/.srcversion-libretools.mk | +| DEVTOOLS_VERSION | $(topsrcdir)/.srcversion-devtools.mk | +| DEVTOOLS_COMMIT | $(topsrcdir)/.srcversion-devtools.mk | + +`srcfiles` basically becomes `am_src_files`. diff --git a/HACKING/code-quality.md b/HACKING/code-quality.md new file mode 100644 index 0000000..8efd561 --- /dev/null +++ b/HACKING/code-quality.md @@ -0,0 +1,135 @@ +Code content +============ + +Be aware of the `librelib(7)` library suite, which lives `src/lib`. +It is a suite of Bash libraries that will help you out. Most of the +people looking at the libretools code are familiar with the `messages` +part of it, which actually contains a bunch of utility routines, not +just message printing. There is also a library for dealing with +`blacklist.txt`, and one for loading configuration files and +PKGBUILDs. These are common tasks, but are tricky to handle +consistently--the libraries are there to make things easier. Take a +look at the man pages. + +Message printing: All of the message printing routines, except for +`term_title` and `flag`, take printf-type arguments. Take advantage +of that; don't use string interpolation (don't do `"foo ${var} +bar"`). The reason for this is that if you don't do string +interpolation, messages can be automatically internationalized. +(Internationalization is incomplete at the momement) + +Message printing: The in `--help`/`-h` text, use `print` to print +lines that should not wrap, `echo` to print blank lines, `prose` to +print paragraphs, `bullet` to print bullet points, and `flag` to print +option flags. The text should follow this general format: + + print "Usage: %s [OPTIONS] VARS_ARE_UNDERSCORE_AND_CAPITAL" "${program_name}" + print "One line description of program, no period" + echo + prose "More details. This is a paragraph." + echo + print "Options:" + flag "-h" "Show this message" + +In the "Usage:" line, use printf `%s` and the value `"${0##*/}"` to +determine the program name at runtime. + +There used to be guidelines for how to align the option flags and +descriptions, but now the `flag` command exists takes care of it for +you. Yay for things being easier! + +When using `set -u`, `set -e`, or `trap`, you should also use `set -E` +to have the error handling be passed down to subshells. + +Feel free to use `set -e` (fail on error), but be careful of the +caveats (there are a bunch of them); don't assume all errors are +checked because of it. + +Use `set -u` if you can; it makes using an unset variable an error. + - If a variable not being set is valid (perhaps a configuration + option), use `${var:-}` when accessing it to suppress the error. + - An empty array counts as unset, so if you have an array that may be + empty, use `${var+"${var[@]}"}` (don't put quotes around the outer + pair of braces) to only access it if it's non-empty. + - The reason for this is that a normal string variable is basically + an array with length=1; an unset variable looks like an array + with length=0. Weird stuff. + +In the shebang, use `#!/usr/bin/env bash`. This allows us to not +hardcode the location of bash (I'm not sure why this is useful for +something distro-dependent like libretools, but fauno seems to have a +use-case for it). + +In the shebang, don't pass flags to bash, besides breaking `env` +(above), it means people will make mistakes when debugging, and +running things with `bash FILENAME`. Instead, use `set` to adjust the +flags inside of the program. + +Obey `$TMPDIR`. It's usually as easy as passing `--tmpdir` to +`mktemp`. + +Use `trap` to clean up your temporary files. This way, even if your +program terminates early, things will be cleaned up. + +Bash best practices +=================== + +Basically, know what you are doing, and be safe with it. The problem +is that most people don't know about safe bash scripting. + +A lot of people look at the "Advanced Bash Scripting" ebook--DO NOT do +that, it is trash... though it contains a "reference card" page that +may be useful and isn't trash. + +Take a look at Gentoo's Bash guidelines +<http://devmanual.gentoo.org/tools-reference/bash/index.html>. +They're pretty good, and cover most of the "gotcha's" about Bash +syntax. It mentions but discourages the use of Bash 3 +features... why? Who still uses Bash 2? Feel free to use Bash 4 +features! + +I wrote an article on Bash arrays +<https://lukeshu.com/blog/bash-arrays.html>. A lot of people think +they're tricky, but they're simple once you know how they work. It's +short enough that you should read the whole thing. Know the +difference between `"${array[@]}"` and `"${array[*]}"`. And I'll say +it again here, ALWAYS wrap those in double quotes; there is no reason +I can think of that the unquoted behavior would ever be the correct +thing. + +My brief rules of thumb: + + - Quote every variable. + - That includes arrays: `"${array[@]}"` and `"${array[*]}"`. + - In most (but not all!) cases inside of `[[ ... ]]` conditions, + variables don't need to be quoted. When in doubt, quote them. + - When assigning one variable to another, you don't need quotes; + you don't need quotes for `foo=$bar` + - Try to avoid global variables; declare all variables in functions + with `local`. + - Or `declare`; inside of a function, unless you pass the `-g` + flag, `declare` makes the variable local. + - Use `local VAR` before a `for VAR in LIST` loop--the variable is created in the + current scope, not the scope of the loop. + - Feeding input to `while` loops is weird because of how subshells + work: + + # Input from a file + # BAD + cat file | while read line; do + ... + done + # GOOD + while read line; do + ... + done <file + + # Input from a program + # BAD + prog | while read line; do + ... + done + # GOOD + while read line; do + ... + done < <(prog) diff --git a/HACKING/code-style.md b/HACKING/code-style.md new file mode 100644 index 0000000..077bc49 --- /dev/null +++ b/HACKING/code-style.md @@ -0,0 +1,62 @@ +The style guidelines aren't terribly strict. As long as things are +consistent per-file, I'm pretty happy. + +Style guidelines +================ + +Unless you have a good reason, use `[[ ... ]]` instead of `[ ... ]`; +they work similarly, but `[[ ... ]]` is sometimes more readable (fine, +rarely, but never less), and is harder to make mistakes with quoting, +because it is syntactic magic, as opposed to `[ ... ]` which is an +executable which just happens to be implemented as a builtin. + +Use a litteral tab for indent. When indenting line-wrapped text, such +as that for `prose`, do it like this: (» indicates tab, · indicates +space) + + func() { + » prose "This is the first line. This paragraph is going to be + » ·······wrapped." + } + +The `; then` and `; do` should go on the same line as +`if`/`elif`/`for`/`while`. Also, there is no space before the `;`. + +Prefer the `for VAR in LIST` syntax over the `for ((init; cond; inc))` +syntax, when possible. For example (heh, `for` example): + + local i + for (( i = 1 ; i <= 10 ; i++ )); do + +should be + + local i + for i in {1..10}; do + +Of course, if the upper bound is a variable, the C-like syntax is +the better option, as otherwise you would have to use `seq` (calling +an external), or `eval` (gross, easy to mess up royally). + +Indent comments like you would code; don't leave them at the beginning +of the line. Example: + + for item in "${list[@]}"; do + if [[ $item == foo ]]; then + # BAD + foobar + fi + if [[ $item == bar ]]; then + # GOOD + barbaz + fi + done + +Fauno, I'm sorry. But I don't know how you can read your own code :P. + +Some people argue in favor of the useless use of cat, because data +should flow from left to right. However, the input redirection +doesn't have to go on the right side of a command: + + cat file | program # useless use of cat + program < file # data flows right to left + < file program # just right diff --git a/HACKING/contributing.md b/HACKING/contributing.md new file mode 100644 index 0000000..65066b5 --- /dev/null +++ b/HACKING/contributing.md @@ -0,0 +1,26 @@ +Contributing +============ + +I'd love to have your patches! Code should be hackable; if you want +to modify something, but can't figure out how: 1) ping me for help, 2) +it probably means the code was too complicated in the first place. + +Patches should be sent to <dev@lists.parabolagnulinux.org>; please put +"[PATCH]" and "libretools" in the subject line. If you have commit +access, but want me to look over it first, feel free to create a new +branch in git, and I will notice it. Try to avoid pushing to the +"master" branch unless it's a trivial change; it makes it easier to +review things; though I *will* look over every commit before I do a +release, so don't think you can sneak something in :) + +Be sure to make sure to follow the licensing requirements in +`HACKING/licensing.md` + +I'd love to discuss possible changes on IRC (I'm lukeshu), either on +irc.freenode.net#parabola or in personal messages. My account may be +online even if I'm not; I will eventually see your message, I do a +search for mentions of "luke" on #parabola every time I get on. + +Please write unit tests for new functionality. Or old functionality. +Please write unit tests! See `HACKING/testing.md` for details on +testing. diff --git a/HACKING/licensing.md b/HACKING/licensing.md new file mode 100644 index 0000000..6d17e31 --- /dev/null +++ b/HACKING/licensing.md @@ -0,0 +1,19 @@ +We don't require copyright assignment or any fancy paperwork! Just +make sure you specify the license and include a copyright statement +with your name and the current year. + +New code should (please) be licensed GPLv2+. I say v2 instead of v3 +because some code from Arch is GPLv2 (no "or any later version"), and +having to worry about which programs can be combined is a huge pain. + +Copyright statements should look like + + # Copyright (C) YEARS NAME <EMAIL> + +for most code, for 3rd-party code that has been imported, indent it a +bit: + + # Copyright (C) YEARS NAME <EMAIL> + +Always put a line with `# License:` specifying the license of that +file. diff --git a/HACKING/testing.md b/HACKING/testing.md new file mode 100644 index 0000000..8dee485 --- /dev/null +++ b/HACKING/testing.md @@ -0,0 +1,18 @@ +Testing +======= + +Please write unit tests for new things. Tests can be run with `make +check`, which just runs `./testenv roundup` in the `test/` directory. +Relatedly, you need the `roundup` (the `sh-roundup` package on +Parabola) tool to run the tests. `./testenv` can be given +`--no-network` and/or `--no-sudo` to dissable tests that require those +things. Make can be made to pass those things in by setting +`TESTENVFLAGS`. If you don't dissable either, I *strongly* recommend +setting TMPDIR to somewhere on a btrfs partition before running the +tests; otherwise the chroot tests will take forever. I mean, they +already take long on btrfs, but without it... _dang_. + +I also recommend having the `haveged` daemon running. That's good +general advice, but also: some of the tests make GPG keys, this +"should" take on the order of 1 second, but can take several minutes +if you don't have `haveged` running. |