-
Notifications
You must be signed in to change notification settings - Fork 10
Add file logger #218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add file logger #218
Changes from all commits
fafc282
21966cb
4228f20
bae7ca9
d70ede9
df6796c
3a53c13
2c48299
c3c6ff8
cf06a5c
16d82f2
302fcbf
eb3a64b
dbb27b2
6b4aeba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| ### Enhancements | ||
|
|
||
| * Add option to log `conda-standalone` and `conda` outputs into a log file | ||
| using the `--log-file` CLI option. (#218) | ||
|
|
||
| ### Bug fixes | ||
|
|
||
| * <news item> | ||
|
|
||
| ### Deprecations | ||
|
|
||
| * <news item> | ||
|
|
||
| ### Docs | ||
|
|
||
| * <news item> | ||
|
|
||
| ### Other | ||
|
|
||
| * <news item> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import logging | ||
| import os | ||
| import re | ||
| import sys | ||
|
|
@@ -22,6 +23,13 @@ | |
| from menuinst.cli.cli import install as install_shortcut | ||
| from ruamel.yaml import YAML | ||
|
|
||
| logger = logging.getLogger() | ||
| # On Windows, these warnings are expected because the uninstaller may still be | ||
| # accessing files (like install.log) that conda cannot rename. | ||
| if sys.platform == "win32": | ||
| conda_logger = logging.getLogger("conda.gateways.disk.delete") | ||
| conda_logger.addFilter(lambda record: "Could not remove or rename" not in record.getMessage()) | ||
|
|
||
|
|
||
| def _remove_file_directory(file: Path, raise_on_error: bool = False): | ||
| """ | ||
|
|
@@ -37,11 +45,14 @@ def _remove_file_directory(file: Path, raise_on_error: bool = False): | |
| elif file.is_symlink() or file.is_file(): | ||
| file.unlink() | ||
| except PermissionError as e: | ||
| message = ( | ||
| f"Could not remove {file}. " | ||
| "You may need to re-run with elevated privileges or manually remove this file." | ||
| ) | ||
| if raise_on_error: | ||
| raise PermissionError( | ||
| f"Could not remove {file}. " | ||
| "You may need to re-run with elevated privileges or manually remove this file." | ||
| ) from e | ||
| raise PermissionError(message) from e | ||
| else: | ||
| logger.warning(message, exc_info=e) | ||
|
|
||
|
|
||
| def _remove_config_file_and_parents(file: Path, raise_on_error: bool = False): | ||
|
|
@@ -163,7 +174,14 @@ def _run_conda_init_reverse(for_user: bool, prefix: Path, prefixes: list[Path]): | |
| # That function will search for activation scripts in sys.prefix which do no exist | ||
| # in the extraction directory of conda-standalone. | ||
| run_plan(plan) | ||
| run_plan_elevated(plan) | ||
| try: | ||
| run_plan_elevated(plan) | ||
| except Exception as exc: | ||
| logger.error( | ||
| "Could not revert some shell profiles because they require elevated privileges. " | ||
| "Check the output for lines with `needs sudo` and edit those files manually.", | ||
| exc_info=exc, | ||
| ) | ||
| print_plan_results(plan) | ||
| for initializer in plan: | ||
| target_path = initializer["kwargs"]["target_path"] | ||
|
|
@@ -202,7 +220,13 @@ def _remove_environments(prefix: Path, prefixes: list[Path]): | |
| # Unprotect frozen environments first | ||
| frozen_file = env_prefix / PREFIX_FROZEN_FILE | ||
| if frozen_file.is_file(): | ||
| _remove_file_directory(frozen_file, raise_on_error=True) | ||
| try: | ||
| _remove_file_directory(frozen_file, raise_on_error=True) | ||
| except PermissionError as e: | ||
| raise PermissionError( | ||
| f"Failed to unprotect '{env_prefix}'. Try to re-run the uninstallation with " | ||
| f"elevated privileges or remove the file '{frozen_file}' manually.", | ||
| ) from e | ||
|
|
||
| install_shortcut(env_prefix, root_prefix=str(menuinst_base_prefix), remove_shortcuts=[]) | ||
| # If conda_root_prefix is the same as prefix, conda remove will not be able | ||
|
|
@@ -214,7 +238,11 @@ def _remove_environments(prefix: Path, prefixes: list[Path]): | |
| if default_activation_prefix == env_prefix: | ||
| os.environ["CONDA_DEFAULT_ACTIVATION_ENV"] = sys.prefix | ||
| reset_context() | ||
| conda_main("remove", "-y", "-p", str(env_prefix), "--all") | ||
|
|
||
| return_code = conda_main("remove", "-y", "-p", str(env_prefix), "--all") | ||
| if return_code != 0: | ||
| raise RuntimeError(f"Failed to remove environment '{env_prefix}'.") | ||
|
|
||
| if conda_root_prefix and conda_root_prefix == env_prefix: | ||
| os.environ["CONDA_ROOT_PREFIX"] = str(conda_root_prefix) | ||
| reset_context() | ||
|
|
@@ -224,7 +252,9 @@ def _remove_environments(prefix: Path, prefixes: list[Path]): | |
|
|
||
|
|
||
| def _remove_caches(): | ||
| conda_main("clean", "--all", "-y") | ||
| return_code = conda_main("clean", "--all", "-y") | ||
| if return_code != 0: | ||
| logger.warning("Failed to remove all cache files.") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here (if we should add the return_code to the warning). Perhaps even less important since it's just a warning. |
||
| # Delete empty package cache directories | ||
| for directory in context.pkgs_dirs: | ||
| pkgs_dir = Path(directory) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -252,8 +252,10 @@ def test_conda_run(): | |||||||
| assert not process.stderr | ||||||||
|
|
||||||||
|
|
||||||||
| def test_conda_run_conda_exe(): | ||||||||
| @pytest.mark.parametrize("with_log", (True, False), ids=("with log", "no log")) | ||||||||
| def test_conda_run_conda_exe(tmp_path: Path, with_log: bool): | ||||||||
| env = os.environ.copy() | ||||||||
| log_file = tmp_path / "conda_run.log" | ||||||||
| for key in os.environ: | ||||||||
| if key.startswith(("CONDA", "_CONDA_", "__CONDA", "_CE_")): | ||||||||
| env.pop(key, None) | ||||||||
|
|
@@ -265,9 +267,18 @@ def test_conda_run_conda_exe(): | |||||||
| "python", | ||||||||
| "-c", | ||||||||
| "import sys,os;print(os.environ['CONDA_EXE'])", | ||||||||
| *(("--log-file", str(log_file)) if with_log else ()), | ||||||||
| check=True, | ||||||||
| text=True, | ||||||||
| capture_output=True, | ||||||||
| env=env, | ||||||||
| ) | ||||||||
| assert os.path.realpath(process.stdout.strip()) == os.path.realpath(CONDA_EXE) | ||||||||
| if with_log: | ||||||||
| assert log_file.exists() | ||||||||
| log_text = log_file.read_text().strip() | ||||||||
| if sys.platform.startswith("win") and os.environ.get("CI"): | ||||||||
| # on CI, setup-miniconda registers `test` as auto-activate for every CMD | ||||||||
| # which adds some unnecessary stderr output; so, only read the first line | ||||||||
| log_text = log_text.split("\n")[0] | ||||||||
| assert os.path.realpath(log_text.strip()) == os.path.realpath(CONDA_EXE) | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to assert whether the process had any output? All of it should have technically gone to the logfile, right?
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Output goes to both the screen and the log file, so we should still check the output. |
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this as import-time logic, or should it be a separate function we call as part of the CLI initialization? Is that too late in the process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it for the visibility, but you are right that it isn't strictly needed at import time. So, I went a third way: importing these main execution function isn't needed until we actually use it. So, I just moved the import statements into the
executefunction.