Repository navigation
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 modifies docs/sphinx_build_test.py to use shutil.copyfile instead of shutil.copy2 when copying the README.md file to a temporary directory, and explicitly sets the destination file's permissions to 0o644 using os.chmod. There are no review comments, and I have no feedback to provide.
|
RBE => Remote Build Execution |
|
Anyone having a look at this ? |
|
Looks like the CI failure is due to some docker glitch |
oharboe
left a comment
oharboe
left a comment
There was a problem hiding this comment.
The underlying bug is copying permissions onto scratch files we then edit; RBE is just where it surfaced. Could the PR be framed that way? Suggested title:
docs: sphinx_build_test: don't copy permissions onto scratch files
Suggested description:
sphinx_build_test copies README.md into a tempdir because conf.py's setup() rewrites it in place. It used shutil.copy2, which also copies the source's permission bits, so a read-only input gave a read-only scratch copy and setup() failed. Use shutil.copyfile, which copies contents only.
The copytree on line 37 has the same pattern: it copies directory permissions, and setup() creates files inside the copied docs/. It may break the same way, though I haven't confirmed it.
The sphinx test was copying a source file and attempting to modify that. However, in an RBE environment, source files are read-only, so the copy was also read-only. Fixed that by explicitly making it writable. Signed-off-by: Henner Zeller <h.zeller@acm.org>
a39f08c to
4360dd8
Compare
|
@hzeller I also think that this will be reviewed faster if you reframe it as handling read only source files case, such as Remote Batch Execution, RBE. I didn't know what RBE was. |
|
The important part for the code is that the file is writable, because that is what the code wants to do, so that is what the comment says and the programmer intents. (RBE is just the Bazel Remote Build Execution which we already partially use wit the remote cache. You yourself already fixed a writable issue before to fix something showing up under RBE #10823 ) |
|
@osamahammad21 I think this makes sense and is a really simple and safe change and I'm rooting for remote execution environments 🥺 |
2 participants
The sphinx test was copying a source file and attempting to modify that. However, in an RBE environment, source files are read-only, so the copy was also read-only. Fixed that by explicitly making it writable.