You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by rmetzger <gi...@git.apache.org> on 2015/02/05 17:32:24 UTC
[GitHub] flink pull request: [FLINK-1166] Add qa-check.sh tool
GitHub user rmetzger opened a pull request:
https://github.com/apache/flink/pull/366
[FLINK-1166] Add qa-check.sh tool
The ./tools/qa-check.sh tool allows developers to check for common errors before opening a pull request.
The qa-checks are designed to be easily extensible by further checks.
I'm currently working on some tooling to automatically execute the tool on new pull requests (or on request).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rmetzger/flink qa-bot
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/366.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #366
----
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1166] Add qa-check.sh tool
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/366#issuecomment-73254306
I found out why the qa-check has more compile errors compared to the master: I've instructed the compiler to report them all ;)
I've addressed all comments in the pull request. I'm going to merge it soon if there are no other comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1166] Add qa-check.sh tool
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/366#issuecomment-73083755
The script builds Flink twice and therefore takes a while. I don't know if we can force users to execute it before making pull requests. However, it would be good to get an automatic report for open pull requests using this script.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1166] Add qa-check.sh tool
Posted by uce <gi...@git.apache.org>.
Github user uce commented on the pull request:
https://github.com/apache/flink/pull/366#issuecomment-73084249
The idea is to run this automatically or on request, which should be fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1166] Add qa-check.sh tool
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/366
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1166] Add qa-check.sh tool
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/366#discussion_r24178788
--- Diff: tools/qa-check.sh ---
@@ -0,0 +1,175 @@
+#!/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.
+#
+
+
+#
+# QA check your changes.
+# Possible options:
+# BRANCH set a another branch as the "check" reference
+#
+#
+# Use the tool like this "BRANCH=release-0.8 ./tools/qa-check.sh"
+#
+
+
+BRANCH=${BRANCH:-origin/master}
+
+
+
+here="`dirname \"$0\"`" # relative
+here="`( cd \"$here\" && pwd )`" # absolutized and normalized
+if [ -z "$here" ] ; then
+ # error; for some reason, the path is not accessible
+ # to the script (e.g. permissions re-evaled after suid)
+ exit 1 # fail
+fi
+flink_home="`dirname \"$here\"`"
+
+echo "flink_home=$flink_home here=$here"
+cd $here
+
+if [ ! -d "_qa_workdir" ] ; then
+ echo "_qa_workdir doesnt exist. Creating it"
+ mkdir _qa_workdir
+fi
+# attention, it overwrites
+echo "_qa_workdir" > .gitignore
+
+cd _qa_workdir
+
+if [ ! -d "flink" ] ; then
+ echo "There is no flink copy in the workdir. Cloning flink"
+ git clone https://github.com/apache/flink.git flink
+ cd flink
--- End diff --
This `cd flink` is not necessary.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1166] Add qa-check.sh tool
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/366#issuecomment-73089829
Thank you for trying it out.
Ideally, with the bot in place the number of warnings will go down over time.
I'll address the comments in the source.
I'm not sure if the number of compiler warnings is correct here.
A third option would be to
a) check out the current master
b) get the reference counts on the master
c) apply the pull request as a patch to master (checking if patching is possible (basically testing if rebase is possible))
d) if rebase was possible, get the new counts.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1166] Add qa-check.sh tool
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/366#issuecomment-73084319
Its not completely building flink, its only generating javadocs or compiling for getting the compiler warnings.
The main purpose of the script is to be executed automatically.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1166] Add qa-check.sh tool
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/366#issuecomment-73087911
I just ran the script on your pull request ;)
> Flink QA-Check results:
> :+1: The number of javadoc errors was 402 and is now 402
> :-1: The change increases the number of compiler warnings from 104 to 314
> :+1: The number of files in the lib/ folder was 142 before the change and is now 142
> Test finished.
> Overall result: :-1:. Some tests failed. Please check messages above
I guess if the user does not rebase to the latest master, then negative QA test results are likely to occur. Also, warnings could be hidden when the number of warnings have increased on the master. Wouldn't it be better to compare against the base of the user's changes?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1166] Add qa-check.sh tool
Posted by uce <gi...@git.apache.org>.
Github user uce commented on a diff in the pull request:
https://github.com/apache/flink/pull/366#discussion_r24179312
--- Diff: tools/qa-check.sh ---
@@ -0,0 +1,175 @@
+#!/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.
+#
+
+
+#
+# QA check your changes.
+# Possible options:
+# BRANCH set a another branch as the "check" reference
+#
+#
+# Use the tool like this "BRANCH=release-0.8 ./tools/qa-check.sh"
+#
+
+
+BRANCH=${BRANCH:-origin/master}
+
+
+
+here="`dirname \"$0\"`" # relative
+here="`( cd \"$here\" && pwd )`" # absolutized and normalized
+if [ -z "$here" ] ; then
+ # error; for some reason, the path is not accessible
+ # to the script (e.g. permissions re-evaled after suid)
+ exit 1 # fail
+fi
+flink_home="`dirname \"$here\"`"
+
+echo "flink_home=$flink_home here=$here"
+cd $here
+
+if [ ! -d "_qa_workdir" ] ; then
+ echo "_qa_workdir doesnt exist. Creating it"
+ mkdir _qa_workdir
+fi
+# attention, it overwrites
+echo "_qa_workdir" > .gitignore
+
+cd _qa_workdir
+
+if [ ! -d "flink" ] ; then
+ echo "There is no flink copy in the workdir. Cloning flink"
+ git clone https://github.com/apache/flink.git flink
+ cd flink
+
+fi
+cd flink
+git fetch origin
+git checkout $BRANCH
+cd $here
+# go to refrence flink directory
+
+cd _qa_workdir
+VAR_DIR=`pwd`
+cd flink
+
+# Initialize variables
+export TESTS_PASSED=true
+export MESSAGES="Flink QA-Check results:"$'\n'
+
+goToTestDirectory() {
+ cd $flink_home
+}
+
+############################ Methods ############################
+
+############ Javadocs ############
+JAVADOC_MVN_COMMAND="mvn javadoc:aggregate -Pdocs-and-source -Dmaven.javadoc.failOnError=false -Dquiet=false | grep \"WARNING\|warning\|error\" | wc -l"
+
+referenceJavadocsErrors() {
+ eval $JAVADOC_MVN_COMMAND > "$VAR_DIR/_JAVADOCS_NUM_WARNINGS"
+}
+
+
+checkJavadocsErrors() {
+ OLD_JAVADOC_ERR_CNT=`cat $VAR_DIR/_JAVADOCS_NUM_WARNINGS`
+ NEW_JAVADOC_ERR_CNT=`eval $JAVADOC_MVN_COMMAND`
+ if [ "$NEW_JAVADOC_ERR_CNT" -gt "$OLD_JAVADOC_ERR_CNT" ]; then
+ MESSAGES+=":-1: The change increases the number of javadoc errors from $OLD_JAVADOC_ERR_CNT to $NEW_JAVADOC_ERR_CNT"$'\n'
+ TESTS_PASSED=false
+ else
+ MESSAGES+=":+1: The number of javadoc errors was $OLD_JAVADOC_ERR_CNT and is now $NEW_JAVADOC_ERR_CNT"$'\n'
+ fi
+}
+
+
+############ Compiler warnings ############
+COMPILER_WARN_MVN_COMMAND="mvn clean compile -Dmaven.compiler.showWarning=true -Dmaven.compiler.showDeprecation=true | grep \"WARNING\" | wc -l"
+referenceCompilerWarnings() {
+ eval $COMPILER_WARN_MVN_COMMAND > "$VAR_DIR/_COMPILER_NUM_WARNINGS"
+}
+
+checkCompilerWarnings() {
+ OLD_COMPILER_ERR_CNT=`cat $VAR_DIR/_COMPILER_NUM_WARNINGS`
+ NEW_COMPILER_ERR_CNT=`eval $COMPILER_WARN_MVN_COMMAND`
+ if [ "$NEW_COMPILER_ERR_CNT" -gt "$OLD_COMPILER_ERR_CNT" ]; then
+ MESSAGES+=":-1: The change increases the number of compiler warnings from $OLD_COMPILER_ERR_CNT to $NEW_COMPILER_ERR_CNT"$'\n'
+ TESTS_PASSED=false
+ else
+ MESSAGES+=":+1: The number of compiler warnings was $OLD_COMPILER_ERR_CNT and is now $NEW_COMPILER_ERR_CNT"$'\n'
+ fi
+}
+
+############ Files in lib ############
+BUILD_MVN_COMMAND="mvn clean package -DskipTests"
--- End diff --
we could also skip Javadocs here with `-Dmaven.javadoc.skip=true`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-1166] Add qa-check.sh tool
Posted by uce <gi...@git.apache.org>.
Github user uce commented on the pull request:
https://github.com/apache/flink/pull/366#issuecomment-73083750
Very nice. Will try it out now :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---