From 2e952ffcf10d186cb52bea088838099a187d423d Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Mon, 21 Aug 2023 11:29:11 +0800 Subject: [PATCH 01/20] Improve site-package pathing to path all shuffix of file --- client/configuration/search_path.py | 48 ++++++++++++++++++++++++++--- 1 file changed, 44 insertions(+), 4 deletions(-) diff --git a/client/configuration/search_path.py b/client/configuration/search_path.py index da7601a7558..12e55e2e660 100644 --- a/client/configuration/search_path.py +++ b/client/configuration/search_path.py @@ -19,13 +19,19 @@ import glob import logging import os -from typing import Dict, Iterable, List, Sequence, Union +import re +from typing import Dict, Iterable, List, Sequence, Tuple, Union from .. import filesystem from . import exceptions LOG: logging.Logger = logging.getLogger(__name__) +dist_info_in_root: Dict[str, List[str]] = {} +_site_filter = re.compile(r".*-([0-99]\.)*dist-info") + +_PYCACHE = re.compile("__pycache__") + def _expand_relative_root(path: str, relative_root: str) -> str: if not path.startswith("//"): @@ -72,9 +78,43 @@ class SitePackageElement(Element): package_name: str is_toplevel_module: bool = False - def package_path(self) -> str: - module_suffix = ".py" if self.is_toplevel_module else "" - return self.package_name + module_suffix + def package_path(self) -> Union[str, None]: + if not self.is_toplevel_module: + return self.package_name + + this_pkg_filter = re.compile( + r"{}-([0-99]\.)*dist-info".format(self.package_name) + ) + + if self.site_root not in dist_info_in_root: + dist_info_in_root[self.site_root] = [] + for directory in os.listdir(self.site_root): + if _site_filter.fullmatch(directory) is not None: + dist_info_in_root[self.site_root].append(directory) + + for directory in dist_info_in_root[self.site_root]: + if this_pkg_filter.fullmatch(directory): + dist_info_path = f"{self.site_root}/{directory}" + break + else: + return None + + not_toplevel_patterns: Tuple[re.Pattern] = (this_pkg_filter, _PYCACHE) + + with open(file=f"{dist_info_path}/RECORD", mode="r") as record: + files = [] + for val in [line.split(",") for line in record.readlines()]: + files.append(*val) + for file in files: + if not file: + continue + for pattern in not_toplevel_patterns: + if pattern.fullmatch(file) is not None: + break + else: + return file + + return None def path(self) -> str: return os.path.join(self.site_root, self.package_path()) From 3810dd6f32c506dd37f217f0b400b51302b7d67d Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Mon, 21 Aug 2023 11:51:55 +0800 Subject: [PATCH 02/20] Update unittest --- client/configuration/tests/search_path_test.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/client/configuration/tests/search_path_test.py b/client/configuration/tests/search_path_test.py index 90a57b2586b..67892a640ce 100644 --- a/client/configuration/tests/search_path_test.py +++ b/client/configuration/tests/search_path_test.py @@ -84,6 +84,14 @@ def test_command_line_argument(self) -> None: SitePackageElement("foo", "bar").command_line_argument(), "foo$bar", ) + + Path.mkdir(Path("foo")) + Path.mkdir(Path("foo/bar-1.0.0.dist-info")) + Path.touch(Path("foo/bar.py")) + + with open("foo/bar-1.0.0.dist-info/RECORD", "w", encoding="UTF-8") as f: + f.write("bar.py") + self.assertEqual( SitePackageElement("foo", "bar", True).command_line_argument(), "foo$bar.py", From 9e7de0f0f3f2e54c80f9270e5016aa38f230f84b Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Mon, 21 Aug 2023 12:27:48 +0800 Subject: [PATCH 03/20] Add unittest for new feature --- client/configuration/tests/search_path_test.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/client/configuration/tests/search_path_test.py b/client/configuration/tests/search_path_test.py index 67892a640ce..359ae6cda27 100644 --- a/client/configuration/tests/search_path_test.py +++ b/client/configuration/tests/search_path_test.py @@ -249,3 +249,13 @@ def test_process_required_raw_elements_site_package_nonexistence(self) -> None: site_roots=[], required=True, ) + + def test_toplevel_module_not_pyfile(self): + Path.mkdir(Path("foo")) + Path.mkdir(Path("foo/bar-1.0.0.dist-info")) + Path.touch(Path("foo/bar.so")) + + with open("foo/bar-1.0.0.dist-info/RECORD", "w", encoding="UTF-8") as f: + f.write("bar.so") + + self.assertEqual(SitePackageElement("foo", "bar", True).path(), "foo/bar.so") From 631e427f05173b0eba1e833d65ffa90776bf1baa Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Mon, 21 Aug 2023 12:35:55 +0800 Subject: [PATCH 04/20] Fix FileExistsError in unittest --- client/configuration/tests/search_path_test.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/client/configuration/tests/search_path_test.py b/client/configuration/tests/search_path_test.py index 359ae6cda27..ec33c5a1e41 100644 --- a/client/configuration/tests/search_path_test.py +++ b/client/configuration/tests/search_path_test.py @@ -85,9 +85,9 @@ def test_command_line_argument(self) -> None: "foo$bar", ) - Path.mkdir(Path("foo")) - Path.mkdir(Path("foo/bar-1.0.0.dist-info")) - Path.touch(Path("foo/bar.py")) + Path.mkdir(Path("foo"), exist_ok=True) + Path.mkdir(Path("foo/bar-1.0.0.dist-info"), exist_ok=True) + Path.touch(Path("foo/bar.py"), exist_ok=True) with open("foo/bar-1.0.0.dist-info/RECORD", "w", encoding="UTF-8") as f: f.write("bar.py") @@ -251,9 +251,9 @@ def test_process_required_raw_elements_site_package_nonexistence(self) -> None: ) def test_toplevel_module_not_pyfile(self): - Path.mkdir(Path("foo")) - Path.mkdir(Path("foo/bar-1.0.0.dist-info")) - Path.touch(Path("foo/bar.so")) + Path.mkdir(Path("foo"), exist_ok=True) + Path.mkdir(Path("foo/bar-1.0.0.dist-info"), exist_ok=True) + Path.touch(Path("foo/bar.so"), exist_ok=True) with open("foo/bar-1.0.0.dist-info/RECORD", "w", encoding="UTF-8") as f: f.write("bar.so") From 5fdddb4d1c14ced07b92fda33effb35a0a223f26 Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Mon, 21 Aug 2023 13:50:23 +0800 Subject: [PATCH 05/20] Fix type errors --- client/configuration/exceptions.py | 5 +++++ client/configuration/search_path.py | 16 ++++++++++------ client/configuration/tests/search_path_test.py | 2 +- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/client/configuration/exceptions.py b/client/configuration/exceptions.py index 644217e89cc..fa18852f729 100644 --- a/client/configuration/exceptions.py +++ b/client/configuration/exceptions.py @@ -18,3 +18,8 @@ def __init__(self, message: str) -> None: class InvalidPythonVersion(InvalidConfiguration): def __init__(self, message: str) -> None: super().__init__(message) + + +class InvalidPackage(ValueError): + def __init__(self, pkg_name: str) -> None: + super().__init__(f"Invalid package: {pkg_name} does not exist.") diff --git a/client/configuration/search_path.py b/client/configuration/search_path.py index 12e55e2e660..b82b6be7fce 100644 --- a/client/configuration/search_path.py +++ b/client/configuration/search_path.py @@ -28,9 +28,9 @@ LOG: logging.Logger = logging.getLogger(__name__) dist_info_in_root: Dict[str, List[str]] = {} -_site_filter = re.compile(r".*-([0-99]\.)*dist-info") +_site_filter: re.Pattern[str] = re.compile(r".*-([0-99]\.)*dist-info") -_PYCACHE = re.compile("__pycache__") +_PYCACHE: re.Pattern[str] = re.compile("__pycache__") def _expand_relative_root(path: str, relative_root: str) -> str: @@ -78,7 +78,7 @@ class SitePackageElement(Element): package_name: str is_toplevel_module: bool = False - def package_path(self) -> Union[str, None]: + def package_path(self) -> str: if not self.is_toplevel_module: return self.package_name @@ -97,10 +97,14 @@ def package_path(self) -> Union[str, None]: dist_info_path = f"{self.site_root}/{directory}" break else: - return None + raise exceptions.InvalidPackage(self.package_name) - not_toplevel_patterns: Tuple[re.Pattern] = (this_pkg_filter, _PYCACHE) + not_toplevel_patterns: Tuple[re.Pattern[str], re.Pattern[str]] = ( + this_pkg_filter, + _PYCACHE, + ) + # pyre-fixme[61]: Local variable `dist_info_path` is undefined, or not always defined. with open(file=f"{dist_info_path}/RECORD", mode="r") as record: files = [] for val in [line.split(",") for line in record.readlines()]: @@ -114,7 +118,7 @@ def package_path(self) -> Union[str, None]: else: return file - return None + raise exceptions.InvalidPackage(self.package_name) def path(self) -> str: return os.path.join(self.site_root, self.package_path()) diff --git a/client/configuration/tests/search_path_test.py b/client/configuration/tests/search_path_test.py index ec33c5a1e41..bf3790e7e8b 100644 --- a/client/configuration/tests/search_path_test.py +++ b/client/configuration/tests/search_path_test.py @@ -250,7 +250,7 @@ def test_process_required_raw_elements_site_package_nonexistence(self) -> None: required=True, ) - def test_toplevel_module_not_pyfile(self): + def test_toplevel_module_not_pyfile(self) -> None: Path.mkdir(Path("foo"), exist_ok=True) Path.mkdir(Path("foo/bar-1.0.0.dist-info"), exist_ok=True) Path.touch(Path("foo/bar.so"), exist_ok=True) From ee6782418fd4781f2a13f4b0a616764ddda31351 Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Mon, 21 Aug 2023 14:01:00 +0800 Subject: [PATCH 06/20] Fix type errors --- client/configuration/search_path.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/client/configuration/search_path.py b/client/configuration/search_path.py index b82b6be7fce..f49b835f3cb 100644 --- a/client/configuration/search_path.py +++ b/client/configuration/search_path.py @@ -28,9 +28,12 @@ LOG: logging.Logger = logging.getLogger(__name__) dist_info_in_root: Dict[str, List[str]] = {} -_site_filter: re.Pattern[str] = re.compile(r".*-([0-99]\.)*dist-info") -_PYCACHE: re.Pattern[str] = re.compile("__pycache__") +# pyre-fixme[5]: Globally accessible variable `_site_filter` has type `re.Pattern[str]` but no type is specified. +_site_filter = re.compile(r".*-([0-99]\.)*dist-info") + +# pyre-fixme[5]: Globally accessible variable `_PYCACHE` has type `re.Pattern[str]` but no type is specified. +_PYCACHE = re.compile("__pycache__") def _expand_relative_root(path: str, relative_root: str) -> str: From 94749bed7481a65a9bf39adb20ebd75aa9dbd761 Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Tue, 22 Aug 2023 11:23:48 +0800 Subject: [PATCH 07/20] Update pattern to filter out path like `__pycache__/valid_before_this_commit` --- client/configuration/search_path.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/configuration/search_path.py b/client/configuration/search_path.py index f49b835f3cb..f11497d8b2c 100644 --- a/client/configuration/search_path.py +++ b/client/configuration/search_path.py @@ -33,7 +33,7 @@ _site_filter = re.compile(r".*-([0-99]\.)*dist-info") # pyre-fixme[5]: Globally accessible variable `_PYCACHE` has type `re.Pattern[str]` but no type is specified. -_PYCACHE = re.compile("__pycache__") +_PYCACHE = re.compile("__pycache__(/)*.*") def _expand_relative_root(path: str, relative_root: str) -> str: @@ -86,7 +86,7 @@ def package_path(self) -> str: return self.package_name this_pkg_filter = re.compile( - r"{}-([0-99]\.)*dist-info".format(self.package_name) + r"{}-([0-99]\.)*dist-info(/)*.*".format(self.package_name) ) if self.site_root not in dist_info_in_root: From 42baee265f28b9fc925f71e3b3917a4950c13c93 Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Tue, 22 Aug 2023 11:48:04 +0800 Subject: [PATCH 08/20] Improve unittest --- .../configuration/tests/search_path_test.py | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/client/configuration/tests/search_path_test.py b/client/configuration/tests/search_path_test.py index bf3790e7e8b..3104721c7d9 100644 --- a/client/configuration/tests/search_path_test.py +++ b/client/configuration/tests/search_path_test.py @@ -3,6 +3,7 @@ # This source code is licensed under the MIT license found in the # LICENSE file in the root directory of this source tree. +import shutil import tempfile from pathlib import Path @@ -85,17 +86,20 @@ def test_command_line_argument(self) -> None: "foo$bar", ) - Path.mkdir(Path("foo"), exist_ok=True) - Path.mkdir(Path("foo/bar-1.0.0.dist-info"), exist_ok=True) - Path.touch(Path("foo/bar.py"), exist_ok=True) + Path.mkdir(Path("foo")) + Path.mkdir(Path("foo/bar-1.0.0.dist-info")) + Path.touch(Path("foo/bar.py")) with open("foo/bar-1.0.0.dist-info/RECORD", "w", encoding="UTF-8") as f: f.write("bar.py") - self.assertEqual( - SitePackageElement("foo", "bar", True).command_line_argument(), - "foo$bar.py", - ) + try: + self.assertEqual( + SitePackageElement("foo", "bar", True).command_line_argument(), + "foo$bar.py", + ) + finally: + shutil.rmtree("foo") def test_expand_global_root(self) -> None: self.assertEqual( @@ -251,11 +255,16 @@ def test_process_required_raw_elements_site_package_nonexistence(self) -> None: ) def test_toplevel_module_not_pyfile(self) -> None: - Path.mkdir(Path("foo"), exist_ok=True) - Path.mkdir(Path("foo/bar-1.0.0.dist-info"), exist_ok=True) - Path.touch(Path("foo/bar.so"), exist_ok=True) + Path.mkdir(Path("foo")) + Path.mkdir(Path("foo/bar-1.0.0.dist-info")) + Path.touch(Path("foo/bar.so")) with open("foo/bar-1.0.0.dist-info/RECORD", "w", encoding="UTF-8") as f: f.write("bar.so") - self.assertEqual(SitePackageElement("foo", "bar", True).path(), "foo/bar.so") + try: + self.assertEqual( + SitePackageElement("foo", "bar", True).path(), "foo/bar.so" + ) + finally: + shutil.rmtree("foo") From d6fac87380346aafb1ac60f8233cf015eb67f064 Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Tue, 22 Aug 2023 17:59:04 +0800 Subject: [PATCH 09/20] Improve handing on not exists site-package --- client/backend_arguments.py | 18 +++++++- client/configuration/exceptions.py | 5 --- client/configuration/search_path.py | 45 +++++++++++-------- .../configuration/tests/search_path_test.py | 36 ++++++++++----- client/tests/backend_arguments_test.py | 20 ++++++--- 5 files changed, 80 insertions(+), 44 deletions(-) diff --git a/client/backend_arguments.py b/client/backend_arguments.py index c5a3703155e..78d1ac9015f 100644 --- a/client/backend_arguments.py +++ b/client/backend_arguments.py @@ -69,7 +69,14 @@ def serialize(self) -> Dict[str, object]: } def get_checked_directory_allowlist(self) -> Set[str]: - return {element.path() for element in self.elements} + paths = set() + + for element in self.elements: + excepted_path = element.path() + if excepted_path is not None: + paths.add(excepted_path) + + return paths def cleanup(self) -> None: pass @@ -98,7 +105,14 @@ def serialize(self) -> Dict[str, object]: } def get_checked_directory_allowlist(self) -> Set[str]: - return {element.path() for element in self.elements} + paths = set() + + for element in self.elements: + excepted_path = element.path() + if excepted_path is not None: + paths.add(excepted_path) + + return paths def cleanup(self) -> None: pass diff --git a/client/configuration/exceptions.py b/client/configuration/exceptions.py index fa18852f729..644217e89cc 100644 --- a/client/configuration/exceptions.py +++ b/client/configuration/exceptions.py @@ -18,8 +18,3 @@ def __init__(self, message: str) -> None: class InvalidPythonVersion(InvalidConfiguration): def __init__(self, message: str) -> None: super().__init__(message) - - -class InvalidPackage(ValueError): - def __init__(self, pkg_name: str) -> None: - super().__init__(f"Invalid package: {pkg_name} does not exist.") diff --git a/client/configuration/search_path.py b/client/configuration/search_path.py index f11497d8b2c..cdb2795b7fc 100644 --- a/client/configuration/search_path.py +++ b/client/configuration/search_path.py @@ -44,11 +44,11 @@ def _expand_relative_root(path: str, relative_root: str) -> str: class Element(abc.ABC): @abc.abstractmethod - def path(self) -> str: + def path(self) -> Union[str, None]: raise NotImplementedError() @abc.abstractmethod - def command_line_argument(self) -> str: + def command_line_argument(self) -> Union[str, None]: raise NotImplementedError() @@ -81,9 +81,11 @@ class SitePackageElement(Element): package_name: str is_toplevel_module: bool = False - def package_path(self) -> str: + def package_path(self) -> Union[str, None]: if not self.is_toplevel_module: - return self.package_name + if os.path.exists(f"{self.site_root}/{self.package_name}"): + return self.package_name + return None this_pkg_filter = re.compile( r"{}-([0-99]\.)*dist-info(/)*.*".format(self.package_name) @@ -100,7 +102,7 @@ def package_path(self) -> str: dist_info_path = f"{self.site_root}/{directory}" break else: - raise exceptions.InvalidPackage(self.package_name) + return None not_toplevel_patterns: Tuple[re.Pattern[str], re.Pattern[str]] = ( this_pkg_filter, @@ -109,25 +111,27 @@ def package_path(self) -> str: # pyre-fixme[61]: Local variable `dist_info_path` is undefined, or not always defined. with open(file=f"{dist_info_path}/RECORD", mode="r") as record: - files = [] - for val in [line.split(",") for line in record.readlines()]: - files.append(*val) - for file in files: - if not file: - continue + for line in record.readlines(): + file_name = line.split(",")[0] for pattern in not_toplevel_patterns: - if pattern.fullmatch(file) is not None: + if pattern.fullmatch(file_name): break else: - return file + return file_name - raise exceptions.InvalidPackage(self.package_name) + return None - def path(self) -> str: - return os.path.join(self.site_root, self.package_path()) + def path(self) -> Union[str, None]: + excepted_package_path: Union[str, None] = self.package_path() + if excepted_package_path is None: + return None + return os.path.join(self.site_root, excepted_package_path) - def command_line_argument(self) -> str: - return self.site_root + "$" + self.package_path() + def command_line_argument(self) -> Union[str, None]: + excepted_package_path: Union[str, None] = self.package_path() + if excepted_package_path is None: + return None + return self.site_root + "$" + excepted_package_path class RawElement(abc.ABC): @@ -270,7 +274,10 @@ def process_raw_elements( elements: List[Element] = [] def add_if_exists(element: Element) -> bool: - if os.path.exists(element.path()): + excepted_path = element.path() + if excepted_path is None: + return False + if os.path.exists(excepted_path): elements.append(element) return True return False diff --git a/client/configuration/tests/search_path_test.py b/client/configuration/tests/search_path_test.py index 3104721c7d9..5cc5fa6d363 100644 --- a/client/configuration/tests/search_path_test.py +++ b/client/configuration/tests/search_path_test.py @@ -71,20 +71,32 @@ def test_create_raw_element(self) -> None: ) def test_path(self) -> None: - self.assertEqual(SimpleElement("foo").path(), "foo") - self.assertEqual(SubdirectoryElement("foo", "bar").path(), "foo/bar") - self.assertEqual(SitePackageElement("foo", "bar").path(), "foo/bar") + Path.mkdir(Path("foo")) + Path.mkdir(Path("foo/bar")) + + try: + self.assertEqual(SimpleElement("foo").path(), "foo") + self.assertEqual(SubdirectoryElement("foo", "bar").path(), "foo/bar") + self.assertEqual(SitePackageElement("foo", "bar").path(), "foo/bar") + finally: + shutil.rmtree("foo") def test_command_line_argument(self) -> None: - self.assertEqual(SimpleElement("foo").command_line_argument(), "foo") - self.assertEqual( - SubdirectoryElement("foo", "bar").command_line_argument(), - "foo$bar", - ) - self.assertEqual( - SitePackageElement("foo", "bar").command_line_argument(), - "foo$bar", - ) + Path.mkdir(Path("foo")) + Path.mkdir(Path("foo/bar")) + + try: + self.assertEqual(SimpleElement("foo").command_line_argument(), "foo") + self.assertEqual( + SubdirectoryElement("foo", "bar").command_line_argument(), + "foo$bar", + ) + self.assertEqual( + SitePackageElement("foo", "bar").command_line_argument(), + "foo$bar", + ) + finally: + shutil.rmtree("foo") Path.mkdir(Path("foo")) Path.mkdir(Path("foo/bar-1.0.0.dist-info")) diff --git a/client/tests/backend_arguments_test.py b/client/tests/backend_arguments_test.py index a114fe2f891..558c504775c 100644 --- a/client/tests/backend_arguments_test.py +++ b/client/tests/backend_arguments_test.py @@ -3,6 +3,7 @@ # This source code is licensed under the MIT license found in the # LICENSE file in the root directory of this source tree. +import shutil import tempfile from pathlib import Path from typing import Iterable, Tuple @@ -576,15 +577,22 @@ def test_get_source_path__confliciting_source_specified(self) -> None: ) def test_get_checked_directory_for_simple_source_path(self) -> None: + Path.mkdir(Path("super")) + Path.mkdir(Path("super/slash")) + element0 = search_path.SimpleElement("ozzie") element1 = search_path.SubdirectoryElement("diva", "flea") element2 = search_path.SitePackageElement("super", "slash") - self.assertCountEqual( - SimpleSourcePath( - [element0, element1, element2, element0] - ).get_checked_directory_allowlist(), - [element0.path(), element1.path(), element2.path()], - ) + + try: + self.assertCountEqual( + SimpleSourcePath( + [element0, element1, element2, element0] + ).get_checked_directory_allowlist(), + [element0.path(), element1.path(), element2.path()], + ) + finally: + shutil.rmtree("super") def test_get_checked_directory_for_buck_source_path(self) -> None: self.assertCountEqual( From 7e6afcbaa246da8fe68ae85e8b6bb4ecc45b583b Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Fri, 8 Sep 2023 17:39:17 +0800 Subject: [PATCH 10/20] Delete unnecessary condition If the `excepted_path` is not None, it will must be a exists path --- client/configuration/search_path.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/client/configuration/search_path.py b/client/configuration/search_path.py index cdb2795b7fc..d55208c9393 100644 --- a/client/configuration/search_path.py +++ b/client/configuration/search_path.py @@ -277,10 +277,8 @@ def add_if_exists(element: Element) -> bool: excepted_path = element.path() if excepted_path is None: return False - if os.path.exists(excepted_path): - elements.append(element) - return True - return False + elements.append(element) + return True for raw_element in raw_elements: expanded_raw_elements = raw_element.expand_glob() From 5c0f85cab8e53a5d2d5f453b99e8f743be7cd368 Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Sat, 9 Sep 2023 12:46:14 +0800 Subject: [PATCH 11/20] Fix element adding in func add_if_exists --- client/configuration/search_path.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/client/configuration/search_path.py b/client/configuration/search_path.py index d55208c9393..3528df7a444 100644 --- a/client/configuration/search_path.py +++ b/client/configuration/search_path.py @@ -83,9 +83,7 @@ class SitePackageElement(Element): def package_path(self) -> Union[str, None]: if not self.is_toplevel_module: - if os.path.exists(f"{self.site_root}/{self.package_name}"): - return self.package_name - return None + return self.package_name this_pkg_filter = re.compile( r"{}-([0-99]\.)*dist-info(/)*.*".format(self.package_name) @@ -275,7 +273,7 @@ def process_raw_elements( def add_if_exists(element: Element) -> bool: excepted_path = element.path() - if excepted_path is None: + if excepted_path is None or not os.path.exists(excepted_path): return False elements.append(element) return True From 1608b6a03897edbc42a8a6d1755d616f66a0b273 Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Sat, 9 Sep 2023 14:05:20 +0800 Subject: [PATCH 12/20] Add unittest for process_raw_element --- client/configuration/tests/search_path_test.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/client/configuration/tests/search_path_test.py b/client/configuration/tests/search_path_test.py index 5cc5fa6d363..593fbbb465d 100644 --- a/client/configuration/tests/search_path_test.py +++ b/client/configuration/tests/search_path_test.py @@ -278,5 +278,9 @@ def test_toplevel_module_not_pyfile(self) -> None: self.assertEqual( SitePackageElement("foo", "bar", True).path(), "foo/bar.so" ) + self.assertEqual( + process_raw_elements([SitePackageRawElement("bar", True)], ["foo"]), + [SitePackageElement("foo", "bar", True)], + ) finally: shutil.rmtree("foo") From f5a3d0a7f4facc957168ba8857fc5eaf8b316ca7 Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Sun, 10 Sep 2023 12:35:28 +0800 Subject: [PATCH 13/20] Rewrite unittest with module tempfile and func `ensure_directories_exist` --- .../configuration/tests/search_path_test.py | 74 +++++++++---------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/client/configuration/tests/search_path_test.py b/client/configuration/tests/search_path_test.py index 593fbbb465d..f4fea804e5a 100644 --- a/client/configuration/tests/search_path_test.py +++ b/client/configuration/tests/search_path_test.py @@ -3,7 +3,6 @@ # This source code is licensed under the MIT license found in the # LICENSE file in the root directory of this source tree. -import shutil import tempfile from pathlib import Path @@ -71,47 +70,45 @@ def test_create_raw_element(self) -> None: ) def test_path(self) -> None: - Path.mkdir(Path("foo")) - Path.mkdir(Path("foo/bar")) - - try: + with tempfile.TemporaryDirectory() as temp_root: + ensure_directories_exists(Path(temp_root), ("foo", "foo/bar")) self.assertEqual(SimpleElement("foo").path(), "foo") self.assertEqual(SubdirectoryElement("foo", "bar").path(), "foo/bar") - self.assertEqual(SitePackageElement("foo", "bar").path(), "foo/bar") - finally: - shutil.rmtree("foo") + self.assertEqual( + SitePackageElement(f"{temp_root}/foo", "bar").path(), + f"{temp_root}/foo/bar", + ) def test_command_line_argument(self) -> None: - Path.mkdir(Path("foo")) - Path.mkdir(Path("foo/bar")) - - try: + with tempfile.TemporaryDirectory() as temp_root: + ensure_directories_exists(Path(temp_root), ("foo", "foo/bar")) self.assertEqual(SimpleElement("foo").command_line_argument(), "foo") self.assertEqual( SubdirectoryElement("foo", "bar").command_line_argument(), "foo$bar", ) self.assertEqual( - SitePackageElement("foo", "bar").command_line_argument(), - "foo$bar", + SitePackageElement(f"{temp_root}/foo", "bar").command_line_argument(), + f"{temp_root}/foo$bar", ) - finally: - shutil.rmtree("foo") - Path.mkdir(Path("foo")) - Path.mkdir(Path("foo/bar-1.0.0.dist-info")) - Path.touch(Path("foo/bar.py")) + with tempfile.TemporaryDirectory() as temp_root: + ensure_directories_exists( + Path(temp_root), ("foo", "foo/bar-1.0.0.dist-info") + ) + Path.touch(Path(f"{temp_root}/foo/bar.py")) - with open("foo/bar-1.0.0.dist-info/RECORD", "w", encoding="UTF-8") as f: - f.write("bar.py") + with open( + f"{temp_root}/foo/bar-1.0.0.dist-info/RECORD", "w", encoding="UTF-8" + ) as f: + f.write("bar.py,,") - try: self.assertEqual( - SitePackageElement("foo", "bar", True).command_line_argument(), - "foo$bar.py", + SitePackageElement( + f"{temp_root}/foo", "bar", True + ).command_line_argument(), + f"{temp_root}/foo$bar.py", ) - finally: - shutil.rmtree("foo") def test_expand_global_root(self) -> None: self.assertEqual( @@ -267,20 +264,23 @@ def test_process_required_raw_elements_site_package_nonexistence(self) -> None: ) def test_toplevel_module_not_pyfile(self) -> None: - Path.mkdir(Path("foo")) - Path.mkdir(Path("foo/bar-1.0.0.dist-info")) - Path.touch(Path("foo/bar.so")) + with tempfile.TemporaryDirectory() as temp_root: + ensure_directories_exists( + Path(temp_root), ("foo", "foo/bar-1.0.0.dist-info") + ) + Path.touch(Path(f"{temp_root}/foo/bar.so")) - with open("foo/bar-1.0.0.dist-info/RECORD", "w", encoding="UTF-8") as f: - f.write("bar.so") + with open( + f"{temp_root}/foo/bar-1.0.0.dist-info/RECORD", "w", encoding="UTF-8" + ) as f: + f.write("bar.so,,") - try: self.assertEqual( - SitePackageElement("foo", "bar", True).path(), "foo/bar.so" + SitePackageElement(f"{temp_root}/foo", "bar", True).path(), "foo/bar.so" ) self.assertEqual( - process_raw_elements([SitePackageRawElement("bar", True)], ["foo"]), - [SitePackageElement("foo", "bar", True)], + process_raw_elements( + [SitePackageRawElement("bar", True)], [f"{temp_root}/foo"] + ), + [SitePackageElement(f"{temp_root}/foo", "bar", True)], ) - finally: - shutil.rmtree("foo") From 4cafef7d1661d3802b432fcf928601a5373a3254 Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Sun, 10 Sep 2023 12:40:01 +0800 Subject: [PATCH 14/20] Rewrite client/backend_arguments.py's func get_checked_directory_allowlist with set-comprehension --- client/backend_arguments.py | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/client/backend_arguments.py b/client/backend_arguments.py index 78d1ac9015f..685ba078b3d 100644 --- a/client/backend_arguments.py +++ b/client/backend_arguments.py @@ -69,14 +69,7 @@ def serialize(self) -> Dict[str, object]: } def get_checked_directory_allowlist(self) -> Set[str]: - paths = set() - - for element in self.elements: - excepted_path = element.path() - if excepted_path is not None: - paths.add(excepted_path) - - return paths + return {element.path() for element in self.elements if element is not None} def cleanup(self) -> None: pass @@ -105,14 +98,7 @@ def serialize(self) -> Dict[str, object]: } def get_checked_directory_allowlist(self) -> Set[str]: - paths = set() - - for element in self.elements: - excepted_path = element.path() - if excepted_path is not None: - paths.add(excepted_path) - - return paths + return {element.path() for element in self.elements if element is not None} def cleanup(self) -> None: pass From 179192ebb9adf82fc2bfd8741f65fe6b5dcc8233 Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Sun, 10 Sep 2023 12:46:00 +0800 Subject: [PATCH 15/20] Fix unittest --- client/configuration/tests/search_path_test.py | 3 ++- client/tests/backend_arguments_test.py | 15 ++++++--------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/client/configuration/tests/search_path_test.py b/client/configuration/tests/search_path_test.py index f4fea804e5a..0c3c30516c8 100644 --- a/client/configuration/tests/search_path_test.py +++ b/client/configuration/tests/search_path_test.py @@ -276,7 +276,8 @@ def test_toplevel_module_not_pyfile(self) -> None: f.write("bar.so,,") self.assertEqual( - SitePackageElement(f"{temp_root}/foo", "bar", True).path(), "foo/bar.so" + SitePackageElement(f"{temp_root}/foo", "bar", True).path(), + f"{temp_root}/foo/bar.so", ) self.assertEqual( process_raw_elements( diff --git a/client/tests/backend_arguments_test.py b/client/tests/backend_arguments_test.py index 558c504775c..e08ae09eb33 100644 --- a/client/tests/backend_arguments_test.py +++ b/client/tests/backend_arguments_test.py @@ -3,7 +3,6 @@ # This source code is licensed under the MIT license found in the # LICENSE file in the root directory of this source tree. -import shutil import tempfile from pathlib import Path from typing import Iterable, Tuple @@ -577,22 +576,20 @@ def test_get_source_path__confliciting_source_specified(self) -> None: ) def test_get_checked_directory_for_simple_source_path(self) -> None: - Path.mkdir(Path("super")) - Path.mkdir(Path("super/slash")) + with tempfile.TemporaryDirectory() as temp_root: + Path.mkdir(Path(f"{temp_root}/super")) + Path.mkdir(Path(f"{temp_root}/super/slash")) - element0 = search_path.SimpleElement("ozzie") - element1 = search_path.SubdirectoryElement("diva", "flea") - element2 = search_path.SitePackageElement("super", "slash") + element0 = search_path.SimpleElement("ozzie") + element1 = search_path.SubdirectoryElement("diva", "flea") + element2 = search_path.SitePackageElement(f"{temp_root}/super", "slash") - try: self.assertCountEqual( SimpleSourcePath( [element0, element1, element2, element0] ).get_checked_directory_allowlist(), [element0.path(), element1.path(), element2.path()], ) - finally: - shutil.rmtree("super") def test_get_checked_directory_for_buck_source_path(self) -> None: self.assertCountEqual( From 8717c8ae037762d1f33ee67ee80c3efffb03ea88 Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Sun, 10 Sep 2023 12:50:40 +0800 Subject: [PATCH 16/20] Add pyre-fixme to avoid false positive type error --- client/backend_arguments.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/client/backend_arguments.py b/client/backend_arguments.py index 685ba078b3d..bd3b59c4bb0 100644 --- a/client/backend_arguments.py +++ b/client/backend_arguments.py @@ -69,6 +69,7 @@ def serialize(self) -> Dict[str, object]: } def get_checked_directory_allowlist(self) -> Set[str]: + # pyre-fixme: Incompatible return type [7]: Expected Set[str] but got Set[Optional[str]]. return {element.path() for element in self.elements if element is not None} def cleanup(self) -> None: @@ -98,6 +99,7 @@ def serialize(self) -> Dict[str, object]: } def get_checked_directory_allowlist(self) -> Set[str]: + # pyre-fixme: Incompatible return type [7]: Expected Set[str] but got Set[Optional[str]]. return {element.path() for element in self.elements if element is not None} def cleanup(self) -> None: From 6bedb969a692e4d781f0afe465fcc212fc65202d Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Fri, 15 Sep 2023 18:30:36 +0800 Subject: [PATCH 17/20] Use comment to specify the re.Pattern type --- client/configuration/search_path.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/client/configuration/search_path.py b/client/configuration/search_path.py index 3528df7a444..f3701d9f371 100644 --- a/client/configuration/search_path.py +++ b/client/configuration/search_path.py @@ -29,11 +29,9 @@ dist_info_in_root: Dict[str, List[str]] = {} -# pyre-fixme[5]: Globally accessible variable `_site_filter` has type `re.Pattern[str]` but no type is specified. -_site_filter = re.compile(r".*-([0-99]\.)*dist-info") +_site_filter = re.compile(r".*-([0-99]\.)*dist-info") # type: re.Pattern[str] -# pyre-fixme[5]: Globally accessible variable `_PYCACHE` has type `re.Pattern[str]` but no type is specified. -_PYCACHE = re.compile("__pycache__(/)*.*") +_PYCACHE = re.compile("__pycache__(/)*.*") # type: re.Pattern[str] def _expand_relative_root(path: str, relative_root: str) -> str: From fc25a8cf7a3a8000d617bc175fb4e9eb24dacfef Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Fri, 15 Sep 2023 19:06:20 +0800 Subject: [PATCH 18/20] Fix typos --- client/configuration/search_path.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/configuration/search_path.py b/client/configuration/search_path.py index f3701d9f371..eb5614bd494 100644 --- a/client/configuration/search_path.py +++ b/client/configuration/search_path.py @@ -270,8 +270,8 @@ def process_raw_elements( elements: List[Element] = [] def add_if_exists(element: Element) -> bool: - excepted_path = element.path() - if excepted_path is None or not os.path.exists(excepted_path): + expected_path = element.path() + if expected_path is None or not os.path.exists(expected_path): return False elements.append(element) return True From 809571b115bb3e5a64e1864670794facf54e40fa Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Fri, 15 Sep 2023 19:12:44 +0800 Subject: [PATCH 19/20] Fix incompatible return type in get_checked_directory_allowlist func --- client/backend_arguments.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/client/backend_arguments.py b/client/backend_arguments.py index bd3b59c4bb0..e83dca58ec3 100644 --- a/client/backend_arguments.py +++ b/client/backend_arguments.py @@ -69,8 +69,11 @@ def serialize(self) -> Dict[str, object]: } def get_checked_directory_allowlist(self) -> Set[str]: - # pyre-fixme: Incompatible return type [7]: Expected Set[str] but got Set[Optional[str]]. - return {element.path() for element in self.elements if element is not None} + allow_list = set() + for element in self.elements: + if expected_element_path := element.path() is not None: + allow_list.add(expected_element_path) + return allow_list def cleanup(self) -> None: pass @@ -99,8 +102,11 @@ def serialize(self) -> Dict[str, object]: } def get_checked_directory_allowlist(self) -> Set[str]: - # pyre-fixme: Incompatible return type [7]: Expected Set[str] but got Set[Optional[str]]. - return {element.path() for element in self.elements if element is not None} + allow_list = set() + for element in self.elements: + if expected_element_path := element.path() is not None: + allow_list.add(expected_element_path) + return allow_list def cleanup(self) -> None: pass From 1edfbc10bd6fc81994c28dd82de030888b688f87 Mon Sep 17 00:00:00 2001 From: Wang <120731947+WangGithubUser@users.noreply.github.com> Date: Fri, 15 Sep 2023 19:25:54 +0800 Subject: [PATCH 20/20] Use comment to specify type of not_toplevel_patterns --- client/configuration/search_path.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/client/configuration/search_path.py b/client/configuration/search_path.py index eb5614bd494..6af6172f8fa 100644 --- a/client/configuration/search_path.py +++ b/client/configuration/search_path.py @@ -85,7 +85,7 @@ def package_path(self) -> Union[str, None]: this_pkg_filter = re.compile( r"{}-([0-99]\.)*dist-info(/)*.*".format(self.package_name) - ) + ) # type: re.Pattern[str] if self.site_root not in dist_info_in_root: dist_info_in_root[self.site_root] = [] @@ -100,10 +100,10 @@ def package_path(self) -> Union[str, None]: else: return None - not_toplevel_patterns: Tuple[re.Pattern[str], re.Pattern[str]] = ( + not_toplevel_patterns = ( this_pkg_filter, _PYCACHE, - ) + ) # type: Tuple[re.Pattern[str], re.Pattern[str]] # pyre-fixme[61]: Local variable `dist_info_path` is undefined, or not always defined. with open(file=f"{dist_info_path}/RECORD", mode="r") as record: