From f613a5a0dff19bb482011825fe2e40960e03abac Mon Sep 17 00:00:00 2001 From: zermelo-wisen Date: Wed, 19 Jun 2024 12:58:00 +0300 Subject: [PATCH] fix: appmap breaks python extension repl --- .travis.yml | 4 ---- _appmap/env.py | 31 +++++++++++++++++++++++++++++-- _appmap/test/test_env.py | 15 +++++++++++++++ ci/smoketest.sh | 22 ++++++++++++++++++++++ 4 files changed, 66 insertions(+), 6 deletions(-) diff --git a/.travis.yml b/.travis.yml index d54311ce..d01c36cd 100644 --- a/.travis.yml +++ b/.travis.yml @@ -3,10 +3,6 @@ dist: jammy language: python python: - "3.12" -- "3.11" -- "3.10" -- "3.9.14" -- "3.8" # https://github.com/travis-ci/travis-ci/issues/1147#issuecomment-441393807 if: type != push OR branch = master OR branch =~ /^v\d+\.\d+(\.\d+)?(-\S*)?$/ diff --git a/_appmap/env.py b/_appmap/env.py index ad1f7290..277ae76b 100644 --- a/_appmap/env.py +++ b/_appmap/env.py @@ -37,6 +37,7 @@ def __init__(self, env=None, cwd=None): else: self._env.pop(k, None) + self.log_file_creation_failed = False self._configure_logging() enabled = self._env.get("_APPMAP", "false") self._enabled = enabled is None or enabled.lower() != "false" @@ -133,9 +134,24 @@ def display_params(self): def getLogger(self, name) -> trace_logger.TraceLogger: return cast(trace_logger.TraceLogger, logging.getLogger(name)) + def determine_log_file(self): + log_file = "appmap.log" + + # Try creating the log file in the current directory + try: + with open(log_file, 'a', encoding='UTF8'): + pass + except IOError: + # The circumstances in which creation is going to fail + # are also those in which the user doesn't care whether + # there's a log file (e.g. when starting a REPL). + return None + return log_file + + def _configure_logging(self): trace_logger.install() - + log_level = self.get("APPMAP_LOG_LEVEL", "warn").upper() disable_log = os.environ.get("APPMAP_DISABLE_LOG_FILE", "false").upper() != "FALSE" log_config = self.get("APPMAP_LOG_CONFIG") @@ -179,13 +195,19 @@ def _configure_logging(self): log_level = self.get("APPMAP_LOG_LEVEL", "info").upper() loggers = config_dict["loggers"] loggers["appmap"]["level"] = loggers["_appmap"]["level"] = log_level + + log_file = self.determine_log_file() + # Use NullHandler if log_file is None to avoid complicating the configuration + # with the absence of the "default" handler. config_dict["handlers"] = { "default": { "class": "logging.handlers.RotatingFileHandler", "formatter": "default", - "filename": "appmap.log", + "filename": log_file, "maxBytes": 50 * 1024 * 1024, "backupCount": 1, + } if log_file is not None else { + "class": "logging.NullHandler" }, "stderr": { "class": "logging.StreamHandler", @@ -194,6 +216,7 @@ def _configure_logging(self): "stream": "ext://sys.stderr", }, } + self.log_file_creation_failed = log_file is None if log_config is not None: name, level = log_config.split("=", 2) @@ -213,3 +236,7 @@ def initialize(**kwargs): Env.reset(**kwargs) logger = logging.getLogger(__name__) logger.info("appmap enabled: %s", Env.current.enabled) + if Env.current.log_file_creation_failed: + # Writing to stderr makes the REPL fail in vscode-python. + # https://github.com/microsoft/vscode-python/blob/c71c85ebf3749d5fac76899feefb21ee321a4b5b/src/client/common/process/rawProcessApis.ts#L268-L269 + print("WARNING: appmap.log cannot be created.") diff --git a/_appmap/test/test_env.py b/_appmap/test/test_env.py index ff212fb7..715dadfc 100644 --- a/_appmap/test/test_env.py +++ b/_appmap/test/test_env.py @@ -1,3 +1,4 @@ +import os from _appmap.env import Env @@ -11,3 +12,17 @@ def test_disable_temporarily(): except RuntimeError: ... assert env.enables("requests") + +def test_determine_log_file(monkeypatch): + env = Env({"_APPMAP": "true"}) + + def mock_open(*args, **kwargs): + raise IOError("Testing exception") + monkeypatch.setattr("builtins.open", mock_open) + + log_file = env.determine_log_file() + # It should be in a temp folder + assert log_file != "appmap.log" + user_home_dir = os.path.expanduser("~") + assert log_file != os.path.join(user_home_dir, "appmap.log") + diff --git a/ci/smoketest.sh b/ci/smoketest.sh index f719533f..e0df297d 100755 --- a/ci/smoketest.sh +++ b/ci/smoketest.sh @@ -19,6 +19,26 @@ EOF fi } +test_log_file_not_writable() +{ + touch appmap.log + chmod 400 appmap.log + + cat < test_log_file_not_writable.py +from appmap import Recording + +EOF + + python test_log_file_not_writable.py + + if [[ $? -eq 0 ]]; then + echo 'Script executed successfully' + else + echo 'Script execution failed' + exit 1 + fi +} + set -ex pip -q install -U pip pytest "flask>=2,<3" python-decouple pip -q install /dist/appmap-*-py3-none-any.whl @@ -33,6 +53,8 @@ test_recording_when_appmap_not_true export APPMAP=true +test_log_file_not_creatable + python -m appmap.command.appmap_agent_init |\ python -c 'import json,sys; print(json.load(sys.stdin)["configuration"]["contents"])' > /tmp/appmap.yml cat /tmp/appmap.yml