Add .download marker file for cache validation#688
Conversation
Add a marker .download file to validate the contents in cache directories. Previously only the existence of the directory was used, so if the download was aborted the cache directory had to be deleted manually if this occurred (with a likely cryptic error message). If the .download check file does not exist, the directory will be deleted and downloaded again. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates CPM's source-cache path to use a .download marker as the signal that a cached dependency was fully downloaded, instead of treating directory existence alone as sufficient. It is the first slice of the broader cache-validation work from #559.
Changes:
- Acquires the cache lock before validating a cached download directory.
- Treats cache directories without a
.downloadmarker as invalid and removes them before re-fetching. - Writes a marker file after a package has been freshly populated into the cache.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "Cache for ${CPM_ARGS_NAME} is missing .download, downloading. (${download_directory}.download)" | ||
| ) | ||
| file(REMOVE_RECURSE ${download_directory}) |
There was a problem hiding this comment.
This could also mitigate the migration issue mentioned below. In case it breaks the user can still decide to manually delete the cache directory.
| cpm_fetch_package("${CPM_ARGS_NAME}" ${DOWNLOAD_ONLY} populated ${CPM_ARGS_UNPARSED_ARGUMENTS}) | ||
| if(CPM_SOURCE_CACHE AND download_directory) | ||
| if(${populated}) | ||
| file(WRITE ${download_directory}.download "") |
| if(EXISTS ${download_directory} AND NOT EXISTS ${download_directory}.download) | ||
| message( | ||
| WARNING | ||
| "Cache for ${CPM_ARGS_NAME} is missing .download, downloading. (${download_directory}.download)" | ||
| ) | ||
| file(REMOVE_RECURSE ${download_directory}) | ||
| endif() | ||
|
|
||
| if(EXISTS ${download_directory}) | ||
| if(CPM_SOURCE_CACHE) | ||
| file(LOCK ${download_directory}/../cmake.lock RELEASE) | ||
| endif() | ||
| # Directory content is considered OK | ||
| file(LOCK ${download_directory}/../cmake.lock RELEASE) | ||
|
|
||
| cpm_store_fetch_properties( |
There was a problem hiding this comment.
I agree, a test case would be great!
|
Hey @jdumas thanks for splitting the PR into smaller parts! This one does look very clean and important to fix download issues. I've also asked Copilot for feedback as it quite good in spotting details I'd miss. One additional issue I see here is that project updating CPM.cmake to a version containing this fix would unexpectedly delete and re-download all dependencies of the project. As this might come unexpected, how do you feel about either hiding the removal behind a flag that must be explicitly enabled in the environment? Alternatively we could add a new flag in the Also I apologise for not having a lot of time to spend on this project at the moment. |
Personally I would rather being forced to redownload everything than having to keep an option around. Adding a new flag to the hash calculation sounds like a good compromise (but it also basically forces a redownload, so not sure it's worth it). I'll come back to this later to address Copilot's comments. Thanks for the review! |
True, it would still trigger a re-download but it won't unexpectedly delete existing dependencies, which could be a problem if the original source is no longer available or if the user doesn't have an internet connection. |
|
Fair point. I'll implement that when I get a chance then. |
Trying to separate #559 in two separate PRs. The first one is pretty simple and simply adds a .download marker to check that a folder is downloaded.
Disclaimer: I had Claude separate #559 in two separate commits. This PR is only for the first part (the .download marker).