hash check when installing packages from cache #3301

ombschervister · 2020-10-28T13:46:57Z

When installing packages, poetry uses cache to avoid redownloading files.
If cached package file is corrupted (due to failed download, etc), poetry will not work until you delete this file from cache directory manually.

This PR adds hash check when installing packages from cache: if hash is incorrect, the file will be redownloaded.

Pull Request Check List

Added tests for changed code.
Updated documentation for changed code.

earonesty · 2021-12-20T20:35:35Z

poetry/installation/chef.py

        cache_dir = self.get_cache_directory_for_link(link)

        archive_types = ["whl", "tar.gz", "tar.bz2", "bz2", "zip"]
        links = []
        for archive_type in archive_types:
            for archive in cache_dir.glob("*.{}".format(archive_type)):
-                links.append(Link(archive.as_uri()))
+


adding a test to https://github.com/python-poetry/poetry/blob/master/tests/installation/test_chef.py is needed to cement the contract that a bad link hash is not accepted

earonesty · 2021-12-20T20:39:39Z

fixes: #3326

joooeey · 2021-12-28T16:35:10Z

I think it would be better to have Poetry remove the broken file from the cache in place of the user. The cache is the software's responsiblity, not the user's.

ombschervister · 2022-01-03T22:14:48Z

@joooeey

I think it would be better to have Poetry remove the broken file from the cache in place of the user. The cache is the software's responsiblity, not the user's.

In this request, Poetry itself removes the broken files. When loading the library, Poettry warns about the corruption of the file in the cache, and then overwrites the file itself.

earonesty

needs documentation change i think

earonesty

some documentation should be added, but otherwise, looks good.

ombschervister · 2022-05-22T06:04:10Z

some documentation should be added, but otherwise, looks good.

Is this something I should do?
Do you mean that I should add comments to my code in the request?
Also I see the documentation in the docs folder but I don't see a place for such detailed implementation details

Secrus · 2022-05-22T21:11:42Z

some documentation should be added, but otherwise, looks good.

Is this something I should do? Do you mean that I should add comments to my code in the request? Also I see the documentation in the docs folder but I don't see a place for such detailed implementation details

Hi. When it comes to documentation, it's expected to add necessary changes to docs if needed. .md files in the docs directory are named the same as sections in the documentation on the Poetry website. However, in this case, I don't think there is a need for change. We don't have information in the documentation about how the cache is working.

Also, please merge with master branch and resolve conflicts.

Secrus · 2022-05-22T21:15:49Z

src/poetry/installation/chef.py

+                if link.hash:
+                    real_hash = get_file_hash(archive, link.hash_name)
+                    if real_hash != link.hash:
+                        logger = logging.getLogger(__name__)


loggers are usually declared on top of files as globals. Also, you need to add the logger to loggers in src.poetry.console.application in line 234.

I did, but after adding the logger to src.poetry.console.application, the messages began to be duplicated

Secrus · 2022-05-22T21:18:26Z

src/poetry/installation/chef.py

+                    real_hash = get_file_hash(archive, link.hash_name)
+                    if real_hash != link.hash:
+                        logger = logging.getLogger(__name__)
+                        logger.warning(f"cache of {link.filename} is corrupted")


Suggested change

logger.warning(f"cache of {link.filename} is corrupted")

logger.warning(f"Cache of {link.filename} is corrupted. It won't be used.")

Leaving only a warning that the cache is corrupted, might confuse user.

Changed to "Cache of {} is corrupted. The file will be reloaded."

Secrus · 2022-05-22T21:20:13Z

src/poetry/utils/helpers.py

+    with open(filepath, "rb") as f:
+
+        res_hash = hashlib.new(hash_name)
+
+        while True:
+            buffer = f.read(block_size)
+            if not buffer:
+                break
+
+            res_hash.update(buffer)
+
+        return res_hash.hexdigest()


Is there a need for that many empty lines?

Secrus · 2022-05-22T21:22:45Z

tests/utils/test_helpers.py

+def test_get_file_hash():
+
+    with tempfile.TemporaryDirectory() as tmp_dir:
+
+        test_data = [b"", b"12345", b"123" * 8000]
+
+        for i, data in enumerate(test_data):
+
+            tmp_file = Path(tmp_dir) / f"file{i}"
+            tmp_file.write_bytes(data)
+
+            real_hash = hashlib.md5(data).hexdigest()
+            assert get_file_hash(tmp_file, "md5") == real_hash


The same as above, I don't think you need that many empty lines.

ombschervister · 2022-05-23T16:33:29Z

src/poetry/installation/chef.py

@@ -42,10 +46,6 @@ def is_wheel(self, archive: Path) -> bool:
        return archive.suffix == ".whl"

    def get_cached_archive_for_link(self, link: Link) -> Link:
-        # If the archive is already a wheel, there is no need to cache it.
-        if link.is_wheel:
-            return link


Also whl was downloaded every poetry install, but at the same time saved to the cache folders.
So the cache exists but was useless.
So I removed this lines.

ombschervister · 2022-05-24T05:31:54Z

@Secrus should i do something else?

earonesty · 2022-07-12T14:08:28Z

@0xDEC0DE and @sdispater worked on a similar issue, maybe they can approve.

0xDEC0DE

Makes sense. One small suggestion, inline.

0xDEC0DE · 2022-07-12T16:14:42Z

src/poetry/installation/chef.py

+                            f"Cache of {link.filename} is corrupted. The file will be"
+                            " reloaded."


Perhaps including the expected+actual hashes in the error messages might help people spot weird problems?

e.g., if a bunch of cached files were truncated to 0 bytes, they'd all log with the same hash, and users would know something wacky had happened.

Or it might be pointless noise. Who knows?

