Merge pull request #7175 from chrahunt/refactor/centralize-setuptools-args

Centralize setuptools args construction

Author: chrahunt

src/pip/_internal/operations/generate_metadata.py

@@ -6,13 +6,14 @@
 
 from pip._internal.exceptions import InstallationError
 from pip._internal.utils.misc import ensure_dir
-from pip._internal.utils.setuptools_build import make_setuptools_shim_args
+from pip._internal.utils.setuptools_build import make_setuptools_egg_info_args
 from pip._internal.utils.subprocess import call_subprocess
 from pip._internal.utils.typing import MYPY_CHECK_RUNNING
 from pip._internal.vcs import vcs
 
 if MYPY_CHECK_RUNNING:
-    from typing import Callable, List
+    from typing import Callable, List, Optional
+
     from pip._internal.req.req_install import InstallRequirement
 
 logger = logging.getLogger(__name__)
@@ -99,27 +100,27 @@ def _generate_metadata_legacy(install_req):
         install_req.setup_py_path, req_details_str,
     )
 
-    # Compose arguments for subprocess call
-    base_cmd = make_setuptools_shim_args(install_req.setup_py_path)
-    if install_req.isolated:
-        base_cmd += ["--no-user-cfg"]
-
+    egg_info_dir = None  # type: Optional[str]
     # For non-editable installs, don't put the .egg-info files at the root,
     # to avoid confusion due to the source code being considered an installed
     # egg.
-    egg_base_option = []  # type: List[str]
     if not install_req.editable:
         egg_info_dir = os.path.join(
             install_req.unpacked_source_directory, 'pip-egg-info',
         )
-        egg_base_option = ['--egg-base', egg_info_dir]
 
         # setuptools complains if the target directory does not exist.
         ensure_dir(egg_info_dir)
 
+    args = make_setuptools_egg_info_args(
+        install_req.setup_py_path,
+        egg_info_dir=egg_info_dir,
+        no_user_config=install_req.isolated,
+    )
+
     with install_req.build_env:
         call_subprocess(
-            base_cmd + ["egg_info"] + egg_base_option,
+            args,
             cwd=install_req.unpacked_source_directory,
             command_desc='python setup.py egg_info',
         )

src/pip/_internal/req/req_install.py

@@ -48,7 +48,10 @@
     rmtree,
 )
 from pip._internal.utils.packaging import get_metadata
-from pip._internal.utils.setuptools_build import make_setuptools_shim_args
+from pip._internal.utils.setuptools_build import (
+    make_setuptools_develop_args,
+    make_setuptools_install_args,
+)
 from pip._internal.utils.subprocess import (
     call_subprocess,
     runner_with_spinner_message,
@@ -690,20 +693,18 @@ def install_editable(
         # type: (...) -> None
         logger.info('Running setup.py develop for %s', self.name)
 
-        if prefix:
-            prefix_param = ['--prefix={}'.format(prefix)]
-            install_options = list(install_options) + prefix_param
-        base_cmd = make_setuptools_shim_args(
+        args = make_setuptools_develop_args(
             self.setup_py_path,
             global_options=global_options,
-            no_user_config=self.isolated
+            install_options=install_options,
+            no_user_config=self.isolated,
+            prefix=prefix,
         )
+
         with indent_log():
             with self.build_env:
                 call_subprocess(
-                    base_cmd +
-                    ['develop', '--no-deps'] +
-                    list(install_options),
+                    args,
                     cwd=self.unpacked_source_directory,
                 )
 
@@ -880,18 +881,33 @@ def install(
         install_options = list(install_options) + \
             self.options.get('install_options', [])
 
+        header_dir = None  # type: Optional[str]
+        if running_under_virtualenv():
+            py_ver_str = 'python' + sysconfig.get_python_version()
+            header_dir = os.path.join(
+                sys.prefix, 'include', 'site', py_ver_str, self.name
+            )
+
         with TempDirectory(kind="record") as temp_dir:
             record_filename = os.path.join(temp_dir.path, 'install-record.txt')
-            install_args = self.get_install_args(
-                global_options, record_filename, root, prefix, pycompile,
+            install_args = make_setuptools_install_args(
+                self.setup_py_path,
+                global_options=global_options,
+                install_options=install_options,
+                record_filename=record_filename,
+                root=root,
+                prefix=prefix,
+                header_dir=header_dir,
+                no_user_config=self.isolated,
+                pycompile=pycompile,
             )
 
             runner = runner_with_spinner_message(
                 "Running setup.py install for {}".format(self.name)
             )
             with indent_log(), self.build_env:
                 runner(
-                    cmd=install_args + install_options,
+                    cmd=install_args,
                     cwd=self.unpacked_source_directory,
                 )
 
@@ -935,39 +951,3 @@ def prepend_root(path):
             inst_files_path = os.path.join(egg_info_dir, 'installed-files.txt')
             with open(inst_files_path, 'w') as f:
                 f.write('\n'.join(new_lines) + '\n')
-
-    def get_install_args(
-        self,
-        global_options,  # type: Sequence[str]
-        record_filename,  # type: str
-        root,  # type: Optional[str]
-        prefix,  # type: Optional[str]
-        pycompile  # type: bool
-    ):
-        # type: (...) -> List[str]
-        install_args = make_setuptools_shim_args(
-            self.setup_py_path,
-            global_options=global_options,
-            no_user_config=self.isolated,
-            unbuffered_output=True
-        )
-        install_args += ['install', '--record', record_filename]
-        install_args += ['--single-version-externally-managed']
-
-        if root is not None:
-            install_args += ['--root', root]
-        if prefix is not None:
-            install_args += ['--prefix', prefix]
-
-        if pycompile:
-            install_args += ["--compile"]
-        else:
-            install_args += ["--no-compile"]
-
-        if running_under_virtualenv():
-            py_ver_str = 'python' + sysconfig.get_python_version()
-            install_args += ['--install-headers',
-                             os.path.join(sys.prefix, 'include', 'site',
-                                          py_ver_str, self.name)]
-
-        return install_args

src/pip/_internal/utils/setuptools_build.py

@@ -3,7 +3,7 @@
 from pip._internal.utils.typing import MYPY_CHECK_RUNNING
 
 if MYPY_CHECK_RUNNING:
-    from typing import List, Sequence
+    from typing import List, Optional, Sequence
 
 # Shim to wrap setup.py invocation with setuptools
 #
@@ -21,10 +21,10 @@
 
 
 def make_setuptools_shim_args(
-        setup_py_path,  # type: str
-        global_options=None,  # type: Sequence[str]
-        no_user_config=False,  # type: bool
-        unbuffered_output=False  # type: bool
+    setup_py_path,  # type: str
+    global_options=None,  # type: Sequence[str]
+    no_user_config=False,  # type: bool
+    unbuffered_output=False  # type: bool
 ):
     # type: (...) -> List[str]
     """
@@ -38,10 +38,129 @@ def make_setuptools_shim_args(
     """
     args = [sys.executable]
     if unbuffered_output:
-        args.append('-u')
-    args.extend(['-c', _SETUPTOOLS_SHIM.format(setup_py_path)])
+        args += ["-u"]
+    args += ["-c", _SETUPTOOLS_SHIM.format(setup_py_path)]
     if global_options:
-        args.extend(global_options)
+        args += global_options
     if no_user_config:
-        args.append('--no-user-cfg')
+        args += ["--no-user-cfg"]
+    return args
+
+
+def make_setuptools_bdist_wheel_args(
+    setup_py_path,  # type: str
+    global_options,  # type: Sequence[str]
+    build_options,  # type: Sequence[str]
+    destination_dir,  # type: str
+    python_tag,  # type: Optional[str]
+):
+    # type: (...) -> List[str]
+    # NOTE: Eventually, we'd want to also -S to the flags here, when we're
+    # isolating. Currently, it breaks Python in virtualenvs, because it
+    # relies on site.py to find parts of the standard library outside the
+    # virtualenv.
+    args = make_setuptools_shim_args(
+        setup_py_path,
+        global_options=global_options,
+        unbuffered_output=True
+    )
+    args += ["bdist_wheel", "-d", destination_dir]
+    args += build_options
+    if python_tag is not None:
+        args += ["--python-tag", python_tag]
+    return args
+
+
+def make_setuptools_clean_args(
+    setup_py_path,  # type: str
+    global_options,  # type: Sequence[str]
+):
+    # type: (...) -> List[str]
+    args = make_setuptools_shim_args(
+        setup_py_path,
+        global_options=global_options,
+        unbuffered_output=True
+    )
+    args += ["clean", "--all"]
+    return args
+
+
+def make_setuptools_develop_args(
+    setup_py_path,  # type: str
+    global_options,  # type: Sequence[str]
+    install_options,  # type: Sequence[str]
+    no_user_config,  # type: bool
+    prefix,  # type: Optional[str]
+):
+    # type: (...) -> List[str]
+    args = make_setuptools_shim_args(
+        setup_py_path,
+        global_options=global_options,
+        no_user_config=no_user_config,
+    )
+
+    args += ["develop", "--no-deps"]
+
+    args += install_options
+
+    if prefix:
+        args += ["--prefix", prefix]
+
+    return args
+
+
+def make_setuptools_egg_info_args(
+    setup_py_path,  # type: str
+    egg_info_dir,  # type: Optional[str]
+    no_user_config,  # type: bool
+):
+    # type: (...) -> List[str]
+    args = make_setuptools_shim_args(setup_py_path)
+    if no_user_config:
+        args += ["--no-user-cfg"]
+
+    args += ["egg_info"]
+
+    if egg_info_dir:
+        args += ["--egg-base", egg_info_dir]
+
+    return args
+
+
+def make_setuptools_install_args(
+    setup_py_path,  # type: str
+    global_options,  # type: Sequence[str]
+    install_options,  # type: Sequence[str]
+    record_filename,  # type: str
+    root,  # type: Optional[str]
+    prefix,  # type: Optional[str]
+    header_dir,  # type: Optional[str]
+    no_user_config,  # type: bool
+    pycompile  # type: bool
+):
+    # type: (...) -> List[str]
+    args = make_setuptools_shim_args(
+        setup_py_path,
+        global_options=global_options,
+        no_user_config=no_user_config,
+        unbuffered_output=True
+    )
+    args += ["install", "--record", record_filename]
+    args += ["--single-version-externally-managed"]
+
+    if root is not None:
+        args += ["--root", root]
+    if prefix is not None:
+        args += ["--prefix", prefix]
+
+    if pycompile:
+        args += ["--compile"]
+    else:
+        args += ["--no-compile"]
+
+    if header_dir:
+        args += ["--install-headers", header_dir]
+
+    args += install_options
+
     return args

src/pip/_internal/wheel.py

@@ -39,7 +39,10 @@
 from pip._internal.utils.logging import indent_log
 from pip._internal.utils.marker_files import has_delete_marker_file
 from pip._internal.utils.misc import captured_stdout, ensure_dir, read_chunks
-from pip._internal.utils.setuptools_build import make_setuptools_shim_args
+from pip._internal.utils.setuptools_build import (
+    make_setuptools_bdist_wheel_args,
+    make_setuptools_clean_args,
+)
 from pip._internal.utils.subprocess import (
     LOG_DIVIDER,
     call_subprocess,
@@ -957,17 +960,6 @@ def _build_one_inside_env(self, req, output_dir, python_tag=None):
             self._clean_one(req)
             return None
 
-    def _base_setup_args(self, req):
-        # NOTE: Eventually, we'd want to also -S to the flags here, when we're
-        # isolating. Currently, it breaks Python in virtualenvs, because it
-        # relies on site.py to find parts of the standard library outside the
-        # virtualenv.
-        return make_setuptools_shim_args(
-            req.setup_py_path,
-            global_options=self.global_options,
-            unbuffered_output=True
-        )
-
     def _build_one_pep517(self, req, tempd, python_tag=None):
         """Build one InstallRequirement using the PEP 517 build process.
 
@@ -1012,16 +1004,17 @@ def _build_one_legacy(self, req, tempd, python_tag=None):
 
         Returns path to wheel if successfully built. Otherwise, returns None.
         """
-        base_args = self._base_setup_args(req)
+        wheel_args = make_setuptools_bdist_wheel_args(
+            req.setup_py_path,
+            global_options=self.global_options,
+            build_options=self.build_options,
+            destination_dir=tempd,
+            python_tag=python_tag,
+        )
 
         spin_message = 'Building wheel for %s (setup.py)' % (req.name,)
         with open_spinner(spin_message) as spinner:
             logger.debug('Destination directory: %s', tempd)
-            wheel_args = base_args + ['bdist_wheel', '-d', tempd] \
-                + self.build_options
-
-            if python_tag is not None:
-                wheel_args += ["--python-tag", python_tag]
 
             try:
                 output = call_subprocess(
@@ -1045,10 +1038,12 @@ def _build_one_legacy(self, req, tempd, python_tag=None):
             return wheel_path
 
     def _clean_one(self, req):
-        base_args = self._base_setup_args(req)
+        clean_args = make_setuptools_clean_args(
+            req.setup_py_path,
+            global_options=self.global_options,
+        )
 
         logger.info('Running setup.py clean for %s', req.name)
-        clean_args = base_args + ['clean', '--all']
         try:
             call_subprocess(clean_args, cwd=req.source_dir)
             return True