Re: [cmake-developers] [Patch] ExternalProject: fix retry logic if error occurs

[Brad King]

On 05/19/2016 09:49 AM, Ruslan Baratov wrote:
> It's a little bit tricky to split refactor/new functionality exact but 
> I've created several patches with step-by-step changes. Hope this helps 
> in review.

Nice, thanks.  I've applied the changes and merged to 'next' for testing:

 ExternalProject: Use uppercase placeholders for script generation
 https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=d610407c

 ExternalProject: Remove unused 'retries' argument from verify script
 https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=e5409d1e

 ExternalProject: Remove unused verify script logic
 https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=33218f6a

 ExternalProject: Avoid repeating download verification
 https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=ebcc7027

 ExternalProject: Re-implement download verification as a dedicated script
 https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=e7d5e4b4

 ExternalProject: Re-implement download logic as a dedicated script
 https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=89113e12

-Brad

-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers

Re: [cmake-developers] [Patch] ExternalProject: fix retry logic if error occurs

[Ruslan Baratov via cmake-developers]

It's a little bit tricky to split refactor/new functionality exact but 
I've created several patches with step-by-step changes. Hope this helps 
in review.


Ruslo

On 18-May-16 22:50, Brad King wrote:

On 05/18/2016 03:30 PM, Ruslan Baratov via cmake-developers wrote:

I've attached patch with applying retry logic in cases when status code
is not zero.

Thanks.  Please split the patch to perform the refactoring into
the ExternalProject-$step.cmake.in files first and then make the
logic changes as a second patch.  Also please add an explanation
of the logic change to the second commit message similar to what
you wrote in the original email.

-Brad



>From 5b4149ddc436f83bff4b7636884f86e9200b9fe7 Mon Sep 17 00:00:00 2001
From: Ruslan Baratov 
Date: Thu, 19 May 2016 14:38:06 +0300
Subject: [PATCH 1/6] ExternalProject refactoring: uppercase variables

Use uppercase variables for future 'configure_file' command.
---
 Modules/ExternalProject.cmake | 54 +--
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/Modules/ExternalProject.cmake b/Modules/ExternalProject.cmake
index 7dad6e5..a7fc1f9 100644
--- a/Modules/ExternalProject.cmake
+++ b/Modules/ExternalProject.cmake
@@ -847,25 +847,25 @@ endif()
 
 endfunction(_ep_write_gitupdate_script)
 
-function(_ep_write_downloadfile_script script_filename remote local timeout no_progress hash tls_verify tls_cainfo)
+function(_ep_write_downloadfile_script script_filename REMOTE LOCAL timeout no_progress hash tls_verify tls_cainfo)
   if(timeout)
-set(timeout_args TIMEOUT ${timeout})
-set(timeout_msg "${timeout} seconds")
+set(TIMEOUT_ARGS TIMEOUT ${timeout})
+set(TIMEOUT_MSG "${timeout} seconds")
   else()
-set(timeout_args "# no TIMEOUT")
-set(timeout_msg "none")
+set(TIMEOUT_ARGS "# no TIMEOUT")
+set(TIMEOUT_MSG "none")
   endif()
 
   if(no_progress)
-set(show_progress "")
+set(SHOW_PROGRESS "")
   else()
