Fixing review comments

Signed-off-by: Lalatendu Mohanty <lmohanty@redhat.com>

Author: LalatenduMohanty

src/fromager/bootstrapper.py

@@ -51,9 +51,9 @@ class SourceBuildResult:
     wheel_filename: pathlib.Path | None
     sdist_filename: pathlib.Path | None
     unpack_dir: pathlib.Path
-    sdist_root_dir: pathlib.Path
-    build_env: build_environment.BuildEnvironment
-    source_url_type: str
+    sdist_root_dir: pathlib.Path | None
+    build_env: build_environment.BuildEnvironment | None
+    source_type: SourceType
 
 
 class Bootstrapper:
@@ -190,17 +190,9 @@ def bootstrap(self, req: Requirement, req_type: RequirementType) -> Version:
         # we're given.
         self.why.append((req_type, req, resolved_version))
 
-        # for cleanup
-        build_env: build_environment.BuildEnvironment | None = None
-        sdist_root_dir: pathlib.Path | None = None
         cached_wheel_filename: pathlib.Path | None = None
-        wheel_filename: pathlib.Path | None = None
-        sdist_filename: pathlib.Path | None = None
-        unpack_dir: pathlib.Path | None = None
         unpacked_cached_wheel: pathlib.Path | None = None
 
-        source_url_type = sources.get_source_type(self.ctx, req)
-
         if pbi.pre_built:
             wheel_filename, unpack_dir = self._download_prebuilt(
                 req=req,
@@ -209,10 +201,14 @@ def bootstrap(self, req: Requirement, req_type: RequirementType) -> Version:
                 wheel_url=source_url,
             )
             # Remember that this is a prebuilt wheel, and where we got it.
-            source_url_type = str(SourceType.PREBUILT)
-            sdist_filename = None
-            sdist_root_dir = None
-            build_env = None
+            build_result = SourceBuildResult(
+                wheel_filename=wheel_filename,
+                sdist_filename=None,
+                unpack_dir=unpack_dir,
+                sdist_root_dir=None,
+                build_env=None,
+                source_type=SourceType.PREBUILT,
+            )
         else:
             # Look for an existing wheel in caches (3 levels: build, downloads,
             # cache server) before building from source.
@@ -230,31 +226,23 @@ def bootstrap(self, req: Requirement, req_type: RequirementType) -> Version:
                 unpacked_cached_wheel=unpacked_cached_wheel,
             )
 
-            # Unpack result
-            wheel_filename = build_result.wheel_filename
-            sdist_filename = build_result.sdist_filename
-            unpack_dir = build_result.unpack_dir
-            sdist_root_dir = build_result.sdist_root_dir
-            build_env = build_result.build_env
-            source_url_type = build_result.source_url_type
-
         hooks.run_post_bootstrap_hooks(
             ctx=self.ctx,
             req=req,
             dist_name=canonicalize_name(req.name),
             dist_version=str(resolved_version),
-            sdist_filename=sdist_filename,
-            wheel_filename=wheel_filename,
+            sdist_filename=build_result.sdist_filename,
+            wheel_filename=build_result.wheel_filename,
         )
 
         install_dependencies = self._get_install_dependencies(
             req=req,
             resolved_version=resolved_version,
-            wheel_filename=wheel_filename,
-            sdist_filename=sdist_filename,
-            sdist_root_dir=sdist_root_dir,
-            build_env=build_env,
-            unpack_dir=unpack_dir,
+            wheel_filename=build_result.wheel_filename,
+            sdist_filename=build_result.sdist_filename,
+            sdist_root_dir=build_result.sdist_root_dir,
+            build_env=build_result.build_env,
+            unpack_dir=build_result.unpack_dir,
         )
 
         logger.debug(
@@ -266,7 +254,7 @@ def bootstrap(self, req: Requirement, req_type: RequirementType) -> Version:
             req=req,
             version=resolved_version,
             source_url=source_url,
-            source_url_type=source_url_type,
+            source_type=build_result.source_type,
             prebuilt=pbi.pre_built,
             constraint=constraint,
         )
@@ -282,7 +270,7 @@ def bootstrap(self, req: Requirement, req_type: RequirementType) -> Version:
 
         # we are done processing this req, so lets remove it from the why chain
         self.why.pop()
-        self.ctx.clean_build_dirs(sdist_root_dir, build_env)
+        self.ctx.clean_build_dirs(build_result.sdist_root_dir, build_result.build_env)
         return resolved_version
 
     @property
