-
Notifications
You must be signed in to change notification settings - Fork 1k
Refactor cache save to create a list of files to compress and reuse existing function #18320
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
Refactor cache save to create a list of files to compress and reuse existing function #18320
Conversation
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.
Looks good, just might be worth double checking on the pkglist.json
change possible effects
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.
Besides the already proposed test, this looks great
Co-authored-by: James <[email protected]>
save(pkglist_path, serialized) | ||
tar_files["pkglist.json"] = pkglist_path | ||
compress_files(tar_files, os.path.basename(tgz_path), os.path.dirname(tgz_path), compresslevel, recursive=True) | ||
remove(pkglist_path) |
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.
If we want to keep this remove, shouldn't it be affecting the whole 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.
What do you mean? I am not sure I understood your comment
if os.path.exists(path): | ||
tgz.add(path, f"{recipe_folder}/{f}", recursive=True) | ||
path = os.path.join(cache_folder, recipe_folder, DOWNLOAD_EXPORT_FOLDER, METADATA) | ||
tar_files: dict[str,str] = {} # {path_in_tar: abs_path} |
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.
😩
Changelog: omit
Docs: omit
As suggested in #18314 (comment)
This PR will simplify #18314 by modifying the compression functionality on cache save command.
Intead of creating a tar object and adding files one by one (very coupled interface with tarfile module), create a list of files which will be passed to a already existing method in conan
compress_files
.This method may be moved to another module in a future PR.