[GitHub] [spark] srowen commented on a change in pull request #32505: [SPARK-35373][BUILD] Check Maven artifact checksum in build/mvn

[GitBox <gi...@apache.org>]

srowen commented on a change in pull request #32505:
URL: https://github.com/apache/spark/pull/32505#discussion_r630330154



##########
File path: build/mvn
##########
@@ -26,36 +26,67 @@ _COMPILE_JVM_OPTS="-Xmx2g -XX:ReservedCodeCacheSize=1g"
 
 # Installs any application tarball given a URL, the expected tarball name,
 # and, optionally, a checkable binary path to determine if the binary has
-# already been installed
-## Arg1 - URL
-## Arg2 - Tarball Name
-## Arg3 - Checkable Binary
+# already been installed. Arguments:
+# 1 - Mirror host
+# 2 - URL path on host
+# 3 - URL query string
+# 4 - checksum suffix
+# 5 - Tarball Name
+# 6 - Checkable Binary
 install_app() {
-  local remote_tarball="$1"
-  local local_tarball="${_DIR}/$2"
-  local binary="${_DIR}/$3"
+  local mirror_host="$1"

Review comment:
       I had to break down the arguments further to make the logic work below. As part of that I tried to refactor this method a bit, which was getting hard to understand.

##########
File path: build/mvn
##########
@@ -26,36 +26,67 @@ _COMPILE_JVM_OPTS="-Xmx2g -XX:ReservedCodeCacheSize=1g"
 
 # Installs any application tarball given a URL, the expected tarball name,
 # and, optionally, a checkable binary path to determine if the binary has
-# already been installed
-## Arg1 - URL
-## Arg2 - Tarball Name
-## Arg3 - Checkable Binary
+# already been installed. Arguments:
+# 1 - Mirror host
+# 2 - URL path on host
+# 3 - URL query string
+# 4 - checksum suffix
+# 5 - Tarball Name
+# 6 - Checkable Binary
 install_app() {
-  local remote_tarball="$1"
-  local local_tarball="${_DIR}/$2"
-  local binary="${_DIR}/$3"
+  local mirror_host="$1"
+  local url_path="$2"
+  local url_query="$3"
+  local checksum_suffix="$4"
+  local local_tarball="${_DIR}/$5"
+  local binary="${_DIR}/$6"
+  local remote_tarball="${mirror_host}/${url_path}${url_query}"
+  local local_checksum="${local_tarball}.${checksum_suffix}"
+  local remote_checksum="https://archive.apache.org/dist/${url_path}.${checksum_suffix}"
 
   local curl_opts="--silent --show-error -L"
   local wget_opts="--no-verbose"
 
-  if [ -z "$3" -o ! -f "$binary" ]; then
+  if [ ! -f "$binary" ]; then
     # check if we already have the tarball
     # check if we have curl installed
     # download application
-    [ ! -f "${local_tarball}" ] && [ $(command -v curl) ] && \
-      echo "exec: curl ${curl_opts} ${remote_tarball}" 1>&2 && \
+    if [ ! -f "${local_tarball}" -a $(command -v curl) ]; then

Review comment:
       I switched to 'regular' if syntax here for consistency, and because it's necessary to make multiple commands below work readably

##########
File path: build/mvn
##########
@@ -26,36 +26,67 @@ _COMPILE_JVM_OPTS="-Xmx2g -XX:ReservedCodeCacheSize=1g"
 
 # Installs any application tarball given a URL, the expected tarball name,
 # and, optionally, a checkable binary path to determine if the binary has
-# already been installed
-## Arg1 - URL
-## Arg2 - Tarball Name
-## Arg3 - Checkable Binary
+# already been installed. Arguments:
+# 1 - Mirror host
+# 2 - URL path on host
+# 3 - URL query string
+# 4 - checksum suffix
+# 5 - Tarball Name
+# 6 - Checkable Binary
 install_app() {
-  local remote_tarball="$1"
-  local local_tarball="${_DIR}/$2"
-  local binary="${_DIR}/$3"
+  local mirror_host="$1"
+  local url_path="$2"
+  local url_query="$3"
+  local checksum_suffix="$4"
+  local local_tarball="${_DIR}/$5"
+  local binary="${_DIR}/$6"
+  local remote_tarball="${mirror_host}/${url_path}${url_query}"
+  local local_checksum="${local_tarball}.${checksum_suffix}"
+  local remote_checksum="https://archive.apache.org/dist/${url_path}.${checksum_suffix}"
 
   local curl_opts="--silent --show-error -L"
   local wget_opts="--no-verbose"
 
-  if [ -z "$3" -o ! -f "$binary" ]; then
+  if [ ! -f "$binary" ]; then

Review comment:
       The old arg 3 was never empty; not sure what this is about

##########
File path: build/mvn
##########
@@ -101,10 +137,14 @@ install_scala() {
   local scala_version=`grep "scala.version" "${_DIR}/../pom.xml" | grep ${scala_binary_version} | head -n1 | awk -F '[<>]' '{print $3}'`
   local scala_bin="${_DIR}/scala-${scala_version}/bin/scala"
   local TYPESAFE_MIRROR=${TYPESAFE_MIRROR:-https://downloads.lightbend.com}
+  local SCALA_TARBALL="scala-${scala_version}.tgz"
 
   install_app \
-    "${TYPESAFE_MIRROR}/scala/${scala_version}/scala-${scala_version}.tgz" \
-    "scala-${scala_version}.tgz" \
+    "${TYPESAFE_MIRROR}" \
+    "scala/${scala_version}/${SCALA_TARBALL}" \
+    "" \
+    "" \

Review comment:
       As noted in the JIRA, Scala distributions on lightbend servers don't have a checksum, so we can't do anything there.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org