-set(show_progress "SHOW_PROGRESS")
+set(SHOW_PROGRESS "SHOW_PROGRESS")
   endif()
 
   if("${hash}" MATCHES "${_ep_hash_regex}")
 string(CONCAT hash_check
-  "if(EXISTS \"${local}\")\n"
-  "  file(\"${CMAKE_MATCH_1}\" \"${local}\" hash_value)\n"
+  "if(EXISTS \"${LOCAL}\")\n"
+  "  file(\"${CMAKE_MATCH_1}\" \"${LOCAL}\" hash_value)\n"
   "  if(\"x\${hash_value}\" STREQUAL \"x${CMAKE_MATCH_2}\")\n"
   "return()\n"
   "  endif()\n"
@@ -875,15 +875,15 @@ function(_ep_write_downloadfile_script script_filename remote local timeout no_p
 set(hash_check "")
   endif()
 
-  set(tls_verify_code "")
-  set(tls_cainfo_code "")
+  set(TLS_VERIFY_CODE "")
+  set(TLS_CAINFO_CODE "")
 
   # check for curl globals in the project
   if(DEFINED CMAKE_TLS_VERIFY)
-set(tls_verify_code "set(CMAKE_TLS_VERIFY ${CMAKE_TLS_VERIFY})")
+set(TLS_VERIFY_CODE "set(CMAKE_TLS_VERIFY ${CMAKE_TLS_VERIFY})")
   endif()
   if(DEFINED CMAKE_TLS_CAINFO)
-set(tls_cainfo_code "set(CMAKE_TLS_CAINFO \"${CMAKE_TLS_CAINFO}\")")
+set(TLS_CAINFO_CODE "set(CMAKE_TLS_CAINFO \"${CMAKE_TLS_CAINFO}\")")
   endif()
 
   # now check for curl locals so that the local values
@@ -892,28 +892,28 @@ function(_ep_write_downloadfile_script script_filename remote local timeout no_p
   # check for tls_verify argument
   string(LENGTH "${tls_verify}" tls_verify_len)
   if(tls_verify_len GREATER 0)
-set(tls_verify_code "set(CMAKE_TLS_VERIFY ${tls_verify})")
+set(TLS_VERIFY_CODE "set(CMAKE_TLS_VERIFY ${tls_verify})")
   endif()
   # check for tls_cainfo argument
   string(LENGTH "${tls_cainfo}" tls_cainfo_len)
   if(tls_cainfo_len GREATER 0)
-set(tls_cainfo_code "set(CMAKE_TLS_CAINFO \"${tls_cainfo}\")")
+set(TLS_CAINFO_CODE "set(CMAKE_TLS_CAINFO \"${tls_cainfo}\")")
   endif()
 
   file(WRITE ${script_filename}
 "${hash_check}message(STATUS \"downloading...
- src='${remote}'
- dst='${local}'
- timeout='${timeout_msg}'\")
+ src='${REMOTE}'
+ dst='${LOCAL}'
+ timeout='${TIMEOUT_MSG}'\")
 
-${tls_verify_code}
-${tls_cainfo_code}
+${TLS_VERIFY_CODE}
+${TLS_CAINFO_CODE}
 
 file(DOWNLOAD
-  \"${remote}\"
-  \"${local}\"
-  ${show_progress}
-  ${timeout_args}
+  \"${REMOTE}\"
+  \"${LOCAL}\"
+  ${SHOW_PROGRESS}
+  ${TIMEOUT_ARGS}
   STATUS status
   LOG log)
 
@@ -921,7 +921,7 @@ list(GET status 0 status_code)
 list(GET status 1 status_string)
 
 if(NOT status_code EQUAL 0)
-  message(FATAL_ERROR \"error: downloading '${remote}' failed
+  message(FATAL_ERROR \"error: downloading '${REMOTE}' failed
   status_code: \${status_code}
   status_string: \${status_string}
   log: \${log}
@@ -935,7 +935,7 @@ message(STATUS \"downloading... done\")
 endfunction()
 
 
-function(_ep_write_verifyfile_script script_filename local hash retries download_script)
+function(_ep_write_verifyfile_script script_filename LOCAL hash retries download_script)
   if("${hash}" MATCHES "${_ep_hash_regex}")
 set(algo "${CMAKE_MATCH_1}")

Re: [cmake-developers] [Patch] ExternalProject: fix retry logic if error occurs

[Brad King]

On 05/18/2016 03:30 PM, Ruslan Baratov via cmake-developers wrote:
> I've attached patch with applying retry logic in cases when status code 
> is not zero.

Thanks.  Please split the patch to perform the refactoring into
the ExternalProject-$step.cmake.in files first and then make the
logic changes as a second patch.  Also please add an explanation
of the logic change to the second commit message similar to what
you wrote in the original email.

-Brad

-- 

Powered by www.kitware.com

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Kitware offers various services to support the CMake community. For more 
information on each offering, please visit:

CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers

[cmake-developers] [Patch] ExternalProject: fix retry logic if error occurs

[Ruslan Baratov via cmake-developers]

Hi,

For now download retry attempt occurs in ExternalProject only if hash of 
archive mismatch. If error happens then script stop with FATAL_ERROR 
(example: 
https://ci.appveyor.com/project/ingenue/hunter/build/1.0.448/job/22kdl633f9w503ut). 
I've attached patch with applying retry logic in cases when status code 
is not zero.


Quite the same happening in 
https://github.com/Kitware/CMake/commit/30a94eecdb5c580d83a224848b78d186643e8105 
fix. Since `if(NOT status_code EQUAL 0)` trigger FATAL_ERROR, retry 
logic in `_ep_write_verifyfile_script` function is not reached. I've set 
default retry number to 5 with pauses 0, 5, 5, 15, 60 seconds. I left 
some space for future improvements if needed (90, 300, 1200=20min) 
probably can be controlled by some variable that will be available for 
users.


Ruslo
>From ce18b27a792ebbb79822b610e6d8e5077007d6cb Mon Sep 17 00:00:00 2001
From: Ruslan Baratov 
Date: Wed, 18 May 2016 22:06:35 +0300
Subject: [PATCH] ExternalProject: fix retry logic

Retry download if error occurs. Refactor ExternalProject module.
---
 Modules/ExternalProject-download.cmake.in | 161 ++
 Modules/ExternalProject-verify.cmake.in   |  48 +
 Modules/ExternalProject.cmake | 155 ++--
 3 files changed, 264 insertions(+), 100 deletions(-)
 create mode 100644 Modules/ExternalProject-download.cmake.in
 create mode 100644 Modules/ExternalProject-verify.cmake.in

diff --git a/Modules/ExternalProject-download.cmake.in b/Modules/ExternalProject-download.cmake.in
new file mode 100644
index 000..5b73cd8
--- /dev/null
+++ b/Modules/ExternalProject-download.cmake.in
@@ -0,0 +1,161 @@
+#=
+# Copyright 2008-2013 Kitware, Inc.
+# Copyright 2016 Ruslan Baratov
+#
+# Distributed under the OSI-approved BSD License (the "License");
+# see accompanying file Copyright.txt for details.
+#
+# This software is distributed WITHOUT ANY WARRANTY; without even the
+# implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+# See the License for more information.
+#=
+# (To distribute this file outside of CMake, substitute the full
+#  License text for the above reference.)
+
+cmake_minimum_required(VERSION 3.5)
+
+function(check_file_hash has_hash hash_is_good)
+  if("${has_hash}" STREQUAL "")
+message(FATAL_ERROR "has_hash Can't be empty")
+  endif()
+
+  if("${hash_is_good}" STREQUAL "")
+message(FATAL_ERROR "hash_is_good Can't be empty")
+  endif()
+
+  if("@ALGO@" STREQUAL "")
+# No check
+set("${has_hash}" FALSE PARENT_SCOPE)
+set("${hash_is_good}" FALSE PARENT_SCOPE)
+return()
+  endif()
+
+  set("${has_hash}" TRUE PARENT_SCOPE)
+
+  message(STATUS "verifying file...
+   file='@LOCAL@'")
+
+  file("@ALGO@" "@LOCAL@" actual_value)
+
+  if(NOT "${actual_value}" STREQUAL "@EXPECT_VALUE@")
+set("${hash_is_good}" FALSE PARENT_SCOPE)
+message(STATUS "@ALGO@ hash of
+@LOCAL@
+  does not match expected value
+expected: '@EXPECT_VALUE@'
+  actual: '${actual_value}'")
+  else()
+set("${hash_is_good}" TRUE PARENT_SCOPE)
+  endif()
+endfunction()
+
+function(sleep_before_download attempt)
+  if(attempt EQUAL 0)
+return()
+  endif()
+
+  if(attempt EQUAL 1)
+message(STATUS "Retrying...")
+return()
+  endif()
+
+  set(sleep_seconds 0)
+
+  if(attempt EQUAL 2)
+set(sleep_seconds 5)
+  elseif(attempt EQUAL 3)
+set(sleep_seconds 5)
+  elseif(attempt EQUAL 4)
+set(sleep_seconds 15)
+  elseif(attempt EQUAL 5)
+set(sleep_seconds 60)
+  elseif(attempt EQUAL 6)
+set(sleep_seconds 90)
+  elseif(attempt EQUAL 7)
+set(sleep_seconds 300)
+  else()
+set(sleep_seconds 1200)
+  endif()
+
+  message(STATUS "Retry after ${sleep_seconds} seconds (attempt #${attempt}) ...")
+
+  execute_process(COMMAND "${CMAKE_COMMAND}" -E sleep "${sleep_seconds}")
+endfunction()
+
+if("@LOCAL@" STREQUAL "")
+  message(FATAL_ERROR "LOCAL can't be empty")
+endif()
+
+if("@REMOTE@" STREQUAL "")
+  message(FATAL_ERROR "REMOTE can't be empty")
+endif()
+
+if(EXISTS "@LOCAL@")
+  check_file_hash(has_hash hash_is_good)
+  if(has_hash)
+if(hash_is_good)
+  message(STATUS "File already exists and hash match (skip download):
+  file='@LOCAL@'
+  @ALGO@='@EXPECT_VALUE@'"
+  )
+  return()
+else()
+  message(STATUS "File already exists but hash mismatch. Removing...")
+  file(REMOVE "@LOCAL@")
+endif()
+  else()
+message(STATUS "File already exists but no hash specified (use URL_HASH):
+  file='@LOCAL@'
+Old file will be removed and new file downloaded from URL."
+)
+file(REMOVE "@LOCAL@")
+  endif()
+endif()
+
+set(retry_number 5)
+
+foreach(i RANGE ${retry_number})
+  sleep_before_download(${i})
+
+  message(STATUS "downloading...
+   src='@REMOTE@'
+   dst='@LOCAL@'
+