adoroszlai commented on code in PR #7896:
URL: https://github.com/apache/ozone/pull/7896#discussion_r1957314332
##########
dev-support/ci/selective_ci_checks.sh:
##########
@@ -418,6 +418,25 @@ function check_needs_findbugs() {
start_end::group_end
}
+function check_needs_pmd() {
+ start_end::group_start "Check if pmd is needed"
+ local pattern_array=(
+ "^hadoop-ozone/dev-support/checks/pmd.sh"
+ "pom.xml"
+ "src/..../java"
+ )
Review Comment:
PMD check should be also triggered if `pmd-ruleset.xml` is changed.
##########
hadoop-ozone/dev-support/checks/pmd.sh:
##########
@@ -0,0 +1,46 @@
+#!/usr/bin/env bash
+# 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.
+
+#checks:basic
+
+DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
+cd "$DIR/../../.." || exit 1
+
+REPORT_DIR=${OUTPUT_DIR:-"$DIR/../../../target/pmd"}
+mkdir -p "$REPORT_DIR"
+
+REPORT_FILE="$REPORT_DIR/summary.txt"
+
+MAVEN_OPTIONS='-B -fae --no-transfer-progress'
+
+declare -i rc
+
+#shellcheck disable=SC2086
+mvn $MAVEN_OPTIONS test-compile pmd:check -Dpmd.failOnViolation=false
-Dpmd.printFailingErrors "$@" > "${REPORT_DIR}/pmd-output.log"
+
+rc=$?
+
+cat "${REPORT_DIR}/pmd-output.log"
+
+find "." -name pmd-output.log -print0 \
+ | xargs -0 sed -n -e '/^\[WARNING\] PMD Failure/p' \
+ | tee "$REPORT_FILE"
+
+## generate counter
+grep -c ':' "$REPORT_FILE" > "$REPORT_DIR/failures"
Review Comment:
`_post_process` will count it for us.
```suggestion
```
##########
hadoop-ozone/dev-support/checks/pmd.sh:
##########
@@ -0,0 +1,46 @@
+#!/usr/bin/env bash
+# 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.
+
+#checks:basic
+
+DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
+cd "$DIR/../../.." || exit 1
+
+REPORT_DIR=${OUTPUT_DIR:-"$DIR/../../../target/pmd"}
+mkdir -p "$REPORT_DIR"
+
+REPORT_FILE="$REPORT_DIR/summary.txt"
+
+MAVEN_OPTIONS='-B -fae --no-transfer-progress'
+
+declare -i rc
+
+#shellcheck disable=SC2086
+mvn $MAVEN_OPTIONS test-compile pmd:check -Dpmd.failOnViolation=false
-Dpmd.printFailingErrors "$@" > "${REPORT_DIR}/pmd-output.log"
+
+rc=$?
+
+cat "${REPORT_DIR}/pmd-output.log"
+
+find "." -name pmd-output.log -print0 \
+ | xargs -0 sed -n -e '/^\[WARNING\] PMD Failure/p' \
+ | tee "$REPORT_FILE"
Review Comment:
`find` is not needed, we have a single output file in known location.
Also, let's omit `[WARNING] ` prefix in the summary file, achieved by `-o`
option.
```suggestion
grep -o "PMD Failure.*" "${REPORT_DIR}/output.log" > "$REPORT_FILE"
```
##########
hadoop-ozone/dev-support/checks/pmd.sh:
##########
@@ -0,0 +1,46 @@
+#!/usr/bin/env bash
+# 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.
+
+#checks:basic
+
+DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
+cd "$DIR/../../.." || exit 1
+
+REPORT_DIR=${OUTPUT_DIR:-"$DIR/../../../target/pmd"}
+mkdir -p "$REPORT_DIR"
+
+REPORT_FILE="$REPORT_DIR/summary.txt"
+
+MAVEN_OPTIONS='-B -fae --no-transfer-progress'
+
+declare -i rc
+
+#shellcheck disable=SC2086
+mvn $MAVEN_OPTIONS test-compile pmd:check -Dpmd.failOnViolation=false
-Dpmd.printFailingErrors "$@" > "${REPORT_DIR}/pmd-output.log"
+
+rc=$?
+
+cat "${REPORT_DIR}/pmd-output.log"
Review Comment:
1. `output.log` is standard name for all checks.
2. Let's move options to `MAVEN_OPTIONS` to make the command shorter.
3. PMD should fail the command on errors, otherwise `rc` will always be 0,
so removing `failOnViolation=false`.
4. `>` and `cat` can be replaced with `| tee`.
```suggestion
MAVEN_OPTIONS='-B -fae --no-transfer-progress -Dpmd.printFailingErrors'
declare -i rc
#shellcheck disable=SC2086
mvn $MAVEN_OPTIONS test-compile pmd:check "$@" | tee
"${REPORT_DIR}/output.log"
rc=$?
```
##########
hadoop-ozone/dev-support/checks/pmd.sh:
##########
@@ -0,0 +1,46 @@
+#!/usr/bin/env bash
+# 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.
+
+#checks:basic
+
Review Comment:
To let `rc` reflect Maven error result despite using pipes.
```suggestion
set -u -o pipefail
```
##########
pom.xml:
##########
@@ -1754,6 +1755,17 @@
<artifactId>cyclonedx-maven-plugin</artifactId>
<version>${cyclonedx.version}</version>
</plugin>
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-pmd-plugin</artifactId>
+ <version>${pmd.version}</version>
+ <configuration>
+ <rulesets>
+ <ruleset>dev-support/pmd/pmd-ruleset.xml</ruleset>
Review Comment:
Looks like the ruleset is not found:
```
[INFO] >>> pmd:3.26.0:check (default-cli) > :pmd @ hdds-config >>>
[INFO]
[INFO] --- pmd:3.26.0:pmd (pmd) @ hdds-config ---
[WARNING] Unable to locate Source XRef to link to -- DISABLED
[WARNING] Unable to locate Source XRef to link to -- DISABLED
[INFO] PMD version: 7.7.0
[WARNING] No rules found in ruleset
hadoop-hdds/config/target/pmd/rulesets/001-pmd-ruleset.xml
[WARNING] No rules found. Maybe you misspelled a rule name?
(hadoop-hdds/config/target/pmd/rulesets/001-pmd-ruleset.xml)
[INFO] Rendering content with
org.apache.maven.skins:maven-fluido-skin:jar:2.0.0-M9 skin
[INFO]
[INFO] <<< pmd:3.26.0:check (default-cli) < :pmd @ hdds-config <<<
```
https://maven.apache.org/plugins/maven-pmd-plugin/examples/multi-module-config.html
##########
hadoop-ozone/dev-support/checks/pmd.sh:
##########
@@ -0,0 +1,46 @@
+#!/usr/bin/env bash
+# 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.
+
+#checks:basic
+
+DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
+cd "$DIR/../../.." || exit 1
+
+REPORT_DIR=${OUTPUT_DIR:-"$DIR/../../../target/pmd"}
+mkdir -p "$REPORT_DIR"
+
+REPORT_FILE="$REPORT_DIR/summary.txt"
+
+MAVEN_OPTIONS='-B -fae --no-transfer-progress'
+
+declare -i rc
+
+#shellcheck disable=SC2086
+mvn $MAVEN_OPTIONS test-compile pmd:check -Dpmd.failOnViolation=false
-Dpmd.printFailingErrors "$@" > "${REPORT_DIR}/pmd-output.log"
+
+rc=$?
+
+cat "${REPORT_DIR}/pmd-output.log"
+
+find "." -name pmd-output.log -print0 \
+ | xargs -0 sed -n -e '/^\[WARNING\] PMD Failure/p' \
+ | tee "$REPORT_FILE"
+
+## generate counter
+grep -c ':' "$REPORT_FILE" > "$REPORT_DIR/failures"
+
+ERROR_PATTERN="\[WARNING\] PMD Failure"
Review Comment:
This pattern we already searched for in the output. `ERROR_PATTERN` should
cover other problems reported by Maven (e.g. compile error).
```suggestion
ERROR_PATTERN="\[ERROR\]"
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
