Fix --quiet (all.sh, check_names.py) #261
Conversation
The help says "Only output component names, and errors if any." but actually it was also printing out all invocations of 'make' that are happening outside of components (clean, neat, generated_files). Which is probably fine for normal usage, but not what we want with --quiet. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
Mbed TLS development no longer has a root makefile. Checking it resulted in the following on stderr: head: cannot open 'Makefile' for reading: No such file or directory and indeed the function was no longer working as intended. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
In a successful run, it had absolutely no visible effect. The output even included cmake's output (see next commit). It only had an effect in case errors are found: then they are reported in a terse way (one line per error) instead of with full context and explanations. As a user, I expect a script invoked with -q to not have 10 lines of output telling me "i'm checking this... it's ok... that too is ok, etc" (And, see next-next commit, I definitely don't expect the list of all things that are OK to be printed on stderr...) Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
So far it was already present even with -q. Clearly we don't want it with -q, and I'd argue in normal usage we don't care either, it really is an internal detail. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
Before this commit, upon success the script was writing nothing to stdout (except output from cmake before the previous commit) and everything to stderr. This is not normal behaviour. It confuses scripts like `all.sh` that try to implemented their own quiet mode by by redirecting stdout of components to /dev/null. It would confuse scripts which would redirect stderr to a file then look if this file is empty or not to double-check for issues (in addition to looking at the script's exite code). Instead, use stdout for normal output (no options) and debug output (-v), and use stderr for warnings and errors. Signed-off-by: Manuel Pégourié-Gonnard <[email protected]>
mpg
added
size-s
gilles-peskine-arm
left a comment
gilles-peskine-arm
left a comment
There was a problem hiding this comment.
Ok for not breaking stuff (except you need to pacify pylint). I haven't verified completeness because if it's good enough for you, it's good enough. I don't like the added complexity much.
| build_dir = tempfile.mkdtemp() | ||
| os.chdir(build_dir) | ||
| subprocess.run( | ||
| result = subprocess.run( |
There was a problem hiding this comment.
Partly preexisting: it would be more robust to use subprocess.check_output (formerly check_call).
| logging.INFO if not args.quiet else | ||
| logging.WARN) | ||
|
|
||
| # stdout handler: DEBUG/INFO only |
There was a problem hiding this comment.
Why not send everything to stderr? That's the normal place for informational messages, as opposed to application data which goes to stdout. Separating two logging streams also creates the risk that they'll end up displayed out of order due to buffering (which I think doesn't happen on Jenkins, it did happen on Travis).
| if [[ -e ${PWD}/framework/scripts/quiet ]]; then | ||
| export PATH=${PWD}/framework/scripts/quiet:$PATH | ||
| # pass on our own QUIET setting to the wrappers | ||
| export QUIET |
There was a problem hiding this comment.
I'd prefer to avoid environment variable names that are too generic. Can we use MBEDTLS_QUIET here? (Just as the environment variable, not as the name inside the script.) Or would that be too confusing if we have two names and too much work if we rename it everywhere?
| if ! in_mbedtls_repo; then | ||
| return | ||
| fi | ||
| local makefiles="library/Makefile programs/Makefile programs/fuzz/Makefile tests/Makefile" |
There was a problem hiding this comment.
Partly preexisting: how about $(git ls-files Makefile '**/Makefile')? That would be more future proof for when we finish removing the makefiles (although I suspect it'll take longer than what some people are hoping).
gilles-peskine-arm
added
size-xs
2 participants
Description
I like to run
all.sh -q <some_check_components>as part of my pre-push script, but these days it's not really as quiet as advertised ("Only output component names, and errors if any."). This PR addresses most of that:-qoption ofcheck_namesmore effective - also make normal output go to stdout not stderr.PR checklist