Added untar progress similar to existing unzip #17519
Conversation
There was a problem hiding this comment.
There are different pieces in this PR.
For the untargz it seems the same approach as used in the FileUploader makes sense, but reusing same code, prepared for later refactor.
For the zip uncompress, if it can use the same class FileProgress, then great, if not, maybe leave that untouched.
conan/tools/files/files.py
Outdated
| # NOT EXPOSED at `conan.tools.files` but used in tests | ||
| import tarfile | ||
| with tarfile.TarFile.open(filename, 'r:*') as tarredgzippedFile: | ||
| import io | ||
| class ProgressFileObject(io.FileIO): |
There was a problem hiding this comment.
We already have a class FileProgress in the FileUploader, maybe it is the time to extract it and reuse it. Or at least copy it as-is, and leave it for a later refactor. But it would be that at least the initial implementation is the same.
There was a problem hiding this comment.
I've extracted that FileProgress class and adapted it to different scenarios! See https://github.com/conan-io/conan/pull/17519/files#diff-9afbf22c7cefd3f7b642e9b1b988ba829d3c36e3092bfb6140d01ad0bba4c571R537
conan/tools/files/files.py
Outdated
| if current_percentage >= self.percentage + self.step: | ||
| if 100 - current_percentage == self.step: | ||
| current_percentage = 100 | ||
| self.output.rewrite_line(txt_msg % current_percentage) |
There was a problem hiding this comment.
Rewrites are not that great, specially for CI output, which result in endless and annoying logs if enabled, and not a trace at all if not enabled. The "timed-output" seems to be a good balance.
There was a problem hiding this comment.
We were already using output.rewrite_line for the unzip progress but I agree it could cause some inconvenience.
With this refactor, we could remove the rewrite by using FileProgress class.
There was a problem hiding this comment.
FileProgress wrapping the full file object, including the wrapping with open/close context is a bit risky, as the file descriptor will not be properly closed. This is one of the reasons why the previous wrapper was not inheriting from io.FileIO and wrapping the minimum necessary for the progress.
| @@ -549,3 +532,17 @@ def move_folder_contents(conanfile, src_folder, dst_folder): | |||
| os.rmdir(src_folder) | |||
| except OSError: | |||
| pass | |||
|
|
|||
|
|
|||
| class FileProgress(io.FileIO): | |||
There was a problem hiding this comment.
This file is not the right place for this class, this file should be intended only for recipe importable from conan.tools. Things from this module shouldn't be used by Conan internals, and this needs to be imported from the internal "upload" code.
| # NOT EXPOSED at `conan.tools.files` but used in tests | ||
| import tarfile | ||
| with tarfile.TarFile.open(filename, 'r:*') as tarredgzippedFile: | ||
| with tarfile.TarFile.open(fileobj=FileProgress(filename, msg="Uncompressing"), mode='r:*') as tarredgzippedFile: |
There was a problem hiding this comment.
This fileob is not being closed correctly, as it is not called with with FileProgres. It would be better if this file is being correctly closed
| pass | ||
|
|
||
| with zipfile.ZipFile(filename, "r") as z: | ||
| with zipfile.ZipFile(FileProgress(filename, msg="Unzipping", mode="r")) as z: |
There was a problem hiding this comment.
Same as above, file not being closed
Changelog: Feature: Add report progress while unpacking tarball files
Docs: omit
Closes #14589
developbranch, documenting this one.