errros: add a specific error when running commands from plugins#2004
Conversation
a30459a to
efdd15a
Compare
There was a problem hiding this comment.
This looks mostly good, although I have a few suggestions inline. One larger suggestion is to also check through the plugins to ensure they're using self.run or self.run_output instead of common.run or common.run_output. I know there's a bit of that in jhbuild, for example. I figure if there's a reason for plugins to not be using self.run et. al, they should go all the way to using subprocess directly.
| raise errors.SnapcraftPluginCommandError( | ||
| command=cmd, part_name=self.name) | ||
|
|
||
| def run_output(self, cmd, cwd=None, **kwargs): |
There was a problem hiding this comment.
How about similar logic here?
|
|
||
| fmt = ( | ||
| 'The command {command!r} run for the {part_name!r} part has failed.\n' | ||
| 'Verify that the part is using the correct parameters and try again.' |
There was a problem hiding this comment.
I think the exit code would be helpful as well. Perhaps something like:
'Failed to run {command!r} for {part_name!r}: '
'Exited with code {exit_code}.\n'
'Verify that the part is using the correct parameters and try again.'
When a plugin fails a horrible traceback is printed because there is no SnapcraftError class handling the situation. LP: #1753965 Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>
efdd15a to
183d2cb
Compare
snapcraft/_baseplugin.py
Outdated
| except CalledProcessError as process_error: | ||
| raise errors.SnapcraftPluginCommandError( | ||
| command=cmd, part_name=self.name, | ||
| exit_code=process_error.returncode) |
There was a problem hiding this comment.
Agh sorry, missed this before, but you probably want a from process_error on both of these.
| fmt = ( | ||
| 'The command {command!r} run for the {part_name!r} part has failed.\n' | ||
| 'Exited with exit code {exit_code}.\n' | ||
| 'Verify that the part is using the correct parameters and try again.' |
There was a problem hiding this comment.
Channeling my inner Leo, this does not meet the agreed-upon error message format which is:
<what>: <why>\n
<how to fix>
It seems to be
<what>\n
<why>\n
<how to fix>
snapcraft/plugins/_python/_pip.py
Outdated
| self._run_output([], stderr=subprocess.STDOUT) | ||
| except subprocess.CalledProcessError as e: | ||
| except (subprocess.CalledProcessError, | ||
| SnapcraftPluginCommandError) as e: |
There was a problem hiding this comment.
Note that self._run{_output} here actually goes through common.run{_output} as it's not part of a plugin. You shouldn't need to be handling SnapcraftPluginCommandErrors here.
snapcraft/plugins/python.py
Outdated
| # in which case we will rely on the `pip install .` ran before | ||
| # this. | ||
| with contextlib.suppress(subprocess.CalledProcessError): | ||
| with contextlib.suppress(subprocess.CalledProcessError, |
There was a problem hiding this comment.
Oh good catch, we would have had issues! However, given that this uses self.run, there's no longer a need to catch CalledProcessError here, is there?
kyrofa
left a comment
There was a problem hiding this comment.
Very nice, thank you for this!
When a plugin fails a horrible traceback is printed because there is no
SnapcraftError class handling the situation.
LP: #1753965
Signed-off-by: Sergio Schvezov sergio.schvezov@canonical.com
./runtests.sh static?./runtests.sh unit?