Cherry picked from PR: python-poetry#3301 Without these changes, Poetry fails if a file in the cache is corrupt and it needs to be cleaned up manually

wagnerluis1982 · 2023-02-21T22:07:28Z

I think this PR is not relevant anymore (and must be closed), since, as far as I understand, current Poetry version already check hashes for every file, cached or not.

But, perhaps the author can salvage something. Two things come to my mind: (1) I didn't find on current code a dedicated test to verify cached hash mismatch, and (2) maybe it can be introduced the nice log messages when there is a cache mismatch.

github-actions · 2024-02-29T01:26:49Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

ombschervister force-pushed the master branch 2 times, most recently from 66c416b to 66c2267 Compare October 28, 2020 13:55

finswimmer requested a review from a team October 30, 2020 18:59

ombschervister force-pushed the master branch 4 times, most recently from d1ddcc6 to c23be60 Compare November 6, 2020 09:26

finswimmer added the kind/enhancement Not a bug or feature, but improves usability or performance label Nov 9, 2020

pietrodn mentioned this pull request Apr 19, 2021

poetry.lock hashes not checked when running poetry install #2422

Closed

3 tasks

earonesty reviewed Dec 20, 2021

View reviewed changes

This was referenced Dec 20, 2021

Handle invalidation of corrupt cache files #2674

Closed

BadZipFile using cached incomplete download #3326

Closed

ombschervister force-pushed the master branch 4 times, most recently from 1ee3f31 to 3e439a4 Compare January 3, 2022 21:56

ombschervister removed the request for review from a team January 3, 2022 22:26

earonesty approved these changes Feb 11, 2022

View reviewed changes

earonesty previously approved these changes May 5, 2022

View reviewed changes

Secrus requested a review from a team May 22, 2022 21:11

Secrus requested changes May 22, 2022

View reviewed changes

ombschervister dismissed earonesty’s stale review via b41b1b7 May 23, 2022 16:16

ombschervister force-pushed the master branch from 3e439a4 to b41b1b7 Compare May 23, 2022 16:16

ombschervister commented May 23, 2022

View reviewed changes

ombschervister force-pushed the master branch from a0c43e8 to d59b5c1 Compare May 24, 2022 04:51

ombschervister force-pushed the master branch 2 times, most recently from 3ca2284 to 54bfc99 Compare May 24, 2022 05:12

chef: check hash of cached files

66bd0dd

ombschervister force-pushed the master branch from 54bfc99 to 66bd0dd Compare May 24, 2022 05:21

This was referenced Jul 12, 2022

BadZipFile on trying to install package from private repository (Gemfury) #2388

Closed

Invalid hashes when running multiple Poetry installs simultaneously #5142

Closed

Poetry refuses to install package with correct hash #4523

Closed

0xDEC0DE approved these changes Jul 12, 2022

View reviewed changes

Antaxify pushed a commit to blackshark-ai/poetry that referenced this pull request Aug 1, 2022

Delete corrupted files in Poetry cache

69578d4

Cherry picked from PR: python-poetry#3301 Without these changes, Poetry fails if a file in the cache is corrupt and it needs to be cleaned up manually

Antaxify mentioned this pull request Aug 1, 2022

12893 fix poetry caching issues blackshark-ai/poetry#1

Merged

ombschervister closed this Feb 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hash check when installing packages from cache #3301

hash check when installing packages from cache #3301

ombschervister commented Oct 28, 2020 •

edited

Loading

earonesty Dec 20, 2021 •

edited

Loading

ombschervister Jan 3, 2022

earonesty commented Dec 20, 2021

joooeey commented Dec 28, 2021

ombschervister commented Jan 3, 2022

earonesty left a comment •

edited

Loading

earonesty left a comment

ombschervister commented May 22, 2022

Secrus commented May 22, 2022 •

edited

Loading

Secrus May 22, 2022

ombschervister May 23, 2022

Secrus May 22, 2022

ombschervister May 23, 2022

Secrus May 22, 2022

ombschervister May 23, 2022

Secrus May 22, 2022

ombschervister May 23, 2022

ombschervister May 23, 2022 •

edited

Loading

ombschervister commented May 24, 2022

earonesty commented Jul 12, 2022

0xDEC0DE left a comment

0xDEC0DE Jul 12, 2022 •

edited

Loading

wagnerluis1982 commented Feb 21, 2023

github-actions bot commented Feb 29, 2024

	logger.warning(f"cache of {link.filename} is corrupted")
	logger.warning(f"Cache of {link.filename} is corrupted. It won't be used.")

		f"Cache of {link.filename} is corrupted. The file will be"
		" reloaded."

hash check when installing packages from cache #3301

hash check when installing packages from cache #3301

Conversation

ombschervister commented Oct 28, 2020 • edited Loading

Pull Request Check List

earonesty Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

earonesty commented Dec 20, 2021

joooeey commented Dec 28, 2021

ombschervister commented Jan 3, 2022

earonesty left a comment • edited Loading

Choose a reason for hiding this comment

earonesty left a comment

Choose a reason for hiding this comment

ombschervister commented May 22, 2022

Secrus commented May 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ombschervister May 23, 2022 • edited Loading

Choose a reason for hiding this comment

ombschervister commented May 24, 2022

earonesty commented Jul 12, 2022

0xDEC0DE left a comment

Choose a reason for hiding this comment

0xDEC0DE Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

wagnerluis1982 commented Feb 21, 2023

github-actions bot commented Feb 29, 2024

ombschervister commented Oct 28, 2020 •

edited

Loading

earonesty Dec 20, 2021 •

edited

Loading

earonesty left a comment •

edited

Loading

Secrus commented May 22, 2022 •

edited

Loading

ombschervister May 23, 2022 •

edited

Loading

0xDEC0DE Jul 12, 2022 •

edited

Loading