Fix/improve output on errors #482
Conversation
We don't document exit code We sometimes don't care for the return of a function (e.g. 0 is the same as 1, though 1 indicates to the developer that something should potentially be handled) As for the change to list_add I don't actually see how it could fail (attaching to a file, or creating one?) and if it does is _very_ unexpected situations we want it to fail louder
This allow us to, at the same time: * create the file * return from inner functions, for exiting purposes * writing to stderr * without the output being consumed by sub-shells
In certain situations, I was getting execution stopping silently (e.g. 404 on curl calls) because we're using shortcuts such as || true, and || return, or even || exit without prior error messages, which this commit attempts to fix
When it's clear to do so, instead of bubbling up return values until we're ready to exit
Reduces the number of functions assert_ is ok to use, but then we have scattered exit vs. return which makes it harder to reason on Also, we move `assert_build_name_unused` to `is_build_name_used` to avoid double negation
If the function is exiting we call exit If it's returning we prefer return (and exiting higher up)
Previous changes allow us to prevent two calls to `get_active_path` while making sure: * we exit on error * always continue to function calls otherwise (we reduce a nesting level with this change)
Prefer failure with controlled messages
We always show_build_logfile with a message then autoclean with a file "pointer" With a function it'll be easier to not forget how to do cleanup, if the function changes in the future
paulo-ferraz-oliveira
force-pushed
the
fix/improve-output-on-error
branch
from
932f562
to
ff6bd1b
Compare
Due to the recent changes we started exiting with 1 on _kerl status_ when no installation is active
paulo-ferraz-oliveira
force-pushed
the
fix/improve-output-on-error
branch
from
c6c64bc
to
e71bef8
Compare
|
This is failing on "active installation validation". It wasn't working (I believe) as expected before, either, it was just not noticeable since we weren't error'ing out. Here's a previous run result 
where there's no output after 😄 |
I think errors should always exit with a non-0 status ... but you might not always show an error when you exit with a non-0 status (this is context dependent)
README.md
Outdated
| @@ -276,7 +274,9 @@ The available releases are: | |||
| ... | |||
| 26.0.2 * | |||
| ``` | |||
| <!-- markdownlint-enable MD007 # line-length --> | |||
|
|
|||
| **Note**: this list, kept in a file managed by Kerl, is different depending on the build backend | |||
There was a problem hiding this comment.
kerl no uppercasing please
There was a problem hiding this comment.
Sure. It was there already, so I took "advantage" of that. Maybe I'll take this chance to lowercase it all, then (?)
Edit: apparently also by me 😄
There was a problem hiding this comment.
|
Apart from lowercasing Kerl, is there anything else you wanna see changed in this pr's scope? |
kerl
Outdated
| curl -s -f -L -o "$KERL_DOWNLOAD_DIR"/MD5 "$ERLANG_DOWNLOAD_URL"/MD5 || return 1 | ||
| http_code=$(_curl "$KERL_DOWNLOAD_DIR"/MD5 "$ERLANG_DOWNLOAD_URL"/MD5) | ||
| if [ 200 != "$http_code" ]; then | ||
| error "_curl $ERLANG_DOWNLOAD_URL/MD5 returned $http_code!" |
There was a problem hiding this comment.
don't think we need the _curl part
There was a problem hiding this comment.
Ok, I'll get rid of that.
There was a problem hiding this comment.
kerl
Outdated
| curl -s -f -L -o "$tarball_file" "$tarball_url" || return 1 | ||
| http_code=$(_curl "$tarball_file" "$tarball_url") | ||
| if [ 200 != "$http_code" ]; then | ||
| error "_curl $tarball_url returned $http_code!" |
There was a problem hiding this comment.
same - i don't think we need to expose the internal function call name in our errors - they just need to say the http request returned X status or w/e
There was a problem hiding this comment.
|
looks good!! |
|
Thanks, @jadeallenx. Merging and going for the next one 😄 |
Description
I was going for #202, but while testing the initial conditions to replicate it I found myself having calls to functions that would silently fail. I decided to look into that as it helps in development and consumption.
As an example,
kerl build>do_normal_build>download>tarball_downloadwould fail if the build was not found inerlang.org, but silently.Now we get
➜ kerl git:(fix/improve-output-on-error) ✗ KERL_BUILD_BACKEND=tarball ./kerl build 26.0.2 Downloading tarball otp_src_26.0.2.tar.gz to .../.kerl/archives... ERROR: _curl https://erlang.org/downloa/otp_src_26.0.2.tar.gz returned 404!I ended up reviewing basically all
exitandreturnsites to increase consistency and improve logging.Opening this as draft while I perform some more tests and local analysis.Further considerations
I tried to do as much commit-by-commit in this one as possible, then I reviewed the individual commits to add comments on changes that were later reversed on self-review.