From 35054666bb542eae0525c59941562ab785989cfc Mon Sep 17 00:00:00 2001
From: Henry Schreiner <henryschreineriii@gmail.com>
Date: Wed, 27 Apr 2022 17:08:46 -0400
Subject: [PATCH] refactor: review comments

---
 cibuildwheel/__main__.py | 22 ++++++++++-----------
 cibuildwheel/options.py  |  7 +++----
 test/test_from_sdist.py  | 41 ----------------------------------------
 unit_test/utils.py       |  2 +-
 4 files changed, 14 insertions(+), 58 deletions(-)

diff --git a/cibuildwheel/__main__.py b/cibuildwheel/__main__.py
index af10eafed..f8158b313 100644
--- a/cibuildwheel/__main__.py
+++ b/cibuildwheel/__main__.py
@@ -67,6 +67,7 @@ def main() -> None:
     parser.add_argument(
         "--output-dir",
         type=Path,
+        default=Path(os.environ.get("CIBW_OUTPUT_DIR", "wheelhouse")),
         help="Destination folder for the wheels. Default: wheelhouse.",
     )
 
@@ -82,17 +83,18 @@ def main() -> None:
 
     parser.add_argument(
         "package_dir",
+        metavar="PACKAGE",
         default=Path("."),
         type=Path,
         nargs="?",
         help="""
-            Path to the package that you want wheels for. Must be a subdirectory
-            of the working directory. When set to a directory, the working
-            directory is still considered the 'project' and is copied into the
-            Docker container on Linux. Default: the working directory. This can
-            also be a tar.gz file - if it is, then --config-file and
-            --output-dir are relative to the current directory, and other paths
-            are relative to the expanded SDist directory.
+            Path to the package that you want wheels for. Default: the working
+            directory. Can be a directory inside the working directory, or an
+            sdist. When set to a directory, the working directory is still
+            considered the 'project' and is copied into the Docker container
+            on Linux.  When set to a tar.gz sdist file, --config-file
+            and --output-dir are relative to the current directory, and other
+            paths are relative to the expanded SDist directory.
         """,
     )
 
@@ -119,11 +121,7 @@ def main() -> None:
     args.package_dir = args.package_dir.resolve()
 
     # This are always relative to the base directory, even in SDist builds
-    args.output_dir = Path(
-        args.output_dir
-        if args.output_dir is not None
-        else os.environ.get("CIBW_OUTPUT_DIR", "wheelhouse")
-    ).resolve()
+    args.output_dir = args.output_dir.resolve()
 
     # Standard builds if a directory or non-existent path is given
     if not args.package_dir.is_file() and not args.package_dir.name.endswith("tar.gz"):
diff --git a/cibuildwheel/options.py b/cibuildwheel/options.py
index bf707b23b..5d9744af1 100644
--- a/cibuildwheel/options.py
+++ b/cibuildwheel/options.py
@@ -8,7 +8,7 @@
 from typing import (
     Any,
     Dict,
-    Iterator,
+    Generator,
     List,
     Mapping,
     NamedTuple,
@@ -48,7 +48,7 @@
 class CommandLineArguments:
     platform: Literal["auto", "linux", "macos", "windows"]
     archs: Optional[str]
-    output_dir: Optional[Path]
+    output_dir: Path
     config_file: str
     package_dir: Path
     print_build_identifiers: bool
@@ -265,7 +265,7 @@ def active_config_overrides(self) -> List[Override]:
         ]
 
     @contextmanager
-    def identifier(self, identifier: Optional[str]) -> Iterator[None]:
+    def identifier(self, identifier: Optional[str]) -> Generator[None, None, None]:
         self.current_identifier = identifier
         try:
             yield
@@ -363,7 +363,6 @@ def package_requires_python_str(self) -> Optional[str]:
     @property
     def globals(self) -> GlobalOptions:
         args = self.command_line_arguments
-        assert args.output_dir is not None, "Must be resolved"
         package_dir = args.package_dir
         output_dir = args.output_dir
 
diff --git a/test/test_from_sdist.py b/test/test_from_sdist.py
index 4af67015f..d1d4ff115 100644
--- a/test/test_from_sdist.py
+++ b/test/test_from_sdist.py
@@ -171,44 +171,3 @@ def test_internal_config_file_argument(tmp_path, capfd):
     # check that before-all was run
     captured = capfd.readouterr()
     assert "test log statement from before-all 1829" in captured.out
-
-
-def test_argument_passthrough(tmp_path, capfd):
-    basic_project = test_projects.new_c_project()
-
-    # make an sdist of a project
-    sdist_dir = tmp_path / "sdist"
-    sdist_dir.mkdir()
-    sdist_path = make_sdist(basic_project, sdist_dir)
-
-    # make a call that should pass some args through to cibuildwheel
-    # this asks cibuildwheel to print the ppc64le build identifiers
-    process = subprocess.run(
-        [
-            sys.executable,
-            "-m",
-            "cibuildwheel",
-            str(sdist_path),
-            "--platform",
-            "linux",
-            "--archs",
-            "ppc64le",
-            "--print-build-identifiers",
-        ],
-        env={
-            **os.environ,
-            "CIBW_BUILD": "cp38-*",
-        },
-        check=True,
-        stdout=subprocess.PIPE,
-        universal_newlines=True,
-    )
-
-    # fmt: off
-    assert process.stdout == textwrap.dedent(
-        """
-        cp38-manylinux_ppc64le
-        cp38-musllinux_ppc64le
-        """
-    ).lstrip()
-    # fmt: on
diff --git a/unit_test/utils.py b/unit_test/utils.py
index c2d94ed0e..ef158d551 100644
--- a/unit_test/utils.py
+++ b/unit_test/utils.py
@@ -10,7 +10,7 @@ def get_default_command_line_arguments() -> CommandLineArguments:
     defaults.allow_empty = False
     defaults.archs = None
     defaults.config_file = ""
-    defaults.output_dir = Path("wheelhouse")  # This must be resolved from "None" before passing
+    defaults.output_dir = Path("wheelhouse")
     defaults.package_dir = Path(".")
     defaults.prerelease_pythons = False
     defaults.print_build_identifiers = False