Uh oh!
There was an error while loading. Please reload this page.
Use pathlib in toolshed and ci helper scripts (part 7 of #2410) - #2496
Use pathlib in toolshed and ci helper scripts (part 7 of #2410)#2496LeSingh1 wants to merge 3 commits into
Conversation
Part 7 of the series proposed in NVIDIA#2410. Path joining and filesystem predicates in the toolshed and ci/tools helper scripts now go through pathlib. glob.glob in dump_cutile_b64.py becomes Path.glob, with the mtime key reading Path.stat(). Kept on os.path, with a comment where it is not obvious: - os.path.abspath in build_static_bitcode_input.py, since sys.path wants a str and Path.absolute() does not normalize. - os.path.isfile in check_generated_file_seals.py. That guard exists to skip anything that is not a readable regular file, and Path.is_file() is not a drop-in: it propagates OSError for errnos outside pathlib's ignore list (EACCES, ENAMETOOLONG) where os.path.isfile returns False. - os.path.normpath in check_spdx.py, which already carries its own comment. The plan on NVIDIA#2410 also listed a root conftest.py; there is no such file. The three conftest.py files live under cuda_pathfinder, cuda_core and cuda_bindings, and none of them use os.path. Verified locally: ci/tools/tests/test_check_release_notes.py passes (42 tests), and check_spdx.py and check_generated_file_seals.py produce output identical to the pre-change scripts when run over every tracked .py file. Signed-off-by: LeSingh1 <sshaurya914@gmail.com>
| def notes_path(package: str, version: str) -> str: | ||
| return os.path.join(package, "docs", "source", "release", f"{version}-notes.rst") | ||
| return str(Path(package, "docs", "source", "release", f"{version}-notes.rst")) |
There was a problem hiding this comment.
We can treat all functions in these toolshed helper scripts as private. (They are generally not imported from third-party code). So we can change the return type on this to Path and prevent more conversions back and forth.
There was a problem hiding this comment.
Done. repo_root is now a Path end to end — load_backport_branch, check_release_notes and validate_backport_decision all take Path, and --repo-root parses with type=Path. That removed the Path(repo_root) re-wrap inside the functions and 19 str(tmp_path) conversions in the tests. The five main() argv lists keep str() since those are command-line strings, which argparse converts back to Path.
| # os.path.isfile, not Path.is_file: this skips anything that is not a | ||
| # readable regular file, and Path.is_file raises on e.g. EACCES. | ||
| if not os.path.isfile(filepath): |
There was a problem hiding this comment.
As mentioned elsewhere, let's not worry about this behavioral difference. Update to Path.is_file
There was a problem hiding this comment.
Done in 4385757 (pushed before I saw the review, flagging so the thread is not left hanging): the checker uses Path(filepath).is_file() and the comment about the os.path.isfile behavioural difference is gone. Also swept the rest of the PR for the same pattern — toolshed/build_static_bitcode_input.py still had one os.path.abspath, now Path.resolve().
Per review: treat these helper scripts as private, so notes_path can return Path and drop the str/Path round-trip at its call site. Accept the behavioral change from os.path.isfile to Path.is_file in check_generated_file_seals.
LeSingh1
commented
Aug 7, 2026
Both addressed.
Left alone deliberately:
|
Follow-up to mdboom's review. - repo_root is now a Path end to end: load_backport_branch, check_release_notes and validate_backport_decision take Path, and --repo-root parses with type=Path. That removes the Path(repo_root) re-wrap inside the functions and the 19 str(tmp_path) conversions the tests needed to call them. The five main() argv lists keep str(): those are command-line strings, which argparse then turns back into a Path. - build_static_bitcode_input: the last os.path use (os.path.abspath) becomes Path.resolve(); the os import is now unused and is dropped.
Path joining and filesystem predicates in the helper scripts, plus
glob.glob→Path.globindump_cutile_b64.py.os.path.isfilestays incheck_generated_file_seals.py: that guard skips anything that is not a readable regular file, andPath.is_file()raises onEACCES/ENAMETOOLONGwhereos.path.isfilereturnsFalse.os.path.abspathstays wheresys.pathneeds astr.Not covered by the
cuda_pathfinderCI job, so I verified locally instead:ci/tools/tests/test_check_release_notes.pypasses (42 tests), and both pre-commit hooks produce output identical to the pre-change scripts when run over every tracked.pyfile.Part 7 of #2410.
Parts 5 and 6 (
cuda_coreandcuda_bindings, largelybuild_hooks.py) are not ready — I have no way to run a wheel build here, and given thePath.is_file()divergence noted on #2410 I would rather not ship unverifiable build-system path changes.