-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix conan cache restore crashing with PermissionError when the cache contains read-only files
#20254
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop2
Are you sure you want to change the base?
Fix conan cache restore crashing with PermissionError when the cache contains read-only files
#20254
Changes from all commits
d216172
67ee68b
4f87b3e
62e41d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import json | ||
| import os | ||
| import shutil | ||
| import stat | ||
| import tarfile | ||
| import tempfile | ||
|
|
||
|
|
@@ -396,6 +397,11 @@ def restore(self, path) -> PackagesList: | |
| fileobj = the_tar.extractfile("pkglist.json") | ||
| pkglist = fileobj.read() | ||
| the_tar.extraction_filter = (lambda member, _: member) # fully_trusted (Py 3.14) | ||
| for member in the_tar.getmembers(): | ||
| # Extraction opens the destination for writing, read-only files would fail | ||
| dest = os.path.join(cache_folder, member.name) | ||
| if member.isfile() and os.path.isfile(dest) and not os.access(dest, os.W_OK): | ||
| os.chmod(dest, os.stat(dest).st_mode | stat.S_IWRITE) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't end up with the same permissions it had before. Is this expected?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly, this seems to be forcing some permissions that it didn't have before?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The I will improve the tests to reflect this, thanks for pointing it out!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not removing them instead? There is still something that concerns me a bit about this, I agree that the current approach of just unzipping on top of the current cache might be not great, maybe it is necessary to think the process of restoring a bit better, beyond a local change of permissions? |
||
| the_tar.extractall(path=cache_folder) | ||
| the_tar.close() | ||
|
|
||
|
|
@@ -434,7 +440,7 @@ def restore(self, path) -> PackagesList: | |
| if db_pkg_folder != unzipped_pkg_folder: | ||
| # If a previous package exists, like a previous restore, then remove it | ||
| if os.path.exists(pkg_layout.package()): | ||
| shutil.rmtree(pkg_layout.package()) | ||
| rmdir(pkg_layout.package()) | ||
| shutil.move(os.path.join(cache_folder, unzipped_pkg_folder), | ||
| pkg_layout.package()) | ||
| pref_bundle["package_folder"] = db_pkg_folder | ||
|
|
@@ -447,7 +453,7 @@ def restore(self, path) -> PackagesList: | |
| if db_metadata_folder != unzipped_metadata_folder: | ||
| # We need to put the package in the final location in the cache | ||
| if os.path.exists(pkg_layout.metadata()): | ||
| shutil.rmtree(pkg_layout.metadata()) | ||
| rmdir(pkg_layout.metadata()) | ||
| shutil.move(os.path.join(cache_folder, unzipped_metadata_folder), | ||
| pkg_layout.metadata()) | ||
| pref_bundle["metadata_folder"] = db_metadata_folder | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change is only applied to the files. Could there be a directory with write-only permissions that ends up causing a similar error?