@@ -606,15 +594,15 @@ def _build_from_source(
                 req, resolved_version, sdist_root_dir, build_env
             )
 
-        source_url_type = sources.get_source_type(self.ctx, req)
+        source_type = sources.get_source_type(self.ctx, req)
 
         return SourceBuildResult(
             wheel_filename=wheel_filename,
             sdist_filename=sdist_filename,
             unpack_dir=unpack_dir,
             sdist_root_dir=sdist_root_dir,
             build_env=build_env,
-            source_url_type=source_url_type,
+            source_type=source_type,
         )
 
     def _look_for_existing_wheel(
@@ -1108,7 +1096,7 @@ def _add_to_build_order(
         req: Requirement,
         version: Version,
         source_url: str,
-        source_url_type: str,
+        source_type: SourceType,
         prebuilt: bool = False,
         constraint: Requirement | None = None,
     ) -> None:
@@ -1129,7 +1117,7 @@ def _add_to_build_order(
             "version": str(version),
             "prebuilt": prebuilt,
             "source_url": source_url,
-            "source_url_type": source_url_type,
+            "source_url_type": str(source_type),
         }
         if req.url:
             info["source_url"] = req.url

src/fromager/sources.py

@@ -27,34 +27,33 @@
     overrides,
     packagesettings,
     pyproject,
-    requirements_file,
     resolver,
     tarballs,
     vendor_rust,
 )
 from .http_retry import RETRYABLE_EXCEPTIONS, retry_on_exception
 from .request_session import session
-from .requirements_file import RequirementType
+from .requirements_file import RequirementType, SourceType
 
 if typing.TYPE_CHECKING:
     from . import build_environment, context
 
 logger = logging.getLogger(__name__)
 
 
-def get_source_type(ctx: context.WorkContext, req: Requirement) -> str:
-    source_type = requirements_file.SourceType.SDIST
+def get_source_type(ctx: context.WorkContext, req: Requirement) -> SourceType:
+    source_type = SourceType.SDIST
     if req.url:
-        return requirements_file.SourceType.GIT
+        return SourceType.GIT
     pbi = ctx.package_build_info(req)
     if (
         overrides.find_override_method(req.name, "download_source")
         or overrides.find_override_method(req.name, "resolve_source")
         or overrides.find_override_method(req.name, "get_resolver_provider")
         or pbi.download_source_url(resolve_template=False)
     ):
-        source_type = requirements_file.SourceType.OVERRIDE
-    return str(source_type)
+        source_type = SourceType.OVERRIDE
+    return source_type
 
 
 @metrics.timeit(description="download source")

tests/test_bootstrapper.py

@@ -9,7 +9,7 @@
 from fromager import bootstrapper
 from fromager.context import WorkContext
 from fromager.dependency_graph import DependencyGraph
-from fromager.requirements_file import RequirementType
+from fromager.requirements_file import RequirementType, SourceType
 
 old_graph = DependencyGraph()
 
@@ -290,13 +290,13 @@ def test_build_order(tmp_context: WorkContext) -> None:
         req=Requirement("buildme>1.0"),
         version=Version("6.0"),
         source_url="url",
-        source_url_type="sdist",
+        source_type=SourceType.SDIST,
     )
     bt._add_to_build_order(
         req=Requirement("testdist>1.0"),
         version=Version("1.2"),
         source_url="url",
-        source_url_type="sdist",
+        source_type=SourceType.SDIST,
     )
     contents_str = bt._build_order_filename.read_text()
     contents = json.loads(contents_str)
@@ -329,19 +329,19 @@ def test_build_order_repeats(tmp_context: WorkContext) -> None:
         Requirement("buildme>1.0"),
         Version("6.0"),
         "url",
-        "sdist",
+        SourceType.SDIST,
     )
     bt._add_to_build_order(
         Requirement("buildme>1.0"),
         Version("6.0"),
         "url",
-        "sdist",
+        SourceType.SDIST,
     )
     bt._add_to_build_order(
         Requirement("buildme[extra]>1.0"),
         Version("6.0"),
         "url",
-        "sdist",
+        SourceType.SDIST,
     )
     contents_str = bt._build_order_filename.read_text()
     contents = json.loads(contents_str)
@@ -365,13 +365,13 @@ def test_build_order_name_canonicalization(tmp_context: WorkContext) -> None:
         Requirement("flit-core>1.0"),
         Version("3.9.0"),
         "url",
