Closed wackou closed 1 year ago
Thanks for the PR. I'm not convinced in adding new docker_error exception, but the rest looks good.
So, I added the docker_error
because I needed to propagate an exception, but dune_error
wasn't available here, and it cannot be imported because it is dune.py
that imports docker.py
. I wasn't 100% convinced it was the best choice either. I see the following options:
docker_error
as of nowdune_error
to a different file that can be imported by both dune.py
and docker.py
(but I'm not sure that dune_error
and docker_error
have the same semantics, i.e. are supposed to represent the same type of errors)check_returncode
on the completed process and let a CalledProcessError
propagate (see: https://docs.python.org/3/library/subprocess.html#subprocess.CompletedProcess.check_returncode) (but then we can't print stdout/stderr before raising the exception)Let me know which solution you prefer the best, or if you have any other suggestion, and I'll implement that.
Thanks!
This refactors a bit the various
docker::execute_cmd_*
methods in order to have a single, generic and flexible one that can be called by other specialized ones if needed.I also added an option to be able to have colored output outside the docker process (this is my main motivation for the PR!) cmake build and test benefit from that for instance.
Also, by default the dune executable will also fail if a
docker.execute_cmd()
fails, which ensures we do not fail silently in certain cases and go on. This can be deactivated by passing thecheck_status=False
option.