weiqingy commented on code in PR #996:
URL: https://github.com/apache/flink-agents/pull/996#discussion_r3763579232
##########
tools/check-license.sh:
##########
@@ -20,41 +20,75 @@
# NOTE: This script is adapted from the Apache Spark project.
-acquire_rat_jar () {
+validate_rat_jar() {
+ local jar_cmd
+
+ if [ -n "${JAVA_HOME:-}" ] && [ -x "$JAVA_HOME/bin/jar" ]; then
+ jar_cmd="$JAVA_HOME/bin/jar"
+ elif command -v jar >/dev/null 2>&1; then
+ jar_cmd="$(command -v jar)"
+ elif command -v unzip >/dev/null 2>&1; then
+ unzip -tq "$JAR" >/dev/null 2>&1
+ return $?
+ else
+ printf "Cannot validate Apache RAT: install a JDK with 'jar' or install
'unzip'.\n" >&2
+ return 2
+ fi
-
URL="https://repo.maven.apache.org/maven2/org/apache/rat/apache-rat/${RAT_VERSION}/apache-rat-${RAT_VERSION}.jar";
+ "$jar_cmd" tf "$JAR" >/dev/null 2>&1
+}
+acquire_rat_jar() {
+
URL="https://repo.maven.apache.org/maven2/org/apache/rat/apache-rat/${RAT_VERSION}/apache-rat-${RAT_VERSION}.jar";
JAR="$rat_jar"
- # Download rat launch jar if it hasn't been downloaded yet
if [ ! -f "$JAR" ]; then
- # Download
printf "Attempting to fetch rat\n"
JAR_DL="${JAR}.part"
- if [ $(command -v curl) ]; then
- curl -L --silent "${URL}" > "$JAR_DL" && mv "$JAR_DL" "$JAR"
- elif [ $(command -v wget) ]; then
- wget --quiet ${URL} -O "$JAR_DL" && mv "$JAR_DL" "$JAR"
+ rm -f "$JAR_DL"
+ if command -v curl >/dev/null 2>&1; then
+ if ! curl --fail --show-error --location --output "$JAR_DL" "$URL"; then
+ rm -f "$JAR_DL"
+ printf "Failed to download Apache RAT from %s.\n" "$URL" >&2
+ return 1
+ fi
+ elif command -v wget >/dev/null 2>&1; then
+ if ! wget --quiet --output-document="$JAR_DL" "$URL"; then
+ rm -f "$JAR_DL"
+ printf "Failed to download Apache RAT from %s.\n" "$URL" >&2
+ return 1
+ fi
else
- printf "You do not have curl or wget installed, please install rat
manually.\n"
- exit -1
+ printf "Cannot download Apache RAT: install 'curl' or 'wget'.\n" >&2
+ return 1
+ fi
+ if ! mv "$JAR_DL" "$JAR"; then
+ rm -f "$JAR_DL"
+ printf "Failed to store the downloaded Apache RAT JAR at %s.\n" "$JAR"
>&2
+ return 1
fi
fi
- unzip -tq "$JAR" &> /dev/null
- if [ $? -ne 0 ]; then
- # We failed to download
- rm "$JAR"
- printf "Our attempt to download rat locally to ${JAR} failed. Please
install rat manually.\n"
- exit -1
+ validate_rat_jar
+ validation_status=$?
+ if [ "$validation_status" -eq 2 ]; then
Review Comment:
With the `[[ -f "$rat_jar" ]] ||` short-circuit gone from the call site at
line 101, validation runs on every invocation, so status 2 now aborts even when
a good cached JAR is already on disk. I ran both versions in one environment
(cached JAR present, neither `jar` nor `unzip` on `PATH`, `JAVA_HOME` unset):
`main` prints `RAT checks passed.` and exits 0, this branch exits 1. It takes
both tools missing, so a JRE-only image without `unzip` is the realistic case.
Worth knowing too that `ci.yml:33-35` runs this script before `Set up JDK`, so
the new requirement rests on whatever the runner image preinstalls.
Since the PR is about reporting a missing dependency, would warning and
proceeding be closer to the intent when the JAR is already on disk, keeping the
hard failure for one we just downloaded?
##########
tools/check-license.sh:
##########
@@ -20,41 +20,75 @@
# NOTE: This script is adapted from the Apache Spark project.
-acquire_rat_jar () {
+validate_rat_jar() {
+ local jar_cmd
+
+ if [ -n "${JAVA_HOME:-}" ] && [ -x "$JAVA_HOME/bin/jar" ]; then
+ jar_cmd="$JAVA_HOME/bin/jar"
+ elif command -v jar >/dev/null 2>&1; then
+ jar_cmd="$(command -v jar)"
+ elif command -v unzip >/dev/null 2>&1; then
+ unzip -tq "$JAR" >/dev/null 2>&1
+ return $?
+ else
+ printf "Cannot validate Apache RAT: install a JDK with 'jar' or install
'unzip'.\n" >&2
+ return 2
+ fi
-
URL="https://repo.maven.apache.org/maven2/org/apache/rat/apache-rat/${RAT_VERSION}/apache-rat-${RAT_VERSION}.jar";
+ "$jar_cmd" tf "$JAR" >/dev/null 2>&1
+}
+acquire_rat_jar() {
+
URL="https://repo.maven.apache.org/maven2/org/apache/rat/apache-rat/${RAT_VERSION}/apache-rat-${RAT_VERSION}.jar";
JAR="$rat_jar"
- # Download rat launch jar if it hasn't been downloaded yet
if [ ! -f "$JAR" ]; then
- # Download
printf "Attempting to fetch rat\n"
JAR_DL="${JAR}.part"
- if [ $(command -v curl) ]; then
- curl -L --silent "${URL}" > "$JAR_DL" && mv "$JAR_DL" "$JAR"
- elif [ $(command -v wget) ]; then
- wget --quiet ${URL} -O "$JAR_DL" && mv "$JAR_DL" "$JAR"
+ rm -f "$JAR_DL"
+ if command -v curl >/dev/null 2>&1; then
+ if ! curl --fail --show-error --location --output "$JAR_DL" "$URL"; then
+ rm -f "$JAR_DL"
+ printf "Failed to download Apache RAT from %s.\n" "$URL" >&2
+ return 1
+ fi
+ elif command -v wget >/dev/null 2>&1; then
+ if ! wget --quiet --output-document="$JAR_DL" "$URL"; then
Review Comment:
`wget -q` turns off wget's output entirely, error text included, so this
branch still surfaces only the generic `Failed to download Apache RAT from %s.`
line. That is the diagnostic the curl branch is being changed to improve.
`--no-verbose` is the middle setting: it drops the progress output while, per
the wget manual, "error messages and basic information still get printed".
Is the quiet output deliberate on this branch, or would `--no-verbose` fit
the goal better?
##########
tools/test/unit/check_license.bats:
##########
@@ -0,0 +1,143 @@
+#!/usr/bin/env bats
+
+################################################################################
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+################################################################################
+
+command() {
Review Comment:
`tools/test/helpers/shim.bash` already provides all five of these helpers
plus a `shim_setup`, and `shim_bin` / `shim_bin_script` here are byte-identical
to `shim.bash:37-45` and `50-58`. `tools/test/unit/shim_self_test.bats:5-10`
shows the load pattern for a unit test. I tried the swap locally: `load
'../helpers/shim'` plus a `shim_setup` call in `setup()`, with the five local
copies deleted, keeps all 5 tests green.
Would reusing the shared helper work for you here?
##########
tools/check-license.sh:
##########
@@ -20,41 +20,75 @@
# NOTE: This script is adapted from the Apache Spark project.
-acquire_rat_jar () {
+validate_rat_jar() {
+ local jar_cmd
+
+ if [ -n "${JAVA_HOME:-}" ] && [ -x "$JAVA_HOME/bin/jar" ]; then
+ jar_cmd="$JAVA_HOME/bin/jar"
+ elif command -v jar >/dev/null 2>&1; then
+ jar_cmd="$(command -v jar)"
+ elif command -v unzip >/dev/null 2>&1; then
+ unzip -tq "$JAR" >/dev/null 2>&1
+ return $?
+ else
+ printf "Cannot validate Apache RAT: install a JDK with 'jar' or install
'unzip'.\n" >&2
+ return 2
+ fi
-
URL="https://repo.maven.apache.org/maven2/org/apache/rat/apache-rat/${RAT_VERSION}/apache-rat-${RAT_VERSION}.jar";
+ "$jar_cmd" tf "$JAR" >/dev/null 2>&1
+}
+acquire_rat_jar() {
+
URL="https://repo.maven.apache.org/maven2/org/apache/rat/apache-rat/${RAT_VERSION}/apache-rat-${RAT_VERSION}.jar";
JAR="$rat_jar"
- # Download rat launch jar if it hasn't been downloaded yet
if [ ! -f "$JAR" ]; then
- # Download
printf "Attempting to fetch rat\n"
JAR_DL="${JAR}.part"
- if [ $(command -v curl) ]; then
- curl -L --silent "${URL}" > "$JAR_DL" && mv "$JAR_DL" "$JAR"
- elif [ $(command -v wget) ]; then
- wget --quiet ${URL} -O "$JAR_DL" && mv "$JAR_DL" "$JAR"
+ rm -f "$JAR_DL"
+ if command -v curl >/dev/null 2>&1; then
+ if ! curl --fail --show-error --location --output "$JAR_DL" "$URL"; then
Review Comment:
nit: `--show-error` only takes effect together with `--silent`. `man curl`
describes it as "When used with -s, --silent, it makes curl show an error
message if it fails." The failure text comes through either way, so nothing is
lost. As written, though, the flag is inert and curl writes its progress meter
to stderr on every download, tty or not.
What do you think about putting `--silent` back next to it, so the pair does
what it says?
##########
tools/test/unit/check_license.bats:
##########
@@ -0,0 +1,143 @@
+#!/usr/bin/env bats
+
+################################################################################
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+################################################################################
+
+command() {
+ if [[ "$1" == "-v" ]]; then
+ local missing
+ for missing in "${MISSING_COMMANDS[@]:-}"; do
+ if [[ "$2" == "$missing" ]]; then
+ return 1
+ fi
+ done
+ fi
+ builtin command "$@"
+}
+
+shim_bin() {
+ local name="$1" exit_code="${2:-0}"
+ cat >"$SHIM_DIR/$name" <<EOF
+#!/usr/bin/env bash
+( IFS=\$'\t'; printf '%s\n' "\$*" ) >> "$SHIM_CALLS/$name.log"
+exit $exit_code
+EOF
+ chmod +x "$SHIM_DIR/$name"
+}
+
+shim_bin_script() {
+ local name="$1" body="$2"
+ cat >"$SHIM_DIR/$name" <<EOF
+#!/usr/bin/env bash
+( IFS=\$'\t'; printf '%s\n' "\$*" ) >> "$SHIM_CALLS/$name.log"
+$body
+EOF
+ chmod +x "$SHIM_DIR/$name"
+}
+
+shim_bin_missing() {
+ MISSING_COMMANDS+=("$1")
+}
+
+shim_call_count() {
+ local log="$SHIM_CALLS/$1.log"
+ if [[ ! -f "$log" ]]; then
+ echo 0
+ return
+ fi
+ wc -l <"$log" | tr -d ' '
+}
+
+setup() {
+ CHECK_LICENSE_SOURCE_ONLY=1
+ source "${BATS_TEST_DIRNAME}/../../check-license.sh"
+ unset CHECK_LICENSE_SOURCE_ONLY
+
+ SHIM_DIR="$BATS_TEST_TMPDIR/bin"
+ SHIM_CALLS="$BATS_TEST_TMPDIR/calls"
+ MISSING_COMMANDS=()
+ mkdir -p "$SHIM_DIR" "$SHIM_CALLS"
+ export PATH="$SHIM_DIR:$PATH"
+
+ RAT_VERSION="test"
+ rat_jar="$BATS_TEST_TMPDIR/apache-rat-test.jar"
+ JAR="$rat_jar"
+ JAVA_HOME="$BATS_TEST_TMPDIR/missing-java-home"
+}
+
+@test "reports missing validation tools without deleting an existing JAR" {
+ touch "$rat_jar"
+ shim_bin_missing jar
+ shim_bin_missing unzip
+
+ run acquire_rat_jar
+
+ [ "$status" -ne 0 ]
+ [[ "$output" == *"install a JDK with 'jar' or install 'unzip'"* ]]
+ [ -f "$rat_jar" ]
+}
+
+@test "rejects and removes an invalid existing JAR" {
+ touch "$rat_jar"
+ shim_bin_missing jar
+ shim_bin unzip 1
+
+ run acquire_rat_jar
+
+ [ "$status" -ne 0 ]
+ [[ "$output" == *"is invalid"* ]]
+ [ ! -f "$rat_jar" ]
+}
+
+@test "does not download an existing valid JAR" {
+ touch "$rat_jar"
+ shim_bin_missing jar
+ shim_bin unzip
+ shim_bin curl
+
+ acquire_rat_jar
+
+ [ "$(shim_call_count curl)" = "0" ]
+ [ "$(shim_call_count unzip)" = "1" ]
+}
+
+@test "reports a download failure and removes the partial file" {
+ shim_bin curl 22
Review Comment:
`shim_bin curl 22` exits without ever creating `${rat_jar}.part`, so the `[
! -e "${rat_jar}.part" ]` assertion at line 126 holds no matter what the script
does. I confirmed by deleting `rm -f "$JAR_DL"` from the curl-failure branch:
all 5 tests still pass. Could the stub create the file first, so the assertion
has something to catch? Something like this, if it helps:
```bash
shim_bin_script curl 'prev=""; for arg in "$@"; do [[ "$prev" == "--output"
]] && : > "$arg"; prev="$arg"; done; exit 22'
```
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