-        "sdist",
+        SourceType.SDIST,
     )
     bt._add_to_build_order(
         Requirement("flit_core>1.0"),
         Version("3.9.0"),
         "url",
-        "sdist",
+        SourceType.SDIST,
     )
     contents_str = bt._build_order_filename.read_text()
     contents = json.loads(contents_str)
@@ -447,50 +447,47 @@ def test_find_cached_wheel_returns_tuple(tmp_context: WorkContext) -> None:
     assert len(result) == 2
 
 
-def test_get_install_dependencies_returns_list(tmp_context: WorkContext) -> None:
+@patch("fromager.dependencies.get_install_dependencies_of_wheel", return_value=set())
+def test_get_install_dependencies_returns_list(
+    mock_get_deps: Mock, tmp_context: WorkContext
+) -> None:
     """Verify _get_install_dependencies returns list."""
     bt = bootstrapper.Bootstrapper(tmp_context)
 
     # Create fake wheel file and mock dependencies
     wheel_file = pathlib.Path("/fake/package-1.0.0-py3-none-any.whl")
     unpack_dir = tmp_context.work_dir
 
-    # Mock the dependency extraction to return empty set
-    import fromager.dependencies as deps
-
-    original_func = deps.get_install_dependencies_of_wheel
-    deps.get_install_dependencies_of_wheel = Mock(return_value=set())
-
-    try:
-        result = bt._get_install_dependencies(
-            req=Requirement("test-package"),
-            resolved_version=Version("1.0.0"),
-            wheel_filename=wheel_file,
-            sdist_filename=None,
-            sdist_root_dir=None,
-            build_env=None,
-            unpack_dir=unpack_dir,
-        )
+    result = bt._get_install_dependencies(
+        req=Requirement("test-package"),
+        resolved_version=Version("1.0.0"),
+        wheel_filename=wheel_file,
+        sdist_filename=None,
+        sdist_root_dir=None,
+        build_env=None,
+        unpack_dir=unpack_dir,
+    )
 
-        # Verify return type is list
-        assert isinstance(result, list)
-    finally:
-        deps.get_install_dependencies_of_wheel = original_func
+    # Verify return type is list
+    assert isinstance(result, list)
+    # Verify the mocked function was called
+    mock_get_deps.assert_called_once()
 
 
 def test_build_from_source_returns_dataclass(tmp_context: WorkContext) -> None:
-    """Verify _build_from_source returns SourceBuildResult."""
+    """Verify _build_from_source returns SourceBuildResult with correct values."""
     bt = bootstrapper.Bootstrapper(tmp_context)
 
     mock_sdist_root = tmp_context.work_dir / "package-1.0.0" / "package-1.0.0"
     mock_sdist_root.parent.mkdir(parents=True, exist_ok=True)
     mock_source_file = tmp_context.work_dir / "package-1.0.0.tar.gz"
     mock_wheel = tmp_context.work_dir / "package-1.0.0-py3-none-any.whl"
+    expected_unpack_dir = mock_sdist_root.parent
 
     with (
         patch("fromager.sources.download_source", return_value=mock_source_file),
         patch("fromager.sources.prepare_source", return_value=mock_sdist_root),
-        patch("fromager.sources.get_source_type", return_value="sdist"),
+        patch("fromager.sources.get_source_type", return_value=SourceType.SDIST),
         patch.object(bt, "_prepare_build_dependencies"),
         patch.object(bt, "_build_wheel", return_value=(mock_wheel, None)),
     ):
@@ -506,10 +503,10 @@ def test_build_from_source_returns_dataclass(tmp_context: WorkContext) -> None:
         # Verify return type is SourceBuildResult
         assert isinstance(result, bootstrapper.SourceBuildResult)
 
-        # Verify all expected fields are present
-        assert hasattr(result, "wheel_filename")
-        assert hasattr(result, "sdist_filename")
-        assert hasattr(result, "unpack_dir")
-        assert hasattr(result, "sdist_root_dir")
-        assert hasattr(result, "build_env")
-        assert hasattr(result, "source_url_type")
+        # Verify all expected fields have correct values
+        assert result.wheel_filename == mock_wheel
+        assert result.sdist_filename is None
+        assert result.unpack_dir == expected_unpack_dir
+        assert result.sdist_root_dir == mock_sdist_root
+        assert result.build_env is not None
+        assert result.source_type == SourceType.SDIST