From 96ad55013f635cdb5246a891cbdc667546f24e0f Mon Sep 17 00:00:00 2001 From: Claudio Matsuoka Date: Sun, 3 Nov 2024 18:49:51 -0300 Subject: [PATCH 1/4] feat(emitter): decouple message emitter and printer Introduce a common protocol that sits between message emitters and the screen printer and log handler, as the first step to allow the local system to listen for remotely emitted messages. This is intended to be used for emitter message streaming from managed instances. Signed-off-by: Claudio Matsuoka --- craft_cli/errors.py | 8 +- craft_cli/messages.py | 227 +++++++++++++++++++++++++++++++++--------- 2 files changed, 187 insertions(+), 48 deletions(-) diff --git a/craft_cli/errors.py b/craft_cli/errors.py index a7a9bddb..336799e5 100644 --- a/craft_cli/errors.py +++ b/craft_cli/errors.py @@ -23,8 +23,8 @@ from typing import Any, Optional, Union, cast -class CraftError(Exception): - """Signal a program error with a lot of information to report.""" +class BaseErrorData: + """Common error fields for CraftError and CLI protocol.""" message: str """The main message to the user, to be shown as first line (and probably only that, @@ -56,6 +56,10 @@ class CraftError(Exception): retcode: int """The code to return when the application finishes.""" + +class CraftError(BaseErrorData, Exception): + """Signal a program error with a lot of information to report.""" + def __init__( # noqa: PLR0913 (too many arguments) self, message: str, diff --git a/craft_cli/messages.py b/craft_cli/messages.py index bff596ed..bffb44cf 100644 --- a/craft_cli/messages.py +++ b/craft_cli/messages.py @@ -25,6 +25,7 @@ import enum import functools +import json import logging import os import pathlib @@ -38,6 +39,8 @@ import platformdirs +from .errors import BaseErrorData, CraftError + try: import win32pipe # type: ignore[import] @@ -67,6 +70,94 @@ # craft_cli/pytest_plugin.py ) TESTMODE = False +# Constants for emitter-to-emitter line protocol +_MSG_MESSAGE = "" +_MSG_VERBOSE = "" +_MSG_DEBUG = "" +_MSG_TRACE = "" +_MSG_PROGRESS_PERMANENT = "

" +_MSG_PROGRESS = "

" +_MSG_ERROR = "" + +class _ErrorMessage(BaseErrorData): + message: str + is_command_error: bool + traceback_lines: list[str] + + def __init__( # noqa: PLR0913 (too many arguments) + self, + message: str, + *, + details: str | None, + resolution: str | None, + docs_url: str | None, + logpath_report: bool, + reportable: bool, + retcode: int, + doc_slug: str | None, + stderr: str | None, + is_command_error: bool, + traceback_lines: list[str] | None, + ) -> None: + self.message = message + self.details = details + self.resolution = resolution + self.docs_url = docs_url + self.logpath_report = logpath_report + self.reportable = reportable + self.retcode = retcode + self.doc_slug = doc_slug + self.stderr = stderr + self.is_command_error = is_command_error + self.traceback_lines = traceback_lines or [] + + @classmethod + def from_error(cls, error: CraftError) -> _ErrorMessage: + """Create an error message from an error exception. + + :param error: The CraftError exception. + :return: The error message for the CLI protocol. + """ + msg = cls( + message = str(error), + details = error.details, + resolution = error.resolution, + docs_url = error.docs_url, + doc_slug = error.doc_slug, + stderr = "", + logpath_report = error.logpath_report, + reportable = error.reportable, + retcode = error.retcode, + is_command_error = False, + traceback_lines = [], + ) + + if error.__cause__: + msg.traceback_lines = list(_get_traceback_lines(error.__cause__)) + + if isinstance(error, errors.CraftCommandError): + msg.is_command_error = True + msg.stderr = error.stderr + + return msg + + @classmethod + def loads(cls, data: str) -> _ErrorMessage: + """Unmarshal error data from a string. + + :param data: Serialized error attributes. + :return: CraftError object. + """ + j = json.loads(data) + return cls(**j) + + def dumps(self) -> str: + """Marshal error data to a string. + + :return: Serialized error attributes. + """ + return json.dumps(self.__dict__) + def _get_log_filepath(appname: str) -> pathlib.Path: """Provide a unique filepath for logging. @@ -532,11 +623,7 @@ def message(self, text: str) -> None: Normally used as the final message, to show the result of a command. """ - stream = None if self._mode == EmitterMode.QUIET else sys.stdout - if self._streaming_brief: - # Clear the message prefix, as this message stands alone - self._printer.set_terminal_prefix("") - self._printer.show(stream, text) + self._handle_cli_message(_MSG_MESSAGE, text) @_active_guard() def verbose(self, text: str) -> None: @@ -545,16 +632,7 @@ def verbose(self, text: str) -> None: Useful to provide more information to the user that shouldn't be exposed when in brief mode for clarity and simplicity. """ - if self._mode in (EmitterMode.QUIET, EmitterMode.BRIEF): - stream = None - use_timestamp = False - elif self._mode == EmitterMode.VERBOSE: - stream = sys.stderr - use_timestamp = False - else: - stream = sys.stderr - use_timestamp = True - self._printer.show(stream, text, use_timestamp=use_timestamp) + self._handle_cli_message(_MSG_VERBOSE, text) @_active_guard() def debug(self, text: str) -> None: @@ -564,11 +642,7 @@ def debug(self, text: str) -> None: for the app developers to understand why things are failing or performing forensics on the produced logs. """ - if self._mode in (EmitterMode.QUIET, EmitterMode.BRIEF, EmitterMode.VERBOSE): - stream = None - else: - stream = sys.stderr - self._printer.show(stream, text, use_timestamp=True) + self._handle_cli_message(_MSG_DEBUG, text) @_active_guard() def trace(self, text: str) -> None: @@ -580,13 +654,81 @@ def trace(self, text: str) -> None: It only produces information to the screen and into the logs if in TRACE mode. """ - # as we're not even logging anything if not in TRACE mode, instead of calling the - # Printer with no stream and the 'avoid_logging' flag (which would be more consistent - # with the rest of the Emitter methods, in this case we just avoid moving any - # machinery as much as possible, because potentially there will be huge number - # of trace calls. - if self._mode == EmitterMode.TRACE: - self._printer.show(sys.stderr, text, use_timestamp=True) + self._handle_cli_message(_MSG_TRACE, text) + + def _handle_cli_message(self, msg_type: str, text: str) -> None: # noqa: PLR0912 (too many branches) + """Process emitted message according to the local system configuration. + + Emitted messages can be handled differently according to the type of the + message and configuration of the message handler. By decoupling the emitter + and message printer, the (potentially remote) emitter sets the type of the + emitted message, which is printed according to the local handler's verbosity + rules. + + Note that text lines are passed directly, whereas structured error data is + serialized. This shouldn't impose significant performance overhead as error + messages are only emitted at the end of a failed session. + + :param msg: The emitted message, in Craft CLI message protocol format. + """ + if msg_type == _MSG_MESSAGE: + stream = None if self._mode == EmitterMode.QUIET else sys.stdout + if self._streaming_brief: + # Clear the message prefix, as this message stands alone + self._printer.set_terminal_prefix("") + self._printer.show(stream, text) + + elif msg_type == _MSG_VERBOSE: + if self._mode in (EmitterMode.QUIET, EmitterMode.BRIEF): + stream = None + use_timestamp = False + elif self._mode == EmitterMode.VERBOSE: + stream = sys.stderr + use_timestamp = False + else: + stream = sys.stderr + use_timestamp = True + self._printer.show(stream, text, use_timestamp=use_timestamp) + + elif msg_type == _MSG_DEBUG: + if self._mode in (EmitterMode.QUIET, EmitterMode.BRIEF, EmitterMode.VERBOSE): + stream = None + else: + stream = sys.stderr + self._printer.show(stream, text, use_timestamp=True) + + elif msg_type == _MSG_TRACE: + # as we're not even logging anything if not in TRACE mode, instead of calling the + # Printer with no stream and the 'avoid_logging' flag (which would be more consistent + # with the rest of the Emitter methods, in this case we just avoid moving any + # machinery as much as possible, because potentially there will be huge number + # of trace calls. + if self._mode == EmitterMode.TRACE: + self._printer.show(sys.stderr, text, use_timestamp=True) + + elif msg_type in (_MSG_PROGRESS_PERMANENT, _MSG_PROGRESS): + is_permament = msg_type == _MSG_PROGRESS_PERMANENT + stream, use_timestamp, ephemeral = self._get_progress_params(is_permament) + + if self._streaming_brief: + # Clear the "new thing" prefix, as this is a new progress message. + self._printer.set_terminal_prefix("") + + self._printer.show(stream, text, ephemeral=ephemeral, use_timestamp=use_timestamp) + + if self._mode == EmitterMode.BRIEF and ephemeral and self._streaming_brief: + # Set the "progress prefix" for upcoming non-permanent messages. + self._printer.set_terminal_prefix(text) + + elif msg_type == _MSG_ERROR: + if self._streaming_brief: + # Clear the message prefix, as this error stands alone + self._printer.set_terminal_prefix("") + self._report_error(_ErrorMessage.loads(text)) + + else: + raise RuntimeError("unknown message type '{msg_type}'") + def _get_progress_params( self, permanent: bool # noqa: FBT001 (boolean positional arg) @@ -626,17 +768,11 @@ def progress(self, text: str, permanent: bool = False) -> None: # noqa: FBT001, These messages will be truncated to the terminal's width, and overwritten by the next line (unless verbose/trace mode). """ - stream, use_timestamp, ephemeral = self._get_progress_params(permanent) - - if self._streaming_brief: - # Clear the "new thing" prefix, as this is a new progress message. - self._printer.set_terminal_prefix("") - - self._printer.show(stream, text, ephemeral=ephemeral, use_timestamp=use_timestamp) + if permanent: + self._handle_cli_message(_MSG_PROGRESS_PERMANENT, text) + else: + self._handle_cli_message(_MSG_PROGRESS, text) - if self._mode == EmitterMode.BRIEF and ephemeral and self._streaming_brief: - # Set the "progress prefix" for upcoming non-permanent messages. - self._printer.set_terminal_prefix(text) @_active_guard() def progress_bar( @@ -710,7 +846,7 @@ def ended_ok(self) -> None: """Finish the messaging system gracefully.""" self._stop() - def _report_error(self, error: errors.CraftError) -> None: # noqa: PLR0912 (too many branches) + def _report_error(self, error: _ErrorMessage) -> None: # noqa: PLR0912 (too many branches) """Report the different message lines from a CraftError.""" if self._mode in (EmitterMode.QUIET, EmitterMode.BRIEF, EmitterMode.VERBOSE): use_timestamp = False @@ -721,10 +857,10 @@ def _report_error(self, error: errors.CraftError) -> None: # noqa: PLR0912 (too # The initial message. Print every line individually to correctly clear # previous lines, if necessary. - for line in str(error).splitlines(): + for line in error.message.splitlines(): self._printer.show(sys.stderr, line, use_timestamp=use_timestamp, end_line=True) - if isinstance(error, errors.CraftCommandError): + if error.is_command_error: stderr = error.stderr if stderr: text = f"Captured error:\n{stderr}" @@ -734,8 +870,8 @@ def _report_error(self, error: errors.CraftError) -> None: # noqa: PLR0912 (too if error.details: text = f"Detailed information: {error.details}" self._printer.show(full_stream, text, use_timestamp=use_timestamp, end_line=True) - if error.__cause__: - for line in _get_traceback_lines(error.__cause__): + if error.traceback_lines: + for line in error.traceback_lines: self._printer.show(full_stream, line, use_timestamp=use_timestamp, end_line=True) # hints for the user to know more @@ -761,12 +897,11 @@ def _report_error(self, error: errors.CraftError) -> None: # noqa: PLR0912 (too @_active_guard(ignore_when_stopped=True) def error(self, error: errors.CraftError) -> None: """Handle the system's indicated error and stop machinery.""" - if self._streaming_brief: - # Clear the message prefix, as this error stands alone - self._printer.set_terminal_prefix("") - self._report_error(error) + msg = _ErrorMessage.from_error(error) + self._handle_cli_message(_MSG_ERROR, msg.dumps()) self._stop() + @_active_guard() def set_secrets(self, secrets: list[str]) -> None: """Set the list of strings that should be masked out in all output.""" From ab23abe25e132cb79c20695b19ece0c3dc9b2008 Mon Sep 17 00:00:00 2001 From: Claudio Matsuoka Date: Mon, 4 Nov 2024 17:54:04 -0300 Subject: [PATCH 2/4] feat(emitter): also decouple captured message streaming Note that server side handling of captured messages will be done by the emitter and not the printer. Signed-off-by: Claudio Matsuoka --- craft_cli/messages.py | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/craft_cli/messages.py b/craft_cli/messages.py index bffb44cf..94929735 100644 --- a/craft_cli/messages.py +++ b/craft_cli/messages.py @@ -78,6 +78,7 @@ _MSG_PROGRESS_PERMANENT = "

