[9.0.1] Force rctx.{download_and,}extract to create user-readable files (https://github.com/bazelbuild/bazel/pull/28531)#28649
Conversation
…zelbuild#28531) Archives in the wild do sometimes contain non-readable files, but other tools work around this and thus mask their brokenness. Context: https://bazelbuild.slack.com/archives/CDCMRLS23/p1770213515354229 Closes bazelbuild#28531. PiperOrigin-RevId: 865960367 Change-Id: I7273eb983d63d6960d184764cec5040bba77b2c2
bazel-io
added
team-ExternalDeps
![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 correctly ensures that files extracted from various archive types (.ar, .tar, .zip) are always user-readable by adding the 0400 permission bit. The change is consistent across all relevant decompressor functions and is well-tested with new shell tests that verify the behavior for each archive type. My review includes suggestions to improve code maintainability by documenting the magic number used for the permission bit.
| filePath.chmod(entry.getMode()); | ||
| // Ensure that all files are at least user-readable. Some archives contain files that | ||
| // are not, but many other tools are working around this and thus mask these issues. | ||
| filePath.chmod(entry.getMode() | 0400); |
There was a problem hiding this comment.
To improve readability and maintainability, it's good practice to avoid magic numbers. Since defining a shared constant might be outside the scope of this change, consider adding a comment to clarify the meaning of 0400.
| filePath.chmod(entry.getMode() | 0400); | |
| filePath.chmod(entry.getMode() | 0400 /* S_IRUSR */); |
| filePath.chmod(entry.getMode()); | ||
| // Ensure that all files are at least user-readable. Some archives contain files that | ||
| // are not, but many other tools are working around this and thus mask these issues. | ||
| filePath.chmod(entry.getMode() | 0400); |
There was a problem hiding this comment.
To improve readability and maintainability, it's good practice to avoid magic numbers. Since defining a shared constant might be outside the scope of this change, consider adding a comment to clarify the meaning of 0400.
| filePath.chmod(entry.getMode() | 0400); | |
| filePath.chmod(entry.getMode() | 0400 /* S_IRUSR */); |
| outputPath.chmod(permissions); | ||
| // Ensure that all files are at least user-readable. Some archives contain files that | ||
| // are not, but many other tools are working around this and thus mask these issues. | ||
| outputPath.chmod(permissions | 0400); |
There was a problem hiding this comment.
To improve readability and maintainability, it's good practice to avoid magic numbers. Since defining a shared constant might be outside the scope of this change, consider adding a comment to clarify the meaning of 0400.
| outputPath.chmod(permissions | 0400); | |
| outputPath.chmod(permissions | 0400 /* S_IRUSR */); |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
155c1fc
github-actions
bot
removed
the
awaiting-review
4 participants
Archives in the wild do sometimes contain non-readable files, but other tools work around this and thus mask their brokenness.
Context: https://bazelbuild.slack.com/archives/CDCMRLS23/p1770213515354229
Closes #28531.
PiperOrigin-RevId: 865960367
Change-Id: I7273eb983d63d6960d184764cec5040bba77b2c2
Commit 0bb7836