llama-run: Fix model download on Windows #15988
Conversation
* fix SSL error (SSL peer certificate or SSH remote key was not OK) * fix program crash on std::filesystem::rename
|
Thanks for the fix I am going to split out the curl stuff in common/arg.cpp to a common/curl.cpp class We should try and consolidate the llama-run specific stuff more in common/arg.cpp to help avoid things like this. |
tools/run/run.cpp
Outdated
| if (!output_file.empty()) { | ||
| // Explicitly close file in order to release lock | ||
| out.close(); | ||
| std::filesystem::rename(output_file_partial, output_file); |
There was a problem hiding this comment.
I would prefer if we put 412-447 into it's own function and relied on RAII to close the file. We would call this function in init and then:
if (!output_file.empty()) {
std::filesystem::rename(output_file_partial, output_file);
}
There was a problem hiding this comment.
Hi, thanks! Yeah, I have created a separated method to utilize RAII.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes two critical issues affecting model downloads on Windows in the llama-run tool: SSL certificate verification errors and crashes during file operations. The changes reorganize the download logic to use temporary files properly and add Windows-specific SSL configuration.
- Restructures the HttpClient class to handle temporary file operations in the public init method
- Adds Windows-specific SSL configuration to use native certificate authority
- Moves file renaming from download completion to the main download flow
tools/run/run.cpp
Outdated
| } | ||
|
|
||
| if (!output_file.empty()) { | ||
| std::filesystem::rename(output_file_partial, output_file); |
There was a problem hiding this comment.
The std::filesystem::rename call lacks error handling. On Windows, this operation can fail due to file permissions, antivirus software, or if the target file is still open. Consider wrapping this in a try-catch block and providing appropriate error handling or retry logic.
| std::filesystem::rename(output_file_partial, output_file); | |
| try { | |
| std::filesystem::rename(output_file_partial, output_file); | |
| } catch (const std::filesystem::filesystem_error& e) { | |
| printe("Error: Failed to rename '%s' to '%s': %s\n", output_file_partial.c_str(), output_file.c_str(), e.what()); | |
| return -1; | |
| } |
Copilot uses AI. Check for mistakes.
tools/run/run.cpp
Outdated
| CURL * curl = nullptr; | ||
| struct curl_slist * chunk = nullptr; | ||
|
|
||
|
|
There was a problem hiding this comment.
Trailing whitespace on line 476 should be removed to maintain code cleanliness.
Copilot uses AI. Check for mistakes.
|
Build failures unrelated |
llama-run: Fix model download on Windows