Removes unused "incremental" property (#948)

groodt · web-flow · commit d400f6bf37aa · 2022-12-27T11:34:30.000+11:00
diff --git a/docs/pip.md b/docs/pip.md
diff --git a/examples/build_file_generation/BUILD.bazel b/examples/build_file_generation/BUILD.bazel
@@ -41,7 +41,6 @@ modules_mapping(
 gazelle_python_manifest(
     name = "gazelle_python_manifest",
     modules_mapping = ":modules_map",
-    pip_repository_incremental = True,
     pip_repository_name = "pip",
     requirements = "//:requirements_lock.txt",
 )
diff --git a/examples/build_file_generation/gazelle_python.yaml b/examples/build_file_generation/gazelle_python.yaml
@@ -114,5 +114,4 @@ manifest:
     zipp.py310compat: zipp
   pip_repository:
     name: pip
-    incremental: true
-integrity: de2beca77b2b1e9c3ef24b56aab589fc05486abf1e0c08b51ea723621360ec73
+integrity: 4153df7683d64d7d6ad56c14ea1c7f7bec84a2ddf9ef8f075d1bb9313b8d11aa
diff --git a/gazelle/README.md b/gazelle/README.md
@@ -63,8 +63,6 @@ gazelle_python_manifest(
     # This is what we called our `pip_install` rule, where third-party
     # python libraries are loaded in BUILD files.
     pip_repository_name = "pip",
-    # When using pip_parse instead of pip_install, set the following.
-    # pip_repository_incremental = True,
     # This should point to wherever we declare our python dependencies
     # (the same as what we passed to the modules_mapping rule in WORKSPACE)
     requirements = "//:requirements_lock.txt",
diff --git a/gazelle/manifest/defs.bzl b/gazelle/manifest/defs.bzl
@@ -9,7 +9,6 @@ def gazelle_python_manifest(
         requirements,
         modules_mapping,
         pip_repository_name = "",
-        pip_repository_incremental = False,
         pip_deps_repository_name = "",
         manifest = ":gazelle_python.yaml"):
     """A macro for defining the updating and testing targets for the Gazelle manifest file.
@@ -18,7 +17,6 @@ def gazelle_python_manifest(
         name: the name used as a base for the targets.
         requirements: the target for the requirements.txt file.
         pip_repository_name: the name of the pip_install or pip_repository target.
-        pip_repository_incremental: the incremental property of pip_repository.
         pip_deps_repository_name: deprecated - the old pip_install target name.
         modules_mapping: the target for the generated modules_mapping.json file.
         manifest: the target for the Gazelle manifest file.
@@ -54,8 +52,6 @@ def gazelle_python_manifest(
         "--update-target",
         update_target_label,
     ]
-    if pip_repository_incremental:
-        update_args.append("--pip-repository-incremental")
 
     go_binary(
         name = update_target,
diff --git a/gazelle/manifest/generate/generate.go b/gazelle/manifest/generate/generate.go
@@ -27,7 +27,6 @@ func main() {
 	var manifestGeneratorHashPath string
 	var requirementsPath string
 	var pipRepositoryName string
-	var pipRepositoryIncremental bool
 	var modulesMappingPath string
 	var outputPath string
 	var updateTarget string
@@ -47,11 +46,6 @@ func main() {
 		"pip-repository-name",
 		"",
 		"The name of the pip_install or pip_repository target.")
-	flag.BoolVar(
-		&pipRepositoryIncremental,
-		"pip-repository-incremental",
-		false,
-		"The value for the incremental option in pip_repository.")
 	flag.StringVar(
 		&modulesMappingPath,
 		"modules-mapping",
@@ -96,7 +90,6 @@ func main() {
 		ModulesMapping: modulesMapping,
 		PipRepository: &manifest.PipRepository{
 			Name:        pipRepositoryName,
-			Incremental: pipRepositoryIncremental,
 		},
 	})
 	if err := writeOutput(
diff --git a/gazelle/manifest/manifest.go b/gazelle/manifest/manifest.go
@@ -130,6 +130,4 @@ type Manifest struct {
 type PipRepository struct {
 	// The name of the pip_install or pip_repository target.
 	Name string
-	// The incremental property of pip_repository.
-	Incremental bool
 }
diff --git a/gazelle/pythonconfig/pythonconfig.go b/gazelle/pythonconfig/pythonconfig.go
@@ -207,15 +207,9 @@ func (c *Config) FindThirdPartyDependency(modName string) (string, bool) {
 				sanitizedDistribution := strings.ToLower(distributionName)
 				sanitizedDistribution = strings.ReplaceAll(sanitizedDistribution, "-", "_")
 				var lbl label.Label
-				if gazelleManifest.PipRepository != nil && gazelleManifest.PipRepository.Incremental {
-					// @<repository_name>_<distribution_name>//:pkg
-					distributionRepositoryName = distributionRepositoryName + "_" + sanitizedDistribution
-					lbl = label.New(distributionRepositoryName, "", "pkg")
-				} else {
-					// @<repository_name>//pypi__<distribution_name>
-					distributionPackage := "pypi__" + sanitizedDistribution
-					lbl = label.New(distributionRepositoryName, distributionPackage, distributionPackage)
-				}
+				// @<repository_name>_<distribution_name>//:pkg
+				distributionRepositoryName = distributionRepositoryName + "_" + sanitizedDistribution
+				lbl = label.New(distributionRepositoryName, "", "pkg")
 				return lbl.String(), true
 			}
 		}
diff --git a/gazelle/testdata/dependency_resolution_order/BUILD.out b/gazelle/testdata/dependency_resolution_order/BUILD.out
@@ -9,6 +9,6 @@ py_library(
     deps = [
         "//baz",
         "//somewhere/bar",
-        "@gazelle_python_test//pypi__some_foo",
+        "@gazelle_python_test_some_foo//:pkg",
     ],
 )
diff --git a/gazelle/testdata/file_name_matches_import_statement/BUILD.out b/gazelle/testdata/file_name_matches_import_statement/BUILD.out
@@ -7,5 +7,5 @@ py_library(
         "rest_framework.py",
     ],
     visibility = ["//:__subpackages__"],
-    deps = ["@gazelle_python_test//pypi__djangorestframework"],
+    deps = ["@gazelle_python_test_djangorestframework//:pkg"],
 )
diff --git a/gazelle/testdata/ignored_invalid_imported_module/BUILD.out b/gazelle/testdata/ignored_invalid_imported_module/BUILD.out
@@ -4,5 +4,5 @@ py_library(
     name = "ignored_invalid_imported_module",
     srcs = ["__init__.py"],
     visibility = ["//:__subpackages__"],
-    deps = ["@gazelle_python_test//pypi__foo"],
+    deps = ["@gazelle_python_test_foo//:pkg"],
 )
diff --git a/gazelle/testdata/monorepo/coarse_grained/BUILD.out b/gazelle/testdata/monorepo/coarse_grained/BUILD.out
@@ -16,5 +16,5 @@ py_library(
         "foo/__init__.py",
     ],
     visibility = ["//:__subpackages__"],
-    deps = ["@root_pip_deps//pypi__rootboto3"],
+    deps = ["@root_pip_deps_rootboto3//:pkg"],
 )
diff --git a/gazelle/testdata/monorepo/one/BUILD.out b/gazelle/testdata/monorepo/one/BUILD.out
@@ -12,6 +12,6 @@ py_binary(
         "//one/bar",
         "//one/bar/baz:modified_name_baz",
         "//one/foo",
-        "@one_pip_deps//pypi__oneboto3",
+        "@one_pip_deps_oneboto3//:pkg",
     ],
 )
diff --git a/gazelle/testdata/monorepo/one/bar/BUILD.out b/gazelle/testdata/monorepo/one/bar/BUILD.out
@@ -8,5 +8,5 @@ py_library(
         "//one:__subpackages__",
         "//three:__subpackages__",
     ],
-    deps = ["@one_pip_deps//pypi__oneboto3"],
+    deps = ["@one_pip_deps_oneboto3//:pkg"],
 )
diff --git a/gazelle/testdata/monorepo/three/BUILD.out b/gazelle/testdata/monorepo/three/BUILD.out
@@ -15,7 +15,7 @@ py_library(
         "//one/bar",
         "//one/bar/baz:modified_name_baz",
         "//one/foo",
-        "@root_pip_deps//pypi__rootboto4",
+        "@root_pip_deps_rootboto4//:pkg",
         "@three_pip_deps_threeboto3//:pkg",
     ],
 )
diff --git a/gazelle/testdata/monorepo/three/gazelle_python.yaml b/gazelle/testdata/monorepo/three/gazelle_python.yaml
@@ -3,4 +3,3 @@ manifest:
     boto3: threeboto3
   pip_repository:
     name: three_pip_deps
-    incremental: true
diff --git a/gazelle/testdata/monorepo/two/BUILD.out b/gazelle/testdata/monorepo/two/BUILD.out
@@ -10,6 +10,6 @@ py_library(
     visibility = ["//two:__subpackages__"],
     deps = [
         "//one/foo",
-        "@two_pip_deps//pypi__twoboto3",
+        "@two_pip_deps_twoboto3//:pkg",
     ],
 )
diff --git a/gazelle/testdata/python_ignore_dependencies_directive/BUILD.out b/gazelle/testdata/python_ignore_dependencies_directive/BUILD.out
@@ -7,5 +7,5 @@ py_library(
     name = "python_ignore_dependencies_directive",
     srcs = ["__init__.py"],
     visibility = ["//:__subpackages__"],
-    deps = ["@gazelle_python_test//pypi__boto3"],
+    deps = ["@gazelle_python_test_boto3//:pkg"],
 )
diff --git a/gazelle/testdata/python_target_with_test_in_name/BUILD.out b/gazelle/testdata/python_target_with_test_in_name/BUILD.out
@@ -8,5 +8,5 @@ py_library(
         "test_not_a_real.py",
     ],
     visibility = ["//:__subpackages__"],
-    deps = ["@gazelle_python_test//pypi__boto3"],
+    deps = ["@gazelle_python_test_boto3//:pkg"],
 )
diff --git a/gazelle/testdata/with_nested_import_statements/BUILD.out b/gazelle/testdata/with_nested_import_statements/BUILD.out
@@ -4,5 +4,5 @@ py_library(
     name = "with_nested_import_statements",
     srcs = ["__init__.py"],
     visibility = ["//:__subpackages__"],
-    deps = ["@gazelle_python_test//pypi__boto3"],
+    deps = ["@gazelle_python_test_boto3//:pkg"],
 )
diff --git a/gazelle/testdata/with_third_party_requirements/BUILD.out b/gazelle/testdata/with_third_party_requirements/BUILD.out
@@ -9,9 +9,9 @@ py_library(
     ],
     visibility = ["//:__subpackages__"],
     deps = [
-        "@gazelle_python_test//pypi__baz",
-        "@gazelle_python_test//pypi__boto3",
-        "@gazelle_python_test//pypi__djangorestframework",
+        "@gazelle_python_test_baz//:pkg",
+        "@gazelle_python_test_boto3//:pkg",
+        "@gazelle_python_test_djangorestframework//:pkg",
     ],
 )
 
@@ -22,6 +22,6 @@ py_binary(
     visibility = ["//:__subpackages__"],
     deps = [
         ":with_third_party_requirements",
-        "@gazelle_python_test//pypi__baz",
+        "@gazelle_python_test_baz//:pkg",
     ],
 )
diff --git a/gazelle/testdata/with_third_party_requirements_from_imports/gazelle_python.yaml b/gazelle/testdata/with_third_party_requirements_from_imports/gazelle_python.yaml
@@ -1661,5 +1661,4 @@ manifest:
     urllib3.util.wait: urllib3
   pip_repository:
     name: gazelle_python_test
-    incremental: true
 integrity: 32e38932043eca090a64ca741758d8e4a5817c2cd7dc821fc927914c32fb3114
diff --git a/python/pip.bzl b/python/pip.bzl
@@ -53,7 +53,7 @@ def pip_parse(requirements = None, requirements_lock = None, name = "pip_parsed_
     Those dependencies become available in a generated `requirements.bzl` file.
     You can instead check this `requirements.bzl` file into your repo, see the "vendoring" section below.
 
-    This macro wraps the [`pip_repository`](./pip_repository.md) rule that invokes `pip`, with `incremental` set.
+    This macro wraps the [`pip_repository`](./pip_repository.md) rule that invokes `pip`.
     In your WORKSPACE file:
 
     ```python
diff --git a/python/pip_install/extract_wheels/wheel_installer.py b/python/pip_install/extract_wheels/wheel_installer.py
@@ -294,19 +294,16 @@ def _extract_wheel(
     pip_data_exclude: List[str],
     enable_implicit_namespace_pkgs: bool,
     repo_prefix: str,
-    incremental: bool = False,
     incremental_dir: Path = Path("."),
     annotation: Optional[annotation.Annotation] = None,
-) -> Optional[str]:
+) -> None:
     """Extracts wheel into given directory and creates py_library and filegroup targets.
 
     Args:
         wheel_file: the filepath of the .whl
         extras: a list of extras to add as dependencies for the installed wheel
         pip_data_exclude: list of file patterns to exclude from the generated data section of the py_library
         enable_implicit_namespace_pkgs: if true, disables conversion of implicit namespace packages and will unzip as-is
-        incremental: If true the extract the wheel in a format suitable for an external repository. This
-            effects the names of libraries and their dependencies, which point to other external repositories.
         incremental_dir: An optional override for the working directory of incremental builds.
         annotation: An optional set of annotations to apply to the BUILD contents of the wheel.
 
@@ -315,14 +312,7 @@ def _extract_wheel(
     """
 
     whl = wheel.Wheel(wheel_file)
-    if incremental:
-        directory = incremental_dir
-    else:
-        directory = bazel.sanitise_name(whl.name, prefix=repo_prefix)
-
-        os.mkdir(directory)
-        # copy the original wheel
-        shutil.copy(whl.path, directory)
+    directory = incremental_dir
     whl.unzip(directory)
 
     if not enable_implicit_namespace_pkgs:
@@ -334,28 +324,12 @@ def _extract_wheel(
     self_edge_dep = set([whl.name])
     whl_deps = sorted(whl.dependencies(extras_requested) - self_edge_dep)
 
-    if incremental:
-        sanitised_dependencies = [
-            bazel.sanitised_repo_library_label(d, repo_prefix=repo_prefix)
-            for d in whl_deps
-        ]
-        sanitised_wheel_file_dependencies = [
-            bazel.sanitised_repo_file_label(d, repo_prefix=repo_prefix)
-            for d in whl_deps
-        ]
-    else:
-        sanitised_dependencies = [
-            _sanitised_library_label(d, prefix=repo_prefix) for d in whl_deps
-        ]
-        sanitised_wheel_file_dependencies = [
-            _sanitised_file_label(d, prefix=repo_prefix) for d in whl_deps
-        ]
-
-    library_name = (
-        bazel.PY_LIBRARY_LABEL
-        if incremental
-        else bazel.sanitise_name(whl.name, repo_prefix)
-    )
+    sanitised_dependencies = [
+        bazel.sanitised_repo_library_label(d, repo_prefix=repo_prefix) for d in whl_deps
+    ]
+    sanitised_wheel_file_dependencies = [
+        bazel.sanitised_repo_file_label(d, repo_prefix=repo_prefix) for d in whl_deps
+    ]
 
     directory_path = Path(directory)
     entry_points = []
@@ -374,7 +348,7 @@ def _extract_wheel(
             _generate_entry_point_rule(
                 entry_point_target_name,
                 entry_point_script_name,
-                library_name,
+                bazel.PY_LIBRARY_LABEL,
             )
         )
 
@@ -399,9 +373,7 @@ def _extract_wheel(
                 additional_content.append(annotation.additive_build_content)
 
         contents = _generate_build_file_contents(
-            name=bazel.PY_LIBRARY_LABEL
-            if incremental
-            else bazel.sanitise_name(whl.name, repo_prefix),
+            name=bazel.PY_LIBRARY_LABEL,
             dependencies=sanitised_dependencies,
             whl_file_deps=sanitised_wheel_file_dependencies,
             data_exclude=data_exclude,
@@ -412,11 +384,6 @@ def _extract_wheel(
         )
         build_file.write(contents)
 
-    if not incremental:
-        os.remove(whl.path)
-        return f"//{directory}"
-    return None
-
 
 def main() -> None:
     parser = argparse.ArgumentParser(
@@ -478,7 +445,6 @@ def main() -> None:
         extras=extras,
         pip_data_exclude=deserialized_args["pip_data_exclude"],
         enable_implicit_namespace_pkgs=args.enable_implicit_namespace_pkgs,
-        incremental=True,
         repo_prefix=args.repo_prefix,
         annotation=args.annotation,
     )
diff --git a/python/pip_install/extract_wheels/wheel_installer_test.py b/python/pip_install/extract_wheels/wheel_installer_test.py
@@ -74,37 +74,21 @@ def setUp(self) -> None:
     def tearDown(self):
         shutil.rmtree(self.wheel_dir)
 
-    def _run(
-        self,
-        repo_prefix: str,
-        incremental: bool = False,
-    ) -> None:
-        generated_bazel_dir = wheel_installer._extract_wheel(
+    def test_wheel_exists(self) -> None:
+        wheel_installer._extract_wheel(
             self.wheel_path,
             extras={},
             pip_data_exclude=[],
             enable_implicit_namespace_pkgs=False,
-            incremental=incremental,
-            repo_prefix=repo_prefix,
+            repo_prefix="prefix_",
             incremental_dir=Path(self.wheel_dir),
         )
-        # Take off the leading // from the returned label.
-        # Assert that the raw wheel ends up in the package.
-        generated_bazel_dir = (
-            generated_bazel_dir[2:] if not incremental else self.wheel_dir
-        )
 
-        self.assertIn(self.wheel_name, os.listdir(generated_bazel_dir))
-        with open("{}/BUILD.bazel".format(generated_bazel_dir)) as build_file:
+        self.assertIn(self.wheel_name, os.listdir(self.wheel_dir))
+        with open("{}/BUILD.bazel".format(self.wheel_dir)) as build_file:
             build_file_content = build_file.read()
             self.assertIn("filegroup", build_file_content)
 
-    def test_nonincremental(self) -> None:
-        self._run(repo_prefix="prefix_")
-
-    def test_incremental(self) -> None:
-        self._run(incremental=True, repo_prefix="prefix_")
-
 
 if __name__ == "__main__":
     unittest.main()

Original file line number	Diff line number	Diff line change
`@@ -41,7 +41,6 @@ modules_mapping(`
`41`	`41`	`gazelle_python_manifest(`
`42`	`42`	`name = "gazelle_python_manifest",`
`43`	`43`	`modules_mapping = ":modules_map",`
`44`		`- pip_repository_incremental = True,`
`45`	`44`	`pip_repository_name = "pip",`
`46`	`45`	`requirements = "//:requirements_lock.txt",`
`47`	`46`	`)`
Original file line number	Diff line number	Diff line change
`@@ -130,6 +130,4 @@ type Manifest struct {`
`130`	`130`	`type PipRepository struct {`
`131`	`131`	`// The name of the pip_install or pip_repository target.`
`132`	`132`	`Name string`
`133`		`- // The incremental property of pip_repository.`
`134`		`- Incremental bool`
`135`	`133`	`}`
Original file line number	Diff line number	Diff line change
`@@ -9,6 +9,6 @@ py_library(`
`9`	`9`	`deps = [`
`10`	`10`	`"//baz",`
`11`	`11`	`"//somewhere/bar",`
`12`		`- "@gazelle_python_test//pypi__some_foo",`
	`12`	`+ "@gazelle_python_test_some_foo//:pkg",`
`13`	`13`	`],`
`14`	`14`	`)`
Original file line number	Diff line number	Diff line change
`@@ -7,5 +7,5 @@ py_library(`
`7`	`7`	`"rest_framework.py",`
`8`	`8`	`],`
`9`	`9`	`visibility = ["//:__subpackages__"],`
`10`		`- deps = ["@gazelle_python_test//pypi__djangorestframework"],`
	`10`	`+ deps = ["@gazelle_python_test_djangorestframework//:pkg"],`
`11`	`11`	`)`