Skip to content
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

Feature/download cache #6287

Merged
merged 28 commits into from Jan 30, 2020
Merged

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Jan 2, 2020

Changelog: Feature: Implement a download cache, which can be shared and concurrently used among different conan user homes, selectable configuring storage.download_cache in conan.conf.
Docs: conan-io/docs#1544

This cache, can cache artifacts with (api v2) and without (api v1) revisions, and also files downloaded in the source() and build() methods with tools.download() and tools.get(). This can speed up one order of magnitude the installation of packages in clean caches.

Implements #4668

#tags: slow
#revisions: 1

@memsharded memsharded marked this pull request as ready for review Jan 14, 2020
@memsharded memsharded requested review from danimtb, jgsogo and czoido Jan 14, 2020
@memsharded memsharded added this to the 1.22 milestone Jan 14, 2020
Copy link
Member

@jgsogo jgsogo left a comment

Some comments having a look at the sources, I will like to give more feedback when running the PR locally

conans/test/functional/download_cache_test.py Outdated Show resolved Hide resolved
conans/client/rest/download_cache.py Outdated Show resolved Hide resolved
conans/client/rest/rest_client_v1.py Show resolved Hide resolved
conans/client/rest/download_cache.py Show resolved Hide resolved
if not os.path.exists(cached_path):
self._file_downloader.download(url, cached_path, auth, retry, retry_wait,
overwrite, headers)
if self._validate_checksum:
Copy link
Member

@jgsogo jgsogo Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would deindent these lines. If I already have a file in the cache, but I want Conan to validate the checksum, it is ok to check it against the cached file (and, if a failure, I would expect Conan to download it before raising, as if there were no cache)

Copy link
Member Author

@memsharded memsharded Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment. The checksum is validated when that file is stored in the cache, and if the file doesn't match the checksum, it is removed from the cache. By definition it is not possible to have an invalid checksum in the cache. Please clarify.

Copy link
Member

@jgsogo jgsogo Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a closer look at the sources now I realize that the checksum is added to the URL to build the cache key, and only if a checksum is provided the download cache is used, so this should never be an issue.

Nevertheless, I would validate it. I think it is a quick operation (compared to download the file) and it will be the only way to recover from a corrupted cache: if the file is not downloaded and the checksum fails, Conan should remove the file and enter the download method again.

conans/client/rest/rest_client_v2.py Show resolved Hide resolved
downloader.download(url, filename, retry=retry, retry_wait=retry_wait, overwrite=overwrite,
auth=auth, headers=headers)
checksum = sha256 or sha1 or md5
if config and config.download_cache and checksum:
Copy link
Member

@jgsogo jgsogo Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deserves a comment (probably in the docs): the download-cache will only be used if a checksum is provided.

czoido
czoido approved these changes Jan 28, 2020
jgsogo
jgsogo approved these changes Jan 29, 2020
Copy link
Member

@jgsogo jgsogo left a comment

🌮 Please, write docs about the feature

Copy link
Member

@jgsogo jgsogo left a comment

Ups! I see the CI has failed in one of these tests (I've relaunched it)

@memsharded memsharded merged commit a51d035 into conan-io:develop Jan 30, 2020
2 checks passed
@memsharded memsharded deleted the feature/download_cache branch Jan 30, 2020
@memsharded memsharded mentioned this pull request Feb 4, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants