[Build] Supports repeated execution of setup-dev.py#61357
[Build] Supports repeated execution of setup-dev.py#61357edoakes merged 4 commits intoray-project:masterfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the idempotency of the setup-dev.py script by adding a check to skip symlinking if it's already done, and by ensuring the temporary directory for ray.serve is cleaned up on each run. The changes are good, but there is a potential permission issue with directory creation that could cause the script to fail when run with sudo. I've left a comment with a suggestion to fix it.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
summaryzb
force-pushed
the
master
branch
2 times, most recently
from
ec11644 to
6d0e9d8
Compare
Signed-off-by: summaryzb <summaryzb@gmail.com>
Signed-off-by: summaryzb <summaryzb@gmail.com>
|
gentle ping @bveeramani @myandpr |
Signed-off-by: summaryzb <summaryzb@gmail.com>
Signed-off-by: summaryzb <summaryzb@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| if os.path.exists(serve_temp_dir): | ||
| subprocess.check_call(sudo + ["rm", "-rf", serve_temp_dir]) | ||
| subprocess.check_call(sudo + ["mkdir", "-p", serve_temp_dir]) | ||
| subprocess.check_call(sudo + ["mv", generated_folder, serve_temp_dir]) |
There was a problem hiding this comment.
Sudo inconsistency between temp dir operations and move-back
Medium Severity
The new code creates /tmp/ray/_serve/ and moves files into it using sudo (lines 91–94), making the directory root-owned. However, the move-back operation at line 111 does not use sudo. When sudo is active, mv without elevated permissions will fail because the user lacks write permission on the root-owned temp directory to remove the entry. The old code was consistent — it never used sudo for temp dir operations, so the directory was always user-owned and the move-back worked.
Additional Locations (1)
bveeramani
left a comment
bveeramani
left a comment
There was a problem hiding this comment.
High-level changes seems reasonable to me.
Will defer to @aslonnie for actual review
aslonnie
left a comment
aslonnie
left a comment
There was a problem hiding this comment.
we do not really own this file... reef team wants to remove this..
## Description When setup-dev.py is executed again, it's very likely encounter a `many symbolic link error` or a problem where the `/tmp/ray/_serve/ directory already exists`. If symbolic link exists just skip it ## Related issues ## Additional information --------- Signed-off-by: summaryzb <summaryzb@gmail.com> Signed-off-by: bittoby <bittoby@users.noreply.github.com>
## Description When setup-dev.py is executed again, it's very likely encounter a `many symbolic link error` or a problem where the `/tmp/ray/_serve/ directory already exists`. If symbolic link exists just skip it ## Related issues ## Additional information --------- Signed-off-by: summaryzb <summaryzb@gmail.com>
4 participants
Description
When setup-dev.py is executed again, it's very likely encounter a
many symbolic link erroror a problem where the/tmp/ray/_serve/ directory already exists.If symbolic link exists just skip it
Related issues
Additional information