" _MSG_PROGRESS = "

" _MSG_ERROR = "" +_MSG_STREAM = "" class _ErrorMessage(BaseErrorData): message: str @@ -336,7 +337,12 @@ def _write(self, data: bytes) -> None: # can correctly count the characters. unicode_line = unicode_line.replace("\t", " ") text = f":: {unicode_line}" - self.printer.show(self.stream, text, **self.printer_flags) + + self._handle_cli_message(_MSG_STREAM, text) + + def _handle_cli_message(self, _msg_type: str, text: str) -> None: + """Process emitted message according to the local system configuration.""" + self.printer.show(self.stream, text, **self.printer_flags) def _run_posix(self) -> None: """Run the thread, handling pipes in the POSIX way.""" @@ -617,6 +623,11 @@ def set_mode(self, mode: EmitterMode) -> None: sys.stderr, msg, use_timestamp=use_timestamp, avoid_logging=True, end_line=True ) + @_active_guard() + def stream(self, text: str) -> None: + """Show strings streamed from a stream context.""" + self._handle_cli_message(_MSG_STREAM, text) + @_active_guard() def message(self, text: str) -> None: """Show an important message to the user. @@ -671,7 +682,21 @@ def _handle_cli_message(self, msg_type: str, text: str) -> None: # noqa: PLR091 :param msg: The emitted message, in Craft CLI message protocol format. """ - if msg_type == _MSG_MESSAGE: + if msg_type == _MSG_STREAM: + if self._mode == EmitterMode.QUIET: + # no third party stream + self._printer.show(None, text, use_timestamp=False, ephemeral=True, end_line=False) + elif self._mode == EmitterMode.BRIEF: + # third party stream to stderr + self._printer.show(sys.stderr, text, use_timestamp=False, ephemeral=True, end_line=False) + elif self._mode == EmitterMode.VERBOSE: + # third party stream to stderr + self._printer.show(sys.stderr, text, use_timestamp=False, ephemeral=False, end_line=True) + else: + # third party stream to stderr with timestamp + self._printer.show(sys.stderr, text, use_timestamp=True, ephemeral=False, end_line=True) + + elif msg_type == _MSG_MESSAGE: stream = None if self._mode == EmitterMode.QUIET else sys.stdout if self._streaming_brief: # Clear the message prefix, as this message stands alone From 6e63af07c08b9a44d764fc3e442964bcaaf5d68f Mon Sep 17 00:00:00 2001 From: Claudio Matsuoka Date: Mon, 4 Nov 2024 19:04:35 -0300 Subject: [PATCH 3/4] chore: address linter warnings Signed-off-by: Claudio Matsuoka --- craft_cli/messages.py | 42 ++++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/craft_cli/messages.py b/craft_cli/messages.py index 94929735..bffd2b45 100644 --- a/craft_cli/messages.py +++ b/craft_cli/messages.py @@ -80,6 +80,7 @@ _MSG_ERROR = "" _MSG_STREAM = "" + class _ErrorMessage(BaseErrorData): message: str is_command_error: bool @@ -120,17 +121,17 @@ def from_error(cls, error: CraftError) -> _ErrorMessage: :return: The error message for the CLI protocol. """ msg = cls( - message = str(error), - details = error.details, - resolution = error.resolution, - docs_url = error.docs_url, - doc_slug = error.doc_slug, - stderr = "", - logpath_report = error.logpath_report, - reportable = error.reportable, - retcode = error.retcode, - is_command_error = False, - traceback_lines = [], + message=str(error), + details=error.details, + resolution=error.resolution, + docs_url=error.docs_url, + doc_slug=error.doc_slug, + stderr="", + logpath_report=error.logpath_report, + reportable=error.reportable, + retcode=error.retcode, + is_command_error=False, + traceback_lines=[], ) if error.__cause__: @@ -667,7 +668,9 @@ def trace(self, text: str) -> None: """ self._handle_cli_message(_MSG_TRACE, text) - def _handle_cli_message(self, msg_type: str, text: str) -> None: # noqa: PLR0912 (too many branches) + def _handle_cli_message( + self, msg_type: str, text: str + ) -> None: # noqa: PLR0912 (too many branches) """Process emitted message according to the local system configuration. Emitted messages can be handled differently according to the type of the @@ -688,13 +691,19 @@ def _handle_cli_message(self, msg_type: str, text: str) -> None: # noqa: PLR091 self._printer.show(None, text, use_timestamp=False, ephemeral=True, end_line=False) elif self._mode == EmitterMode.BRIEF: # third party stream to stderr - self._printer.show(sys.stderr, text, use_timestamp=False, ephemeral=True, end_line=False) + self._printer.show( + sys.stderr, text, use_timestamp=False, ephemeral=True, end_line=False + ) elif self._mode == EmitterMode.VERBOSE: # third party stream to stderr - self._printer.show(sys.stderr, text, use_timestamp=False, ephemeral=False, end_line=True) + self._printer.show( + sys.stderr, text, use_timestamp=False, ephemeral=False, end_line=True + ) else: # third party stream to stderr with timestamp - self._printer.show(sys.stderr, text, use_timestamp=True, ephemeral=False, end_line=True) + self._printer.show( + sys.stderr, text, use_timestamp=True, ephemeral=False, end_line=True + ) elif msg_type == _MSG_MESSAGE: stream = None if self._mode == EmitterMode.QUIET else sys.stdout @@ -754,7 +763,6 @@ def _handle_cli_message(self, msg_type: str, text: str) -> None: # noqa: PLR091 else: raise RuntimeError("unknown message type '{msg_type}'") - def _get_progress_params( self, permanent: bool # noqa: FBT001 (boolean positional arg) ) -> tuple[TextIO | None, bool, bool]: @@ -798,7 +806,6 @@ def progress(self, text: str, permanent: bool = False) -> None: # noqa: FBT001, else: self._handle_cli_message(_MSG_PROGRESS, text) - @_active_guard() def progress_bar( self, text: str, total: float, delta: bool = True # noqa: FBT001, FBT002 @@ -926,7 +933,6 @@ def error(self, error: errors.CraftError) -> None: self._handle_cli_message(_MSG_ERROR, msg.dumps()) self._stop() - @_active_guard() def set_secrets(self, secrets: list[str]) -> None: """Set the list of strings that should be masked out in all output.""" From 3a5a613f69e448823c1d41537e3a3ed398834973 Mon Sep 17 00:00:00 2001 From: Claudio Matsuoka Date: Wed, 6 Nov 2024 17:03:50 -0300 Subject: [PATCH 4/4] refactor: make emitter message handler public Signed-off-by: Claudio Matsuoka --- craft_cli/messages.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/craft_cli/messages.py b/craft_cli/messages.py index bffd2b45..c1b6b13a 100644 --- a/craft_cli/messages.py +++ b/craft_cli/messages.py @@ -339,9 +339,9 @@ def _write(self, data: bytes) -> None: unicode_line = unicode_line.replace("\t", " ") text = f":: {unicode_line}" - self._handle_cli_message(_MSG_STREAM, text) + self.handle_message(_MSG_STREAM, text) - def _handle_cli_message(self, _msg_type: str, text: str) -> None: + def handle_message(self, _msg_type: str, text: str) -> None: """Process emitted message according to the local system configuration.""" self.printer.show(self.stream, text, **self.printer_flags) @@ -627,7 +627,7 @@ def set_mode(self, mode: EmitterMode) -> None: @_active_guard() def stream(self, text: str) -> None: """Show strings streamed from a stream context.""" - self._handle_cli_message(_MSG_STREAM, text) + self.handle_message(_MSG_STREAM, text) @_active_guard() def message(self, text: str) -> None: @@ -635,7 +635,7 @@ def message(self, text: str) -> None: Normally used as the final message, to show the result of a command. """ - self._handle_cli_message(_MSG_MESSAGE, text) + self.handle_message(_MSG_MESSAGE, text) @_active_guard() def verbose(self, text: str) -> None: @@ -644,7 +644,7 @@ def verbose(self, text: str) -> None: Useful to provide more information to the user that shouldn't be exposed when in brief mode for clarity and simplicity. """ - self._handle_cli_message(_MSG_VERBOSE, text) + self.handle_message(_MSG_VERBOSE, text) @_active_guard() def debug(self, text: str) -> None: @@ -654,7 +654,7 @@ def debug(self, text: str) -> None: for the app developers to understand why things are failing or performing forensics on the produced logs. """ - self._handle_cli_message(_MSG_DEBUG, text) + self.handle_message(_MSG_DEBUG, text) @_active_guard() def trace(self, text: str) -> None: @@ -666,11 +666,11 @@ def trace(self, text: str) -> None: It only produces information to the screen and into the logs if in TRACE mode. """ - self._handle_cli_message(_MSG_TRACE, text) + self.handle_message(_MSG_TRACE, text) - def _handle_cli_message( + def handle_message( # noqa: PLR0912 (too many branches) self, msg_type: str, text: str - ) -> None: # noqa: PLR0912 (too many branches) + ) -> None: """Process emitted message according to the local system configuration. Emitted messages can be handled differently according to the type of the @@ -802,9 +802,9 @@ def progress(self, text: str, permanent: bool = False) -> None: # noqa: FBT001, line (unless verbose/trace mode). """ if permanent: - self._handle_cli_message(_MSG_PROGRESS_PERMANENT, text) + self.handle_message(_MSG_PROGRESS_PERMANENT, text) else: - self._handle_cli_message(_MSG_PROGRESS, text) + self.handle_message(_MSG_PROGRESS, text) @_active_guard() def progress_bar( @@ -930,7 +930,7 @@ def _report_error(self, error: _ErrorMessage) -> None: # noqa: PLR0912 (too man def error(self, error: errors.CraftError) -> None: """Handle the system's indicated error and stop machinery.""" msg = _ErrorMessage.from_error(error) - self._handle_cli_message(_MSG_ERROR, msg.dumps()) + self.handle_message(_MSG_ERROR, msg.dumps()) self._stop() @_active_guard()