Skip to content
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

Introducing conda_cli, tmp_env, and path_factory fixtures #12592

Merged
merged 19 commits into from
May 16, 2023
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions conda/cli/main_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -345,9 +345,7 @@ def execute(args, parser):
if (
args.all or all(not getattr(args, opt) for opt in options)
) and not context.json:
stdout_logger = getLogger("conda.stdoutlog")
stdout_logger.info(get_main_info_str(info_dict))
stdout_logger.info("\n")
print(get_main_info_str(info_dict) + "\n")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting way to send messages presumably to stdout with stdout_logger instead of print. I wonder if that flexibility was ever used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about this change, the conda.stdoutlog logger behaves the same as print()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's effectively the same for the CLI.

I had to change it because the logging was resulting in the output getting collected differently so the new conda_cli didn't work correctly for run commands.

Per my understanding, an effort was previously undertaken to replace all print (to both stdout and stderr) with this kind of logging logic. My best guess is this was to potentially allow third-party tools (like navigator) to better redirect/interface with the text streams. This effort was never completed and the logging system has become over complicated as a result. (I'll ping the navigator team to check that this won't cause issues)


if args.envs:
from ..core.envs_manager import list_all_known_prefixes
Expand Down
10 changes: 3 additions & 7 deletions conda/cli/main_remove.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# Copyright (C) 2012 Anaconda, Inc
# SPDX-License-Identifier: BSD-3-Clause
import logging
import sys
from os.path import isfile, join

from ..base.context import context
Expand Down Expand Up @@ -55,12 +54,11 @@ def execute(args, parser):
if args.all:
if prefix == context.root_prefix:
raise CondaEnvironmentError(
"cannot remove root environment,\n"
" add -n NAME or -p PREFIX option"
"cannot remove root environment, add -n NAME or -p PREFIX option"
)
if not isfile(join(prefix, "conda-meta", "history")):
raise DirectoryNotACondaEnvironmentError(prefix)
print("\nRemove all packages in environment %s:\n" % prefix, file=sys.stderr)
print(f"\nRemove all packages in environment {prefix}:\n")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes stderr to stdout

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct, IMO the text printed does not belong in stderr:

$ conda create -n scratch python --yes
...
$ conda remove -n scratch --all --yes > /dev/null

Remove all packages in environment /Users/kodegard/.conda/arm64/23.1.0/3.10/envs/scratch:
$ conda create -n scratch python --yes
...
$ conda remove -n scratch --all 2> /dev/null

## Package Plan ##

  environment location: /Users/kodegard/.conda/arm64/23.1.0/3.10/envs/scratch


The following packages will be REMOVED:

  bzip2-1.0.8-h620ffc9_4
  ca-certificates-2023.01.10-hca03da5_0
  certifi-2022.12.7-py311hca03da5_0
  libffi-3.4.2-hca03da5_6
  ncurses-6.4-h313beb8_0
  openssl-1.1.1t-h1a28f6b_0
  pip-23.0.1-py311hca03da5_0
  python-3.11.2-hc0d8a6c_0
  readline-8.2-h1a28f6b_0
  setuptools-65.6.3-py311hca03da5_0
  sqlite-3.41.2-h80987f9_0
  tk-8.6.12-hb8d0fd4_0
  tzdata-2023c-h04d1e81_0
  wheel-0.38.4-py311hca03da5_0
  xz-5.2.10-h80987f9_1
  zlib-1.2.13-h5a0b063_0


Preparing transaction: done
Verifying transaction: done
Executing transaction: done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that seemed to be a bug


if "package_names" in args:
stp = PrefixSetup(
Expand All @@ -75,9 +73,7 @@ def execute(args, parser):
try:
handle_txn(txn, prefix, args, False, True)
except PackagesNotFoundError:
print(
"No packages found in %s. Continuing environment removal" % prefix
)
print(f"No packages found in {prefix}. Continuing environment removal")
if not context.dry_run:
rm_rf(prefix, clean_empty_parents=True)
unregister_env(prefix)
Expand Down
2 changes: 1 addition & 1 deletion conda/cli/main_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,4 @@ def execute(args, parser):
log = getLogger(__name__)
log.warning(f"CONDA_TEST_SAVE_TEMPS :: retaining main_run script {script}")

return response
return response.rc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is odd, wasn't the return code returned before? Might make things tricky for VS Code etc.

Copy link
Contributor Author

@kenodegard kenodegard Apr 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently no, we've been returning the entire Response object (which happens to be a custom NamedTuple?)

This should only impact downstream projects if they invoke conda run via Python (I'll ping the VSCode community to confirm)

This change ensures that all of the execute functions defined for our subcommands consistently return an int exit code (a good change IMO)

100 changes: 100 additions & 0 deletions conda/testing/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,23 @@
# Ideally we'd have two modes, 'removed' and 'fixed'. I have seen
# condabin come from an entirely different installation than
# CONDA_PREFIX too in some instances and that really needs fixing.
from __future__ import annotations

import os
import sys
import uuid
import warnings
from contextlib import contextmanager
from dataclasses import dataclass
from os.path import dirname, isfile, join, normpath
from pathlib import Path
from subprocess import check_output
from typing import Iterator

import pytest

from conda.base.context import context, reset_context
from conda.cli.main import init_loggers
from conda.common.compat import on_win

from ..deprecations import deprecated
Expand Down Expand Up @@ -156,3 +165,94 @@ def conda_check_versions_aligned():
)
with open(version_file, "w") as fh:
fh.write(version_from_git)


@dataclass
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a dataclass?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I had:

class CondaCLIFixture:
    def __init__(self, capsys: CaptureFixture):
        self.capsys = capsys

Which is what a dataclass does 🤷🏼‍♂️

class CondaCLIFixture:
capsys: CaptureFixture

def __call__(self, *argv: str, no_capture: bool = False) -> tuple[str, str, int]:
"""Mimic what is done in `conda.cli.main.main`"""
# drop the conda argument if provided
if argv[0] == "conda":
argv = argv[1:]

kenodegard marked this conversation as resolved.
Show resolved Hide resolved
# extra checks to handle legacy subcommands
if argv[0] == "env":
from conda_env.cli.main import create_parser as generate_parser
from conda_env.cli.main import do_call

argv = argv[1:]
else:
from conda.cli.conda_argparse import do_call, generate_parser

# ensure arguments are string
argv = tuple(map(str, argv))

# parse arguments
parser = generate_parser()
args = parser.parse_args(argv)

# initialize context and loggers
context.__init__(argparse_args=args)
init_loggers(context)

# run command
result = do_call(args, parser)
out, err = self.capsys.readouterr()

# restore to prior state
reset_context()

return out, err, result


@pytest.fixture
def conda_cli(capsys: CaptureFixture) -> CondaCLIFixture:
yield CondaCLIFixture(capsys)


@dataclass
class PathFactoryFixture:
tmp_path: Path

def __call__(
self,
name: str | None = None,
prefix: str | None = None,
suffix: str | None = None,
) -> Path:
prefix = prefix or ""
name = name or uuid.uuid4().hex
suffix = suffix or ""
return self.tmp_path / (prefix + name + suffix)


@pytest.fixture
def path_factory(tmp_path: Path) -> PathFactoryFixture:
yield PathFactoryFixture(tmp_path)


@dataclass
class TmpEnvFixture:
path_factory: PathFactoryFixture
conda_cli: CondaCLIFixture

@contextmanager
def __call__(
self,
*packages: str,
prefix: str | os.PathLike | None = None,
) -> Iterator[Path]:
prefix = Path(prefix or self.path_factory())

reset_context([prefix / "condarc"])
self.conda_cli("create", "--prefix", prefix, *packages, "--yes", "--quiet")
yield prefix


@pytest.fixture
def tmp_env(
path_factory: PathFactoryFixture, conda_cli: CondaCLIFixture
) -> TmpEnvFixture:
yield TmpEnvFixture(path_factory, conda_cli)
2 changes: 2 additions & 0 deletions conda/testing/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import pytest

from conda.auxlib.compat import shlex_split_unicode
from conda.deprecations import deprecated
from conda_env.cli import main as conda_env_cli

from .. import cli
Expand Down Expand Up @@ -124,6 +125,7 @@ def assert_in(a, b, output=""):
)


@deprecated("23.9", "24.3", addendum="Use `conda.testing.run` instead.")
def run_inprocess_conda_command(command, disallow_stderr: bool = True):
# anything that uses this function is an integration test
reset_context(())
Expand Down
14 changes: 4 additions & 10 deletions conda/testing/integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
from conda.common.url import escape_channel_url, path_to_url
from conda.core.package_cache_data import PackageCacheData
from conda.core.prefix_data import PrefixData
from conda.deprecations import deprecated
from conda.exceptions import conda_exception_handler
from conda.gateways.disk.create import mkdir_p
from conda.gateways.disk.delete import rm_rf
Expand Down Expand Up @@ -210,6 +211,7 @@ def temp_chdir(target_dir):
os.chdir(curdir)


@deprecated("23.9", "24.3", addendum="Use `conda.testing.run` instead.")
def run_command(command, prefix, *arguments, **kwargs):
assert isinstance(arguments, tuple), "run_command() arguments must be tuples"
arguments = massage_arguments(arguments)
Expand Down Expand Up @@ -278,21 +280,13 @@ def run_command(command, prefix, *arguments, **kwargs):
TEST_LOG_LEVEL, "requests"
):
arguments = encode_arguments(arguments)
is_run = arguments[0] == "run"
if is_run:
cap_args = (None, None)
with argv(["python_api"] + arguments), captured(*cap_args) as c:
if use_exception_handler:
result = conda_exception_handler(do_call, args, p)
else:
result = do_call(args, p)
if is_run:
stdout = result.stdout
stderr = result.stderr
result = result.rc
else:
stdout = c.stdout
stderr = c.stderr
stdout = c.stdout
stderr = c.stderr
print(stdout, file=sys.stdout)
print(stderr, file=sys.stderr)

Expand Down
21 changes: 21 additions & 0 deletions news/12592-conda_cli-fixture
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
### Enhancements

* Add `conda_cli` fixture to replace `conda.testing.helpers.run_inprocess_conda_command` and `conda.testing.integration.run_command`. (#12592)
* Add `tmp_env` fixture to replace `conda.testing.integration.make_temp_env`. (#12592)
* Add `path_factory` fixture to replace custom prefix logic like `conda.testing.integration._get_temp_prefix` and `conda.testing.integration.make_temp_prefix`. (#12592)

### Bug fixes

* <news item>

### Deprecations

* <news item>

### Docs

* <news item>

### Other

* <news item>
Loading