From ba780276a207536bdc16a19fd531d2f8262629c1 Mon Sep 17 00:00:00 2001 From: Meng Xiangzhuo Date: Wed, 23 Oct 2024 00:22:23 +0800 Subject: [PATCH 1/4] feat: auto-create logs dir if it's absent --- freqtrade/loggers/__init__.py | 2 ++ tests/test_log_setup.py | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/freqtrade/loggers/__init__.py b/freqtrade/loggers/__init__.py index 1cc0590a1..ba0e23b5e 100644 --- a/freqtrade/loggers/__init__.py +++ b/freqtrade/loggers/__init__.py @@ -1,6 +1,7 @@ import logging from logging import Formatter from logging.handlers import RotatingFileHandler, SysLogHandler +from pathlib import Path from freqtrade.constants import Config from freqtrade.exceptions import OperationalException @@ -83,6 +84,7 @@ def setup_logging(config: Config) -> None: handler_jd.setFormatter(Formatter("%(name)s - %(levelname)s - %(message)s")) logging.root.addHandler(handler_jd) else: + Path(logfile).parent.mkdir(parents=True, exist_ok=True) handler_rf = get_existing_handlers(RotatingFileHandler) if handler_rf: logging.root.removeHandler(handler_rf) diff --git a/tests/test_log_setup.py b/tests/test_log_setup.py index 142134b34..19c6b190d 100644 --- a/tests/test_log_setup.py +++ b/tests/test_log_setup.py @@ -86,7 +86,7 @@ def test_set_loggers_Filehandler(tmp_path): logger = logging.getLogger() orig_handlers = logger.handlers logger.handlers = [] - logfile = tmp_path / "ft_logfile.log" + logfile = tmp_path / "logs/ft_logfile.log" config = { "verbosity": 2, "logfile": str(logfile), From 87c8e85068099b80cafe7f233e69668a0ef9c475 Mon Sep 17 00:00:00 2001 From: Meng Xiangzhuo Date: Fri, 25 Oct 2024 00:01:41 +0800 Subject: [PATCH 2/4] feat: add user friendly message on permission error --- freqtrade/loggers/__init__.py | 25 +++++++++++++++++++------ tests/test_log_setup.py | 21 +++++++++++++++++++++ 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/freqtrade/loggers/__init__.py b/freqtrade/loggers/__init__.py index ba0e23b5e..65162cb0e 100644 --- a/freqtrade/loggers/__init__.py +++ b/freqtrade/loggers/__init__.py @@ -1,4 +1,5 @@ import logging +import sys from logging import Formatter from logging.handlers import RotatingFileHandler, SysLogHandler from pathlib import Path @@ -84,15 +85,27 @@ def setup_logging(config: Config) -> None: handler_jd.setFormatter(Formatter("%(name)s - %(levelname)s - %(message)s")) logging.root.addHandler(handler_jd) else: - Path(logfile).parent.mkdir(parents=True, exist_ok=True) handler_rf = get_existing_handlers(RotatingFileHandler) if handler_rf: logging.root.removeHandler(handler_rf) - handler_rf = RotatingFileHandler( - logfile, - maxBytes=1024 * 1024 * 10, # 10Mb - backupCount=10, - ) + try: + logfile_path = Path(logfile) + logfile_path.parent.mkdir(parents=True, exist_ok=True) + handler_rf = RotatingFileHandler( + logfile_path, + maxBytes=1024 * 1024 * 10, # 10Mb + backupCount=10, + ) + except PermissionError: + logger.error( + f'Failed to create or access log file "{logfile_path.absolute()}". ' + "Please make sure you have the write permission to the log file or its parent " + "directories. If you're running freqtrade using docker, you see this error " + "message probably because you've logged in as the root user, please switch to " + "non-root user, delete and recreate the directories you need, and then try " + "again." + ) + sys.exit(1) handler_rf.setFormatter(Formatter(LOGFORMAT)) logging.root.addHandler(handler_rf) diff --git a/tests/test_log_setup.py b/tests/test_log_setup.py index 19c6b190d..e9cd8c342 100644 --- a/tests/test_log_setup.py +++ b/tests/test_log_setup.py @@ -107,6 +107,27 @@ def test_set_loggers_Filehandler(tmp_path): logger.handlers = orig_handlers +@pytest.mark.skipif(sys.platform == "win32", reason="does not run on windows") +def test_set_loggers_Filehandler_without_permission(tmp_path): + logger = logging.getLogger() + orig_handlers = logger.handlers + logger.handlers = [] + + tmp_path.chmod(0o400) + logfile = tmp_path / "logs/ft_logfile.log" + config = { + "verbosity": 2, + "logfile": str(logfile), + } + + setup_logging_pre() + with pytest.raises(SystemExit) as excinfo: + setup_logging(config) + assert excinfo.value.code == 1 + + logger.handlers = orig_handlers + + @pytest.mark.skip(reason="systemd is not installed on every system, so we're not testing this.") def test_set_loggers_journald(mocker): logger = logging.getLogger() From e7b0e3293d37e4e80b4f8af952f7955acdf11a6a Mon Sep 17 00:00:00 2001 From: Matthias Date: Fri, 25 Oct 2024 06:34:46 +0200 Subject: [PATCH 3/4] feat: Exit with exception, not with exit1 this aligns to how other parts of the code work - leaving "exit" to the outermost caller. --- freqtrade/loggers/__init__.py | 4 +--- tests/test_log_setup.py | 3 +-- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/freqtrade/loggers/__init__.py b/freqtrade/loggers/__init__.py index 65162cb0e..7e18d3cba 100644 --- a/freqtrade/loggers/__init__.py +++ b/freqtrade/loggers/__init__.py @@ -1,5 +1,4 @@ import logging -import sys from logging import Formatter from logging.handlers import RotatingFileHandler, SysLogHandler from pathlib import Path @@ -97,7 +96,7 @@ def setup_logging(config: Config) -> None: backupCount=10, ) except PermissionError: - logger.error( + raise OperationalException( f'Failed to create or access log file "{logfile_path.absolute()}". ' "Please make sure you have the write permission to the log file or its parent " "directories. If you're running freqtrade using docker, you see this error " @@ -105,7 +104,6 @@ def setup_logging(config: Config) -> None: "non-root user, delete and recreate the directories you need, and then try " "again." ) - sys.exit(1) handler_rf.setFormatter(Formatter(LOGFORMAT)) logging.root.addHandler(handler_rf) diff --git a/tests/test_log_setup.py b/tests/test_log_setup.py index e9cd8c342..ec267c85e 100644 --- a/tests/test_log_setup.py +++ b/tests/test_log_setup.py @@ -121,9 +121,8 @@ def test_set_loggers_Filehandler_without_permission(tmp_path): } setup_logging_pre() - with pytest.raises(SystemExit) as excinfo: + with pytest.raises(OperationalException): setup_logging(config) - assert excinfo.value.code == 1 logger.handlers = orig_handlers From c4cbf6de3b11d7d0fefbc6c3dcbecccea6035e36 Mon Sep 17 00:00:00 2001 From: Matthias Date: Fri, 25 Oct 2024 07:22:11 +0200 Subject: [PATCH 4/4] tests: reset permissions on tmp-path to facilitate cleanup --- tests/test_log_setup.py | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/tests/test_log_setup.py b/tests/test_log_setup.py index ec267c85e..6dc88b79d 100644 --- a/tests/test_log_setup.py +++ b/tests/test_log_setup.py @@ -113,18 +113,21 @@ def test_set_loggers_Filehandler_without_permission(tmp_path): orig_handlers = logger.handlers logger.handlers = [] - tmp_path.chmod(0o400) - logfile = tmp_path / "logs/ft_logfile.log" - config = { - "verbosity": 2, - "logfile": str(logfile), - } + try: + tmp_path.chmod(0o400) + logfile = tmp_path / "logs/ft_logfile.log" + config = { + "verbosity": 2, + "logfile": str(logfile), + } - setup_logging_pre() - with pytest.raises(OperationalException): - setup_logging(config) + setup_logging_pre() + with pytest.raises(OperationalException): + setup_logging(config) - logger.handlers = orig_handlers + logger.handlers = orig_handlers + finally: + tmp_path.chmod(0o700) @pytest.mark.skip(reason="systemd is not installed on every system, so we're not testing this.")