Update overlay_directories.py #3158
Conversation
Changes made: Renamed functions to follow snake_case naming convention. Moved function docstrings inside the functions. Changed RuntimeError to FileNotFoundError in check_dir_exists function. Removed redundant use of os.path.abspath. Improved formatting for better readability. Simplified the logic in main function for creating symlinks. Added missing type hints for function arguments and return types.
There was a problem hiding this comment.
Looks ok to me, but I don't use the bazel build. @sjain-stanford can review/land
There was a problem hiding this comment.
Besides the formatting/code-style changes, are there any functional changes here? Please include in the PR description a little about the "why" besides the "what".
In addition, if you haven't already, please trigger Bazel CI on your fork/branch. Actions -> Bazel Build and Test -> Run Workflow -> select your branch and hit Run Workflow, and paste the run link as comment here. This is needed since Bazel CI is not run on PRs to main automatically (it runs post-merge).
| raise RuntimeError( | ||
| "Must be invoked with a python 3 interpreter but was %s" % | ||
| sys.executable) | ||
| def check_python_version(): |
There was a problem hiding this comment.
I'd stick to what's used in llvm here, as it makes "syncing" with upstream changes easier.
https://sourcegraph.com/github.com/llvm/llvm-project@main/-/blob/utils/bazel/overlay_directories.py
| def _symlink_abs(from_path, to_path): | ||
| if not os.path.exists(to_path): | ||
| os.symlink(os.path.abspath(from_path), os.path.abspath(to_path)) | ||
| def symlink_abs(from_path, to_path): |
| """Main function to overlay directories using symlinks.""" | ||
| for root, dirs, files in os.walk(args.overlay): | ||
| rel_root = os.path.relpath(root, start=args.overlay) | ||
| if rel_root != ".": | ||
| os.mkdir(os.path.join(args.target, rel_root)) | ||
|
|
||
| for file in files: | ||
| relpath = os.path.join(rel_root, file) | ||
| symlink_abs(os.path.join(args.overlay, relpath), | ||
| os.path.join(args.target, relpath)) | ||
|
|
||
| for src_entry in os.listdir(os.path.join(args.src, rel_root)): | ||
| if src_entry not in dirs: | ||
| relpath = os.path.join(rel_root, src_entry) | ||
| symlink_abs(os.path.join(args.src, relpath), | ||
| os.path.join(args.target, relpath)) |
There was a problem hiding this comment.
Any changes other than indentation here? I tried squinting but couldn't find.
|
Hi @r0cketdyne, can you please address the comments? |
Changes made:
Renamed functions to follow snake_case naming convention. Moved function docstrings inside the functions.
Changed RuntimeError to FileNotFoundError in check_dir_exists function.
Removed redundant use of os.path.abspath.
Improved formatting for better readability.
Simplified the logic in main function for creating symlinks.
Added missing type hints for function arguments and return types.