Better handling of conflicting --copies and --symlinks

docs/changelog/1784.bugfix.rst

@@ -0,0 +1,3 @@
+Better handling of optionlicting :option:`copies` and :option:`symlinks`. Introduce priority of where the option is set
+to follow the order: CLI, env var, file, hardcoded. If both set at same level prefers copy over symlink. - by
+user:`gaborbernat`.

src/virtualenv/__main__.py

@@ -1,11 +1,11 @@
 from __future__ import absolute_import, print_function, unicode_literals
 
-import argparse
 import logging
 import os
 import sys
 from datetime import datetime
 
+from virtualenv.config.cli.parser import VirtualEnvOptions
 from virtualenv.util.six import ensure_text
 
 
@@ -46,7 +46,7 @@ def __str__(self):
 
 
 def run_with_catch(args=None):
-    options = argparse.Namespace()
+    options = VirtualEnvOptions()
     try:
         run(args, options)
     except (KeyboardInterrupt, Exception) as exception:

src/virtualenv/config/cli/parser.py

@@ -1,6 +1,6 @@
 from __future__ import absolute_import, unicode_literals
 
-from argparse import SUPPRESS, ArgumentDefaultsHelpFormatter, ArgumentParser
+from argparse import SUPPRESS, ArgumentDefaultsHelpFormatter, ArgumentParser, Namespace
 from collections import OrderedDict
 
 from virtualenv.config.convert import get_type
@@ -9,6 +9,38 @@
 from ..ini import IniConfig
 
 
+class VirtualEnvOptions(Namespace):
+    def __init__(self, **kwargs):
+        super(VirtualEnvOptions, self).__init__(**kwargs)
+        self._src = None
+        self._sources = {}
+
+    def set_src(self, key, value, src):
+        setattr(self, key, value)
+        if src.startswith("env var"):
+            src = "env var"
+        self._sources[key] = src
+
+    def __setattr__(self, key, value):
+        if getattr(self, "_src", None) is not None:
+            self._sources[key] = self._src
+        super(VirtualEnvOptions, self).__setattr__(key, value)
+
+    def get_source(self, key):
+        return self._sources.get(key)
+
+    @property
+    def verbosity(self):
+        if not hasattr(self, "verbose") and not hasattr(self, "quiet"):
+            return None
+        return max(self.verbose - self.quiet, 0)
+
+    def __repr__(self):
+        return "{}({})".format(
+            type(self).__name__, ", ".join("{}={}".format(k, v) for k, v in vars(self).items() if not k.startswith("_"))
+        )
+
+
 class VirtualEnvConfigParser(ArgumentParser):
     """
     Custom option parser which updates its defaults by checking the configuration files and environmental variables
@@ -24,8 +56,9 @@ def __init__(self, options=None, *args, **kwargs):
         super(VirtualEnvConfigParser, self).__init__(*args, **kwargs)
         self._fixed = set()
         self._elements = None
-        self._verbosity = None
-        self._options = options
+        if options is not None and not isinstance(options, VirtualEnvOptions):
+            raise TypeError("options must be of type VirtualEnvOptions")
+        self.options = VirtualEnvOptions() if options is None else options
         self._interpreter = None
         self._app_data = None
 
@@ -52,18 +85,25 @@ def _fix_default(self, action):
                         break
             if outcome is not None:
                 action.default, action.default_source = outcome
+            else:
+                outcome = action.default, "default"
+            self.options.set_src(action.dest, *outcome)
 
     def enable_help(self):
         self._fix_defaults()
         self.add_argument("-h", "--help", action="help", default=SUPPRESS, help="show this help message and exit")
 
     def parse_known_args(self, args=None, namespace=None):
+        if namespace is None:
+            namespace = self.options
+        elif namespace is not self.options:
+            raise ValueError("can only pass in parser.options")
         self._fix_defaults()
-        return super(VirtualEnvConfigParser, self).parse_known_args(args, namespace=namespace)
-
-    def parse_args(self, args=None, namespace=None):
-        self._fix_defaults()
-        return super(VirtualEnvConfigParser, self).parse_args(args, namespace=namespace)
+        self.options._src = "cli"
+        try:
+            return super(VirtualEnvConfigParser, self).parse_known_args(args, namespace=namespace)
+        finally:
+            self.options._src = None
 
 
 class HelpFormatter(ArgumentDefaultsHelpFormatter):

src/virtualenv/create/via_global_ref/api.py

@@ -34,11 +34,26 @@ def can_symlink(self):
 class ViaGlobalRefApi(Creator):
     def __init__(self, options, interpreter):
         super(ViaGlobalRefApi, self).__init__(options, interpreter)
-        copies = getattr(options, "copies", False)
-        symlinks = getattr(options, "symlinks", False)
-        self.symlinks = symlinks is True and copies is False
+        self.symlinks = self._should_symlink(options)
         self.enable_system_site_package = options.system_site
 
+    @staticmethod
+    def _should_symlink(options):
+        # Priority of where the option is set to follow the order: CLI, env var, file, hardcoded.
+        # If both set at same level prefers copy over symlink.
+        copies, symlinks = getattr(options, "copies", False), getattr(options, "symlinks", False)
+        copy_src, sym_src = options.get_source("copies"), options.get_source("symlinks")
+        for level in ["cli", "env var", "file", "default"]:
+            s_opt = symlinks if sym_src == level else None
+            c_opt = copies if copy_src == level else None
+            if s_opt is True and c_opt is True:
+                return False
+            if s_opt is True:
+                return True
+            if c_opt is True:
+                return False
+        return False  # fallback to copy
+
     @classmethod
     def add_parser_arguments(cls, parser, interpreter, meta, app_data):
         super(ViaGlobalRefApi, cls).add_parser_arguments(parser, interpreter, meta, app_data)
@@ -50,6 +65,8 @@ def add_parser_arguments(cls, parser, interpreter, meta, app_data):
             help="give the virtual environment access to the system site-packages dir",
         )
         group = parser.add_mutually_exclusive_group()
+        if not meta.can_symlink and not meta.can_copy:
+            raise RuntimeError("neither symlink or copy method supported")
         if meta.can_symlink:
             group.add_argument(
                 "--symlinks",

src/virtualenv/report.py

@@ -18,8 +18,7 @@
 LOGGER = logging.getLogger()
 
 
-def setup_report(verbose, quiet):
-    verbosity = max(verbose - quiet, 0)
+def setup_report(verbosity):
     _clean_handlers(LOGGER)
     if verbosity > MAX_LEVEL:
         verbosity = MAX_LEVEL  # pragma: no cover

src/virtualenv/run/__init__.py

@@ -1,6 +1,5 @@
 from __future__ import absolute_import, unicode_literals
 
-import argparse
 import logging
 
 from virtualenv.run.app_data import AppDataAction
@@ -19,23 +18,21 @@ def cli_run(args, options=None):
     """Create a virtual environment given some command line interface arguments
 
     :param args: the command line arguments
-    :param options: passing in a ``argparse.Namespace`` object allows return of the parsed options
+    :param options: passing in a ``VirtualEnvOptions`` object allows return of the parsed options
     :return: the session object of the creation (its structure for now is experimental and might change on short notice)
     """
-    if options is None:
-        options = argparse.Namespace()
     session = session_via_cli(args, options)
     with session:
         session.run()
     return session
 
 
 # noinspection PyProtectedMember
-def session_via_cli(args, options):
+def session_via_cli(args, options=None):
     parser = build_parser(args, options)
-    parser.parse_args(args, namespace=parser._options)
-    creator, seeder, activators = tuple(e.create(parser._options) for e in parser._elements)  # create types
-    session = Session(parser._verbosity, options.app_data, parser._interpreter, creator, seeder, activators)
+    options = parser.parse_args(args)
+    creator, seeder, activators = tuple(e.create(options) for e in parser._elements)  # create types
+    session = Session(options.verbosity, options.app_data, parser._interpreter, creator, seeder, activators)
     return session
 
 
@@ -50,7 +47,7 @@ def build_parser(args=None, options=None):
         default=False,
         help="on failure also display the stacktrace internals of virtualenv",
     )
-    parser._options, parser._verbosity = _do_report_setup(parser, args)
+    _do_report_setup(parser, args)
     # here we need a write-able application data (e.g. the zipapp might need this for discovery cache)
     default_app_data = AppDataAction.default()
     parser.add_argument(
@@ -67,7 +64,7 @@ def build_parser(args=None, options=None):
         help="start with empty app data folder",
         default=False,
     )
-    discover = get_discover(parser, args, parser._options)
+    discover = get_discover(parser, args)
     parser._interpreter = interpreter = discover.interpreter
     if interpreter is None:
         raise RuntimeError("failed to find interpreter for {}".format(discover))
@@ -76,9 +73,9 @@ def build_parser(args=None, options=None):
         SeederSelector(interpreter, parser),
         ActivationSelector(interpreter, parser),
     ]
-    parser.parse_known_args(args, namespace=parser._options)
+    options, _ = parser.parse_known_args(args)
     for element in parser._elements:
-        element.handle_selected_arg_parse(parser._options)
+        element.handle_selected_arg_parse(options)
     parser.enable_help()
     return parser
 
@@ -103,6 +100,5 @@ def _do_report_setup(parser, args):
     verbosity = verbosity_group.add_mutually_exclusive_group()
     verbosity.add_argument("-v", "--verbose", action="count", dest="verbose", help="increase verbosity", default=2)
     verbosity.add_argument("-q", "--quiet", action="count", dest="quiet", help="decrease verbosity", default=0)
-    options, _ = parser.parse_known_args(args, namespace=parser._options)
-    verbosity_value = setup_report(options.verbose, options.quiet)
-    return options, verbosity_value
+    option, _ = parser.parse_known_args(args)
+    setup_report(option.verbosity)

src/virtualenv/run/plugin/discovery.py

@@ -9,7 +9,7 @@ class Discovery(PluginLoader):
     """"""
 
 
-def get_discover(parser, args, options):
+def get_discover(parser, args):
     discover_types = Discovery.entry_points_for("virtualenv.discovery")
     discovery_parser = parser.add_argument_group(
         title="discovery", description="discover and provide a target interpreter"
@@ -21,7 +21,7 @@ def get_discover(parser, args, options):
         required=False,
         help="interpreter discovery method",
     )
-    options, _ = parser.parse_known_args(args, namespace=options)
+    options, _ = parser.parse_known_args(args)
     if options.app_data == "<temp folder>":
         options.app_data = TempAppData()
     if options.clear_app_data:

tests/unit/config/test_env_var.py

@@ -1,7 +1,6 @@
 from __future__ import absolute_import, unicode_literals
 
 import os
-from argparse import Namespace
 
 import pytest
 
@@ -10,11 +9,6 @@
 from virtualenv.util.path import Path
 
 
-def parse_cli(args):
-    options = Namespace()
-    return session_via_cli(args, options)
-
-
 @pytest.fixture()
 def empty_conf(tmp_path, monkeypatch):
     conf = tmp_path / "conf.ini"
@@ -24,13 +18,13 @@ def empty_conf(tmp_path, monkeypatch):
 
 def test_value_ok(monkeypatch, empty_conf):
     monkeypatch.setenv(str("VIRTUALENV_VERBOSE"), str("5"))
-    result = parse_cli(["venv"])
+    result = session_via_cli(["venv"])
     assert result.verbosity == 5
 
 
 def test_value_bad(monkeypatch, caplog, empty_conf):
     monkeypatch.setenv(str("VIRTUALENV_VERBOSE"), str("a"))
-    result = parse_cli(["venv"])
+    result = session_via_cli(["venv"])
     assert result.verbosity == 2
     assert len(caplog.messages) == 1
     assert "env var VIRTUALENV_VERBOSE failed to convert" in caplog.messages[0]
@@ -44,7 +38,7 @@ def test_extra_search_dir_via_env_var(tmp_path, monkeypatch):
     (tmp_path / "a").mkdir()
     (tmp_path / "b").mkdir()
     (tmp_path / "c").mkdir()
-    result = parse_cli(["venv"])
+    result = session_via_cli(["venv"])
     assert result.seeder.extra_search_dir == [Path("a").resolve(), Path("b").resolve(), Path("c").resolve()]
 
 
@@ -65,5 +59,5 @@ def func(self, action):
     monkeypatch.delenv(str("SYMLINKS"), raising=False)
     monkeypatch.delenv(str("VIRTUALENV_COPIES"), raising=False)
     monkeypatch.setenv(str("VIRTUALENV_ALWAYS_COPY"), str("1"))
-    result = parse_cli(["venv"])
+    result = session_via_cli(["venv"])
     assert result.creator.symlinks is False

tests/unit/config/test_ini.py

@@ -0,0 +1,28 @@
+from __future__ import unicode_literals
+
+from textwrap import dedent
+
+import pytest
+
+from virtualenv.info import fs_supports_symlink
+from virtualenv.run import session_via_cli
+from virtualenv.util.six import ensure_str
+
+
+@pytest.mark.skipif(not fs_supports_symlink(), reason="symlink is not supported")
+def test_ini_can_be_overwritten_by_flag(tmp_path, monkeypatch):
+    custom_ini = tmp_path / "conf.ini"
+    custom_ini.write_text(
+        dedent(
+            """
+        [virtualenv]
+        copies = True
+        """
+        )
+    )
+    monkeypatch.setenv(ensure_str("VIRTUALENV_CONFIG_FILE"), str(custom_ini))
+
+    result = session_via_cli(["venv", "--symlinks"])
+
+    symlinks = result.creator.symlinks
+    assert symlinks is True

tests/unit/create/test_creator.py

@@ -349,7 +349,9 @@ def test_cross_major(cross_python, coverage_env, tmp_path, session_app_data, cur
 
 def test_create_parallel(tmp_path, monkeypatch, temp_app_data):
     def create(count):
-        subprocess.check_call([sys.executable, "-m", "virtualenv", str(tmp_path / "venv{}".format(count))])
+        subprocess.check_call(
+            [sys.executable, "-m", "virtualenv", "-vvv", str(tmp_path / "venv{}".format(count)), "--without-pip"]
+        )
 
     threads = [Thread(target=create, args=(i,)) for i in range(1, 4)]
     for thread in threads:
@@ -386,6 +388,7 @@ def test_create_long_path(current_fastest, tmp_path):
     subprocess.check_call([str(result.creator.script("pip")), "--version"])
 
 
+@pytest.mark.timeout(timeout=60)
 @pytest.mark.parametrize("creator", set(PythonInfo.current_system().creators().key_to_class) - {"builtin"})
 def test_create_distutils_cfg(creator, tmp_path, monkeypatch):
     result = cli_run([ensure_text(str(tmp_path / "venv")), "--activators", "", "--creator", creator])