[path] Add warning for using checkout_dir
Change-Id: I4d356a8bdd5740713f517283a3fb4a9128c21e7e
Bug: 329113288
Reviewed-on: https://chromium-review.googlesource.com/c/infra/luci/recipes-py/+/5444880
Auto-Submit: Rob Mohr <mohrr@google.com>
Reviewed-by: Chan Li <chanli@chromium.org>
Commit-Queue: Chan Li <chanli@chromium.org>
diff --git a/README.recipes.md b/README.recipes.md
index e2a8db8..fcf7035 100644
--- a/README.recipes.md
+++ b/README.recipes.md
@@ -2873,7 +2873,7 @@
#### **class [PathApi](/recipe_modules/path/api.py#328)([RecipeApi](/recipe_engine/recipe_api.py#471)):**
-— **def [\_\_contains\_\_](/recipe_modules/path/api.py#588)(self, pathname: NamedBasePathsType):**
+  **@recipe_api.ignore_warnings('recipe_engine/CHECKOUT_DIR_DEPRECATED')**<br>— **def [\_\_contains\_\_](/recipe_modules/path/api.py#589)(self, pathname: NamedBasePathsType):**
This method is DEPRECATED.
@@ -2888,7 +2888,7 @@
a very complicated 'config' system. All of that has been removed, but this
method remains for now.
-— **def [\_\_getitem\_\_](/recipe_modules/path/api.py#691)(self, name: NamedBasePathsType):**
+— **def [\_\_getitem\_\_](/recipe_modules/path/api.py#702)(self, name: NamedBasePathsType):**
Gets the base path named `name`. See module docstring for more info.
@@ -2903,7 +2903,7 @@
pass the Paths around instead of using this global variable).
***
-— **def [\_\_setitem\_\_](/recipe_modules/path/api.py#606)(self, pathname: CheckoutPathNameType, path: config_types.Path):**
+  **@recipe_api.ignore_warnings('recipe_engine/CHECKOUT_DIR_DEPRECATED')**<br>— **def [\_\_setitem\_\_](/recipe_modules/path/api.py#608)(self, pathname: CheckoutPathNameType, path: config_types.Path):**
Sets the checkout path.
@@ -2913,7 +2913,7 @@
The only valid value of `pathname` is the literal string CheckoutPathName.
-— **def [abs\_to\_path](/recipe_modules/path/api.py#527)(self, abs_string_path: str):**
+  **@recipe_api.ignore_warnings('recipe_engine/CHECKOUT_DIR_DEPRECATED')**<br>— **def [abs\_to\_path](/recipe_modules/path/api.py#527)(self, abs_string_path: str):**
Converts an absolute path string `abs_string_path` to a real Path
object, using the most appropriate known base path.
@@ -2941,7 +2941,7 @@
Raises an ValueError if the preconditions are not met, otherwise returns the
Path object.
-— **def [abspath](/recipe_modules/path/api.py#801)(self, path: (config_types.Path | str)):**
+— **def [abspath](/recipe_modules/path/api.py#812)(self, path: (config_types.Path | str)):**
Equivalent to os.abspath.
@@ -2952,11 +2952,11 @@
Args:
* path - The path to check.
-— **def [basename](/recipe_modules/path/api.py#805)(self, path: (config_types.Path | str)):**
+— **def [basename](/recipe_modules/path/api.py#816)(self, path: (config_types.Path | str)):**
Equivalent to os.path.basename.
-  **@property**<br>— **def [cache\_dir](/recipe_modules/path/api.py#735)(self):**
+  **@property**<br>— **def [cache\_dir](/recipe_modules/path/api.py#746)(self):**
This directory is provided by whatever's running the recipe.
@@ -2979,7 +2979,7 @@
Note that directories created under here /may/ be evicted in between runs of
the recipe (i.e. to relieve disk pressure).
-— **def [cast\_to\_path](/recipe_modules/path/api.py#769)(self, strpath: str):**
+— **def [cast\_to\_path](/recipe_modules/path/api.py#780)(self, strpath: str):**
This returns a Path for strpath which can be used anywhere a Path is
required.
@@ -2992,20 +2992,20 @@
cache_dir), the returned Path will be based on that known path. This is
important for test compatibility.
-  **@checkout_dir.setter**<br>— **def [checkout\_dir](/recipe_modules/path/api.py#630)(self, path: config_types.Path):**
+  **@checkout_dir.setter**<br>— **def [checkout\_dir](/recipe_modules/path/api.py#634)(self, path: config_types.Path):**
Sets the global variable `api.path.checkout_dir` to the given path.
-  **@property**<br>— **def [cleanup\_dir](/recipe_modules/path/api.py#760)(self):**
+  **@property**<br>— **def [cleanup\_dir](/recipe_modules/path/api.py#771)(self):**
This directory is guaranteed to be cleaned up (eventually) after the
execution of this recipe.
This directory is guaranteed to be empty when the recipe starts.
-— **def [dirname](/recipe_modules/path/api.py#809)(self, path: (config_types.Path | str)):**
+— **def [dirname](/recipe_modules/path/api.py#820)(self, path: (config_types.Path | str)):**
For "foo/bar/baz", return "foo/bar".
@@ -3018,7 +3018,7 @@
Returns dirname of path
-— **def [eq](/recipe_modules/path/api.py#973)(self, path1: config_types.Path, path2: config_types.Path):**
+— **def [eq](/recipe_modules/path/api.py#984)(self, path1: config_types.Path, path2: config_types.Path):**
Check whether path1 points to the same path as path2.
@@ -3026,20 +3026,20 @@
**DEPRECATED**: Just directly compare path1 and path2 with `==`.
***
-— **def [exists](/recipe_modules/path/api.py#913)(self, path):**
+— **def [exists](/recipe_modules/path/api.py#924)(self, path):**
Equivalent to os.path.exists.
The presence or absence of paths can be mocked during the execution of the
recipe by using the mock_* methods.
-— **def [expanduser](/recipe_modules/path/api.py#904)(self, path):**
+— **def [expanduser](/recipe_modules/path/api.py#915)(self, path):**
Do not use this, use `api.path.home_dir` instead.
This ONLY handles `path` == "~", and returns `str(api.path.home_dir)`.
-— **def [get](/recipe_modules/path/api.py#659)(self, name: NamedBasePathsType, \*, skip_deprecation=False):**
+  **@recipe_api.ignore_warnings('recipe_engine/CHECKOUT_DIR_DEPRECATED')**<br>— **def [get](/recipe_modules/path/api.py#669)(self, name: NamedBasePathsType, \*, skip_deprecation=False):**
Gets the base path named `name`. See module docstring for more info.
@@ -3054,7 +3054,7 @@
pass the Paths around instead of using this global variable).
***
-  **@property**<br>— **def [home\_dir](/recipe_modules/path/api.py#717)(self):**
+  **@property**<br>— **def [home\_dir](/recipe_modules/path/api.py#728)(self):**
This is the path to the current $HOME directory.
@@ -3066,7 +3066,7 @@
This is called by the recipe engine immediately after __init__(), but
with `self._paths_client` initialized.
-— **def [is\_parent\_of](/recipe_modules/path/api.py#980)(self, parent: config_types.Path, child: config_types.Path):**
+— **def [is\_parent\_of](/recipe_modules/path/api.py#991)(self, parent: config_types.Path, child: config_types.Path):**
Check whether child is contained within parent.
@@ -3074,21 +3074,21 @@
**DEPRECATED**: Just use `parent.is_parent_of(child)`.
***
-— **def [isdir](/recipe_modules/path/api.py#921)(self, path):**
+— **def [isdir](/recipe_modules/path/api.py#932)(self, path):**
Equivalent to os.path.isdir.
The presence or absence of paths can be mocked during the execution of the
recipe by using the mock_* methods.
-— **def [isfile](/recipe_modules/path/api.py#929)(self, path):**
+— **def [isfile](/recipe_modules/path/api.py#940)(self, path):**
Equivalent to os.path.isfile.
The presence or absence of paths can be mocked during the execution of the
recipe by using the mock_* methods.
-— **def [join](/recipe_modules/path/api.py#828)(self, path, \*paths):**
+— **def [join](/recipe_modules/path/api.py#839)(self, path, \*paths):**
Equivalent to os.path.join.
@@ -3126,23 +3126,23 @@
either a resource script of your recipe module or recipe.
***
-— **def [mock\_add\_directory](/recipe_modules/path/api.py#948)(self, path: config_types.Path):**
+— **def [mock\_add\_directory](/recipe_modules/path/api.py#959)(self, path: config_types.Path):**
For testing purposes, mark that file |path| exists.
-— **def [mock\_add\_file](/recipe_modules/path/api.py#944)(self, path: config_types.Path):**
+— **def [mock\_add\_file](/recipe_modules/path/api.py#955)(self, path: config_types.Path):**
For testing purposes, mark that file |path| exists.
-— **def [mock\_add\_paths](/recipe_modules/path/api.py#937)(self, path: config_types.Path, kind: FileType=FileType.FILE):**
+— **def [mock\_add\_paths](/recipe_modules/path/api.py#948)(self, path: config_types.Path, kind: FileType=FileType.FILE):**
For testing purposes, mark that |path| exists.
-— **def [mock\_copy\_paths](/recipe_modules/path/api.py#952)(self, source: config_types.Path, dest: config_types.Path):**
+— **def [mock\_copy\_paths](/recipe_modules/path/api.py#963)(self, source: config_types.Path, dest: config_types.Path):**
For testing purposes, copy |source| to |dest|.
-— **def [mock\_remove\_paths](/recipe_modules/path/api.py#959)(self, path: config_types.Path, should_remove: Callable[([str], bool)]=(lambda p: True)):**
+— **def [mock\_remove\_paths](/recipe_modules/path/api.py#970)(self, path: config_types.Path, should_remove: Callable[([str], bool)]=(lambda p: True)):**
For testing purposes, mark that |path| doesn't exist.
@@ -3151,34 +3151,34 @@
should_remove: Called for every candidate path. Return True to remove this
path.
-— **def [normpath](/recipe_modules/path/api.py#900)(self, path):**
+— **def [normpath](/recipe_modules/path/api.py#911)(self, path):**
Equivalent to os.path.normpath.
-  **@property**<br>— **def [pardir](/recipe_modules/path/api.py#786)(self):**
+  **@property**<br>— **def [pardir](/recipe_modules/path/api.py#797)(self):**
Equivalent to os.pardir.
-  **@property**<br>— **def [pathsep](/recipe_modules/path/api.py#796)(self):**
+  **@property**<br>— **def [pathsep](/recipe_modules/path/api.py#807)(self):**
Equivalent to os.pathsep.
-— **def [realpath](/recipe_modules/path/api.py#888)(self, path: (config_types.Path | str)):**
+— **def [realpath](/recipe_modules/path/api.py#899)(self, path: (config_types.Path | str)):**
Equivalent to os.path.realpath.
-— **def [relpath](/recipe_modules/path/api.py#892)(self, path, start):**
+— **def [relpath](/recipe_modules/path/api.py#903)(self, path, start):**
Roughly equivalent to os.path.relpath.
Unlike os.path.relpath, `start` is _required_. If you want the 'current
directory', use the `recipe_engine/context` module's `cwd` property.
-  **@property**<br>— **def [sep](/recipe_modules/path/api.py#791)(self):**
+  **@property**<br>— **def [sep](/recipe_modules/path/api.py#802)(self):**
Equivalent to os.sep.
-— **def [split](/recipe_modules/path/api.py#843)(self, path):**
+— **def [split](/recipe_modules/path/api.py#854)(self, path):**
For "foo/bar/baz", return ("foo/bar", "baz").
@@ -3192,7 +3192,7 @@
Returns (dirname(path), basename(path)).
-— **def [splitext](/recipe_modules/path/api.py#864)(self, path: (config_types.Path | str)):**
+— **def [splitext](/recipe_modules/path/api.py#875)(self, path: (config_types.Path | str)):**
For "foo/bar.baz", return ("foo/bar", ".baz").
@@ -3207,7 +3207,7 @@
Returns:
(name, extension_including_dot).
-  **@property**<br>— **def [start\_dir](/recipe_modules/path/api.py#706)(self):**
+  **@property**<br>— **def [start\_dir](/recipe_modules/path/api.py#717)(self):**
This is the directory that the recipe started in. it's similar to `cwd`,
except that it's constant for the duration of the entire program.
@@ -3216,7 +3216,7 @@
See the 'recipe_engine/context' module which allows modifying the cwd safely
via a context manager.
-  **@property**<br>— **def [tmp\_base\_dir](/recipe_modules/path/api.py#726)(self):**
+  **@property**<br>— **def [tmp\_base\_dir](/recipe_modules/path/api.py#737)(self):**
This directory is the system-configured temp dir.
@@ -5755,24 +5755,24 @@
  **@recipe_api.ignore_warnings('recipe_engine/CHECKOUT_DIR_DEPRECATED', 'recipe_engine/PATH_GETITEM_DEPRECATED')**<br>— **def [RunSteps](/recipe_modules/path/tests/deprecated.py#13)(api):**
### *recipes* / [path:tests/dynamic\_paths](/recipe_modules/path/tests/dynamic_paths.py)
-[DEPS](/recipe_modules/path/tests/dynamic_paths.py#7): [path](#recipe_modules-path)
+[DEPS](/recipe_modules/path/tests/dynamic_paths.py#8): [path](#recipe_modules-path)
-— **def [RunSteps](/recipe_modules/path/tests/dynamic_paths.py#10)(api):**
+  **@recipe_api.ignore_warnings('recipe_engine/CHECKOUT_DIR_DEPRECATED')**<br>— **def [RunSteps](/recipe_modules/path/tests/dynamic_paths.py#11)(api):**
### *recipes* / [path:tests/exists](/recipe_modules/path/tests/exists.py)
-[DEPS](/recipe_modules/path/tests/exists.py#7): [path](#recipe_modules-path)
+[DEPS](/recipe_modules/path/tests/exists.py#8): [path](#recipe_modules-path)
-— **def [RunSteps](/recipe_modules/path/tests/exists.py#10)(api):**
+  **@recipe_api.ignore_warnings('recipe_engine/CHECKOUT_DIR_DEPRECATED')**<br>— **def [RunSteps](/recipe_modules/path/tests/exists.py#11)(api):**
### *recipes* / [path:tests/test\_api\_legacy](/recipe_modules/path/tests/test_api_legacy.py)
-[DEPS](/recipe_modules/path/tests/test_api_legacy.py#8): [path](#recipe_modules-path)
+[DEPS](/recipe_modules/path/tests/test_api_legacy.py#9): [path](#recipe_modules-path)
Test to cover legacy aspects of PathTestApi.
-— **def [RunSteps](/recipe_modules/path/tests/test_api_legacy.py#19)(api):**
+  **@recipe_api.ignore_warnings('recipe_engine/CHECKOUT_DIR_DEPRECATED')**<br>— **def [RunSteps](/recipe_modules/path/tests/test_api_legacy.py#20)(api):**
### *recipes* / [placeholder](/recipes/placeholder.py)
[DEPS](/recipes/placeholder.py#5): [buildbucket](#recipe_modules-buildbucket), [properties](#recipe_modules-properties), [step](#recipe_modules-step), [swarming](#recipe_modules-swarming), [time](#recipe_modules-time)
diff --git a/recipe.warnings b/recipe.warnings
index a544118..380fb45 100644
--- a/recipe.warnings
+++ b/recipe.warnings
@@ -101,3 +101,39 @@
deadline: "2024-06-01"
google_issue { id: 332776145 }
}
+
+warning {
+ name: "CHECKOUT_DIR_DEPRECATED"
+ description: "api.path.checkout_dir is deprecated."
+ description: ""
+ description: "This value acts like a global variable and leads to very"
+ description: "confusing code. Instead of this, prefer the following (in order"
+ description: "from most to least desirable):"
+ description: ""
+ description: "1) Modify functions which inspect api.path.checkout_dir to"
+ description: "instead directly take the directory to operate on."
+ description: ""
+ description: "2) Modify functions to use the recipe_engine/context module's"
+ description: "cwd value. This can still be confusing if abused, because it"
+ description: "changes the behavior of the function based on the context from"
+ description: "which it's called. However, for some 'leaf' functions which are"
+ description: "just directly invoking a tool in a directory, this can be"
+ description: "acceptable. Unlike the checkout_dir value in path, the context"
+ description: "module keeps per-thread state, which means that concurrent"
+ description: "futures in the recipe will each see their own value as"
+ description: "appropriate for their own call stack."
+ description: ""
+ description: "3) Introduce your own 'global' path to a module that you own,"
+ description: "and then use this. This doesn't solve the code clarity problem"
+ description: "directly, but can be a useful transition for code which heavily"
+ description: "relies on the existing checkout_dir within its own codebase."
+ description: "This will not solve the problem for common modules which use"
+ description: "the checkout_dir global variable without a way to explicitly"
+ description: "pass in a directory, however."
+ description: ""
+ description: "If you pick this third path, please consider making this global"
+ description: "only assignable once, and making it a hard error (e.g. raise"
+ description: "ValueError) if this is accessed prior to assignment."
+ deadline: "2024-06-15"
+ google_issue { id: 329113288 }
+}
diff --git a/recipe_modules/path/api.py b/recipe_modules/path/api.py
index 5725032..a81a9c6 100644
--- a/recipe_modules/path/api.py
+++ b/recipe_modules/path/api.py
@@ -524,6 +524,7 @@
self.mock_add_paths(temp_file, FileType.FILE)
return temp_file
+ @recipe_api.ignore_warnings('recipe_engine/CHECKOUT_DIR_DEPRECATED')
def abs_to_path(self, abs_string_path: str) -> config_types.Path:
"""Converts an absolute path string `abs_string_path` to a real Path
object, using the most appropriate known base path.
@@ -585,6 +586,7 @@
sub_path = abs_string_path[len(sPath):].strip(self.sep)
return path.joinpath(*sub_path.split(self.sep))
+ @recipe_api.ignore_warnings('recipe_engine/CHECKOUT_DIR_DEPRECATED')
def __contains__(self, pathname: NamedBasePathsType) -> bool:
"""This method is DEPRECATED.
@@ -603,6 +605,7 @@
return bool(self._checkout_dir)
return pathname in self.NamedBasePaths
+ @recipe_api.ignore_warnings('recipe_engine/CHECKOUT_DIR_DEPRECATED')
def __setitem__(self, pathname: CheckoutPathNameType,
path: config_types.Path) -> None:
"""Sets the checkout path.
@@ -621,6 +624,7 @@
@property
def checkout_dir(self) -> config_types.Path|None:
"""Returns the Path which was assigned to this checkout_dir property."""
+ self.m.warning.issue('CHECKOUT_DIR_DEPRECATED')
if cdir := self._checkout_dir:
# If the checkout_dir is already set, just return it directly.
return cdir
@@ -632,6 +636,8 @@
"""Sets the global variable `api.path.checkout_dir` to the given path.
"""
+ self.m.warning.issue('CHECKOUT_DIR_DEPRECATED')
+
if not isinstance(path, config_types.Path):
raise ValueError(
f'api.path.checkout_dir called with bad type: {path!r} ({type(path)})')
@@ -656,6 +662,11 @@
assert isinstance(self._path_mod, fake_path)
self._path_mod._mock_path_exists.mark_checkout_dir_set()
+ # We can always ignore checkout_dir warnings from this method because all
+ # uses of this method produce PATH_GETITEM_DEPRECATED warnings. When those
+ # get fixed, callers will be accessing api.path.checkout_dir directly and
+ # will then hit the CHECKOUT_DIR_DEPRECATED warning.
+ @recipe_api.ignore_warnings('recipe_engine/CHECKOUT_DIR_DEPRECATED')
def get(self, name: NamedBasePathsType, *,
skip_deprecation=False) -> config_types.Path:
"""Gets the base path named `name`. See module docstring for more info.
diff --git a/recipe_modules/path/tests/dynamic_paths.py b/recipe_modules/path/tests/dynamic_paths.py
index 7421d44..08234f8 100644
--- a/recipe_modules/path/tests/dynamic_paths.py
+++ b/recipe_modules/path/tests/dynamic_paths.py
@@ -2,11 +2,13 @@
# Use of this source code is governed under the Apache License, Version 2.0
# that can be found in the LICENSE file.
+from recipe_engine import recipe_api
from recipe_engine.post_process import DropExpectation
DEPS = ['path']
+@recipe_api.ignore_warnings('recipe_engine/CHECKOUT_DIR_DEPRECATED')
def RunSteps(api):
try:
api.path.checkout_dir = 'hello'
diff --git a/recipe_modules/path/tests/exists.py b/recipe_modules/path/tests/exists.py
index 04d329e..6b718bd 100644
--- a/recipe_modules/path/tests/exists.py
+++ b/recipe_modules/path/tests/exists.py
@@ -2,11 +2,13 @@
# Use of this source code is governed under the Apache License, Version 2.0
# that can be found in the LICENSE file.
+from recipe_engine import recipe_api
from recipe_engine.post_process import DropExpectation
DEPS = ['path']
+@recipe_api.ignore_warnings('recipe_engine/CHECKOUT_DIR_DEPRECATED')
def RunSteps(api):
assert not api.path.exists(api.path.start_dir / 'does not exist')
assert not api.path.isfile(api.path.start_dir / 'does not exist')
diff --git a/recipe_modules/path/tests/test_api_legacy.py b/recipe_modules/path/tests/test_api_legacy.py
index b4da7d8..dac335d 100644
--- a/recipe_modules/path/tests/test_api_legacy.py
+++ b/recipe_modules/path/tests/test_api_legacy.py
@@ -3,6 +3,7 @@
# that can be found in the LICENSE file.
"""Test to cover legacy aspects of PathTestApi."""
+from recipe_engine import recipe_api
from recipe_engine.post_process import DropExpectation
DEPS = ['path']
@@ -16,6 +17,7 @@
]
+@recipe_api.ignore_warnings('recipe_engine/CHECKOUT_DIR_DEPRECATED')
def RunSteps(api):
for name in GETATTR_NAMES:
p = getattr(api.path, name) / 'file'
