cmake: Fix dependency download logic#13078
cmake: Fix dependency download logic#13078zhangboyang wants to merge 1 commit intoobsproject:masterfrom
Conversation
PatTheMav
left a comment
PatTheMav
left a comment
There was a problem hiding this comment.
Why not simply move the file removal above the FATAL_ERROR message, which ensures that CMake will delete an incomplete or mismatching file?
That way the original assumption (file exists, so prior setup succeeded) is still correct and hashing of large archives or other file operations are skipped.
The main purpose of why this was implemented this way was to make re-configurations of the build system (which happens regularly and often for many maintainers) to be as fast as possible.
Running a hash check for the file on every configuration introduces the kind of overhead the implementation tries to avoid.
zhangboyang
force-pushed
the
fix_dependency_download_logic
branch
from
b5d870d to
efec774
Compare
If the download progress is interrupted by Ctrl-C, the "file(REMOVE ..." will not be executed even if it is before the "message(FATAL_ERROR". So the broken file is left there and will be used next time. This patch solves the Ctrl-C problem. Although it is generally not safe to use Ctrl-C to interrupt cmake, this patch can make life easier if internet is slow or unstable.
I refactored my patch, it now use the rename approach. Please have a look :) |
PatTheMav
left a comment
PatTheMav
left a comment
There was a problem hiding this comment.
Using a temporary location for the actual download would've indeed been my suggested alternative to keep the intention of the original code to treat the existence of the expected file as the "sentinel" value to skip any extra evaluations, so this seems like the better solution.
Previously, if download failure occurred or interrupted by Ctrl-C, the broken file is not removed. This is true for the first case because "message(FATAL_ERROR ..." stops cmake and "file(REMOVE ..." is never reached. After that, if cmake is run second time, SHA256 verification is skipped and the broken file will be used. This patch fixes this by using a temporary intermediate file. The file data is downloaded into a temporary file first, then the temporary file is atomically renamed to destination. Error checking of download status is also improved. The if clause now checks error code against zero more strictly, according to cmake's documentation.
zhangboyang
force-pushed
the
fix_dependency_download_logic
branch
from
efec774 to
58377e2
Compare
WizardCM
added
kind/enhancement
5 participants
Description
Previously, if download failure occurred or interrupted by Ctrl-C, the broken file is not removed. This is true for the first case because "message(FATAL_ERROR ..." stops cmake and "file(REMOVE ..." is never reached. After that, if cmake is run second time, SHA256 verification is skipped and the broken file will be used.
This patch fixes this by always checking SHA256 of the downloaded file, using a if clause. Although "file(DOWNLOAD ... EXPECTED_HASH ..." already provides same functionality, using it makes status printing harder.
Error checking of download status is also improved. The if clause now checks equality, according to cmake's documentation.
The redundant "file(REMOVE ..." is also removed.
Motivation and Context
Before applying this patch: first run, download failure
Second run, broken downloaded file is used and boom
How Has This Been Tested?
Win10 x64, Visual C++ 2022 Community, Git-2.52.0, CMake 4.2.3
Manually run
cmakefew times, it now correctly redownload broken dependency files.Types of changes
Tweak (non-breaking change to improve existing functionality)
Checklist: