-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add docker compose based test environment for Linux #687
Add docker compose based test environment for Linux #687
Conversation
This makes it trivial to inspect, line by line, what got covered. That is useful when evaluating coverage for a new feature. Signed-off-by: Martí Bolívar <[email protected]>
Nice! No time to review at this moment but if you're looking for some Windows inspiration you can find some here: https://github.com/thesofproject/sof/blob/main/.github/workflows/zephyr.yml https://github.com/thesofproject/sof/actions/workflows/daily-tests.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno much about Docker unfortunately...
@@ -0,0 +1,29 @@ | |||
#!/bin/bash | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set -e |
It does not catch everything but it catches a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its omission is intentional in this file -- see error handling being done manually below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set -e
is not incompatible with manual error handling at all. It would be totally unusable otherwise. set -e
catches many failures that the developer did not anticipate or was too lazy or too optimistic to handle manually - now and in the future.
The only problem with set -e
is the false, 100% safety impression that it can give to some inexperienced people. For everything else it's fine.
docker-testing/in-container-test.sh
Outdated
WEST=/west | ||
|
||
die() { | ||
[ ! -z "$@" ] && echo "error: $@" >&2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer an empty error:
. No one should die
without a message.
I also prefer a die
that is fully printf
compatible:
die()
{
>&2 printf '%s ERROR: ' "$0"
# We want die() to be usable exactly like printf
# shellcheck disable=SC2059
>&2 printf "$@"
exit 1
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an error path when there's no argument. I'd rather not use printf syntax, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot this: this script is more than big enough to deserve a main()
function.
Very small change, all the benefits listed in:
One thing we learned in SOF though; the Dockerfiles should be in a separate repo: Except for "unit" test code which is very dependent on the version of the production code it's testing, you usually want the convenience to combine any version of test code with any version of production code. Typically: git bisect production code with the latest version of test code. Or here: git bisect with the latest docker image. Now don't get me wrong: anyone can take 2 random git repos mostly unrelated to each other, merge them into one and start interleaving their unrelated development histories. It is totally possible and even not that uncommon (especially when you don't know about Another instance: every Dockerfile change should trigger some Github Action that rebuilds the container and runs the test suite again. But any Dockerfile CI takes a very long time so you don't want that to happen for mosts west PRs which do NOT change any Dockerfile. Congratulations, now you need a path-based filter on every Github Action to separate two different CIs that should have never been together in the first place. Possible but pretty inconvenient. |
Our release process documentation recommends getting passing tox results on as many popular linux distributions as time allows. Doing this by hand is cumbersome, redundant, and error prone. Add a directory with a helper script that automates the entire process using docker compose and document its use in MAINTAINERS.rst. Signed-off-by: Martí Bolívar <[email protected]>
123e644
to
8049708
Compare
This is meant for manual testing on various distros at release time; can we keep things simple for now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still no time to look at the docker files yet sorry.
@@ -0,0 +1,29 @@ | |||
#!/bin/bash | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set -e
is not incompatible with manual error handling at all. It would be totally unusable otherwise. set -e
catches many failures that the developer did not anticipate or was too lazy or too optimistic to handle manually - now and in the future.
The only problem with set -e
is the false, 100% safety impression that it can give to some inexperienced people. For everything else it's fine.
WEST=/west | ||
|
||
die() { | ||
if [ ! -z "$@" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"$@" expands to multiple strings. That's not what you want here
http://mywiki.wooledge.org/BashPitfalls#for_arg_in_.24.2A
http://mywiki.wooledge.org/BashGuide/Parameters#Special_Parameters_and_Variables
if [ ! -z "$@" ]; then | |
if [ -n "$*" ]; then |
Or even just:
if [ ! -z "$@" ]; then | |
if [ -n "$1" ]; then |
Or if [ $# -eq 0 ]
Then for consistency:
echo "error: $*" >&2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW you don't HAVE to printf 'error: %s' "$var"
, you can still do printf "error: $var"
if you want. The well-tested die()
function I recommended earlier supports either indifferently.
TOX_LOG_IN_HOST="$WEST_TOX_OUT_IN_HOST/tox.log" | ||
WEST_TESTDIR="/tmp/west" | ||
|
||
mkdir "$WEST_TOX_OUT" || die "failed to make $WEST_TOX_OUT in container ($WEST_TOX_OUT_IN_HOST in host)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit
mkdir "$WEST_TOX_OUT" || die "failed to make $WEST_TOX_OUT in container ($WEST_TOX_OUT_IN_HOST in host)" | |
mkdir "$WEST_TOX_OUT" || | |
die "failed to make $WEST_TOX_OUT in container ($WEST_TOX_OUT_IN_HOST in host)" |
(No need for a backslash in this case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How likely is this mkdir
to fail really? Isn't set -e
enough?
main() | ||
{ | ||
# Verify the container environment set up meets this script's requirements. | ||
[ ! -z "$WEST_TOX_OUT" ] || die "missing $WEST_TOX_OUT" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[ ! -z "$WEST_TOX_OUT" ] || die "missing $WEST_TOX_OUT" | |
[ -n "$WEST_TOX_OUT" ] || die "missing $WEST_TOX_OUT" |
The fewer negations the better and copy/paste of !
can trigger history recall in interactive testing on the command line.
When preparing a west release, we like to make sure that it works on more Linux environments than whatever is currently tested in GitHub CI. Doing this by hand is a pain. Add some automation for making this more painless.