file(DOWNLOAD): delete the local file on download failure
Hi,
This is a follow up of the issue #24093 (closed)
I would like to propose a breaking change, that I can develop once we agree on what should be done.
The problem
Currently, if file(DOWNLOAD ... EXPECTED_HASH ...)
fails to download the given file :
- the error code from libcurl is ignored
- an empty file is always created (it is created before starting the download to ensure the file can be created)
- the hash check is performed on the empty file
- which caused a misguiding
file DOWNLOAD HASH mismatch
error
This behavior makes most people believe it's an issue with the hash, while the real error is just the file could not be downloaded. I would like to improve that.
Option 1 (breaking change): always delete the local file on failure / local file will exist only on success
Always delete the file in case libcurl returns an error (similar to curl --remove-on-error
):
-
file(DOWNLOAD)
: the function will succeed as it does currently, even on download failure- success of the operation still has to be tested with
STATUS
(as today) - the change is: in case of download failure you just won't get any local file created
- in other words: if you get the local file, you can consider the download succeeded (you can't do that currently)
- success of the operation still has to be tested with
-
file(DOWNLOAD ... EXPECTED_HASH ...)
: the function will fail as it currently does on download failure- but the error message will be different
- rather than complaining about
file DOWNLOAD HASH mismatch
- the hash check operation won't be performed, and a new error message will be displayed (for example:
file DOWNLOAD failure: SSL connect error
)
To recap: the behavior of the cmake function will NOT change (success/failure), only the existence of the output file will (will exist only on success).
I like this option because I think it will make cmake scripts more robust by default. Indeed, if it breaks existing scripts, I tend to think it's a good thing because it will make people realize the local file they were using was actually garbage (they did not check the download STATUS
before using it).
file DOWNLOAD HASH mismatch
error message
Option 2: no breaking change, just improve the -
file(DOWNLOAD)
: no change at all -
file(DOWNLOAD ... EXPECTED_HASH ...)
: if download failed, a specific error message will be displayed rather than failing to check the hash- the hash comparison won't be performed and the function will fail immediately with a new error message (for example:
file DOWNLOAD failure: SSL connect error
)
- the hash comparison won't be performed and the function will fail immediately with a new error message (for example:
What do you think?
I wonder what the impact would be on existing unit tests as well