Refactor metadata directory handling (#7087)

src/pip/_internal/operations/generate_metadata.py

@@ -16,15 +16,21 @@
 
 
 def get_metadata_generator(install_req):
-    # type: (InstallRequirement) -> Callable[[InstallRequirement], None]
+    # type: (InstallRequirement) -> Callable[[InstallRequirement], str]
+    """Return a callable metadata generator for this InstallRequirement.
+
+    A metadata generator takes an InstallRequirement (install_req) as an input,
+    generates metadata via the appropriate process for that install_req and
+    returns the generated metadata directory.
+    """
     if not install_req.use_pep517:
         return _generate_metadata_legacy
 
     return _generate_metadata
 
 
 def _generate_metadata_legacy(install_req):
-    # type: (InstallRequirement) -> None
+    # type: (InstallRequirement) -> str
     req_details_str = install_req.name or "from {}".format(install_req.link)
     logger.debug(
         'Running setup.py (path:%s) egg_info for package %s',
@@ -56,7 +62,10 @@ def _generate_metadata_legacy(install_req):
             command_desc='python setup.py egg_info',
         )
 
+    # Return the metadata directory.
+    return install_req.find_egg_info()
+
 
 def _generate_metadata(install_req):
-    # type: (InstallRequirement) -> None
-    install_req.prepare_pep517_metadata()
+    # type: (InstallRequirement) -> str
+    return install_req.prepare_pep517_metadata()

src/pip/_internal/req/req_install.py

@@ -121,7 +121,6 @@ def __init__(
             markers = req.marker
         self.markers = markers
 
-        self._egg_info_path = None  # type: Optional[str]
         # This holds the pkg_resources.Distribution object if this requirement
         # is already available:
         self.satisfied_by = None
@@ -367,29 +366,34 @@ def move_to_correct_build_directory(self):
             return
         assert self.req is not None
         assert self._temp_build_dir
-        assert (self._ideal_build_dir is not None and
-                self._ideal_build_dir.path)  # type: ignore
+        assert (
+            self._ideal_build_dir is not None and
+            self._ideal_build_dir.path  # type: ignore
+        )
         old_location = self._temp_build_dir
-        self._temp_build_dir = None
+        self._temp_build_dir = None  # checked inside ensure_build_location
 
+        # Figure out the correct place to put the files.
         new_location = self.ensure_build_location(self._ideal_build_dir)
         if os.path.exists(new_location):
             raise InstallationError(
                 'A package already exists in %s; please remove it to continue'
-                % display_path(new_location))
+                % display_path(new_location)
+            )
+
+        # Move the files to the correct location.
         logger.debug(
             'Moving package %s from %s to new location %s',
             self, display_path(old_location.path), display_path(new_location),
         )
         shutil.move(old_location.path, new_location)
+
+        # Update directory-tracking variables, to be in line with new_location
+        self.source_dir = os.path.normpath(os.path.abspath(new_location))
         self._temp_build_dir = TempDirectory(
             path=new_location, kind="req-install",
         )
 
-        self._ideal_build_dir = None
-        self.source_dir = os.path.normpath(os.path.abspath(new_location))
-        self._egg_info_path = None
-
         # Correct the metadata directory, if it exists
         if self.metadata_directory:
             old_meta = self.metadata_directory
@@ -398,6 +402,11 @@ def move_to_correct_build_directory(self):
             new_meta = os.path.normpath(os.path.abspath(new_meta))
             self.metadata_directory = new_meta
 
+        # Done with any "move built files" work, since have moved files to the
+        # "ideal" build location. Setting to None allows to clearly flag that
+        # no more moves are needed.
+        self._ideal_build_dir = None
+
     def remove_temporary_source(self):
         # type: () -> None
         """Remove the source files from this requirement, if they are marked
@@ -568,7 +577,7 @@ def prepare_metadata(self):
 
         metadata_generator = get_metadata_generator(self)
         with indent_log():
-            metadata_generator(self)
+            self.metadata_directory = metadata_generator(self)
 
         if not self.req:
             if isinstance(parse_version(self.metadata["Version"]), Version):
@@ -595,7 +604,7 @@ def prepare_metadata(self):
                 self.req = Requirement(metadata_name)
 
     def prepare_pep517_metadata(self):
-        # type: () -> None
+        # type: () -> str
         assert self.pep517_backend is not None
 
         # NOTE: This needs to be refactored to stop using atexit
@@ -618,14 +627,10 @@ def prepare_pep517_metadata(self):
                 metadata_dir
             )
 
-        self.metadata_directory = os.path.join(metadata_dir, distinfo_dir)
+        return os.path.join(metadata_dir, distinfo_dir)
 
-    @property
-    def egg_info_path(self):
+    def find_egg_info(self):
         # type: () -> str
-        if self._egg_info_path is not None:
-            return self._egg_info_path
-
         def looks_like_virtual_env(path):
             return (
                 os.path.lexists(os.path.join(path, 'bin', 'python')) or
@@ -674,8 +679,7 @@ def depth_of_directory(dir_):
         if len(filenames) > 1:
             filenames.sort(key=depth_of_directory)
 
-        self._egg_info_path = os.path.join(base, filenames[0])
-        return self._egg_info_path
+        return os.path.join(base, filenames[0])
 
     @property
     def metadata(self):
@@ -688,16 +692,16 @@ def metadata(self):
     def get_dist(self):
         # type: () -> Distribution
         """Return a pkg_resources.Distribution for this requirement"""
-        if self.metadata_directory:
-            dist_dir = self.metadata_directory
-            dist_cls = pkg_resources.DistInfoDistribution
+        dist_dir = self.metadata_directory.rstrip(os.sep)
+
+        # Determine the correct Distribution object type.
+        if dist_dir.endswith(".egg-info"):
+            dist_cls = pkg_resources.Distribution
         else:
-            dist_dir = self.egg_info_path.rstrip(os.path.sep)
-            # https://github.com/python/mypy/issues/1174
-            dist_cls = pkg_resources.Distribution  # type: ignore
+            assert dist_dir.endswith(".dist-info")
+            dist_cls = pkg_resources.DistInfoDistribution
 
-        # dist_dir_name can be of the form "<project>.dist-info" or
-        # e.g. "<project>.egg-info".
+        # Build a PathMetadata object, from path to metadata. :wink:
         base_dir, dist_dir_name = os.path.split(dist_dir)
         dist_name = os.path.splitext(dist_dir_name)[0]
         metadata = pkg_resources.PathMetadata(base_dir, dist_dir)

tests/unit/test_req.py

@@ -438,7 +438,7 @@ def test_url_preserved_editable_req(self):
     ))
     def test_get_dist(self, path):
         req = install_req_from_line('foo')
-        req._egg_info_path = path
+        req.metadata_directory = path
         dist = req.get_dist()
         assert isinstance(dist, pkg_resources.Distribution)
         assert dist.project_name == 'foo'