This is an automated email from the ASF dual-hosted git repository. vladimirsitnikov pushed a commit to branch drop_HydromaticFileSetCheck in repository https://gitbox.apache.org/repos/asf/calcite.git
commit 9895227aedc89bbf7e331d132731aed70a2b84ad Author: Vladimir Sitnikov <[email protected]> AuthorDate: Tue Dec 3 21:42:59 2019 +0300 [CALCITE-3559] Drop HydromaticFileSetCheck, upgrade Checkstyle 7.8.2 -> 8.27 --- build.gradle.kts | 39 ++++---- buildSrc/settings.gradle.kts | 1 + buildSrc/subprojects/buildext/buildext.gradle.kts | 26 ++++++ .../calcite/buildtools/buildext/BuildExtPlugin.kt | 27 ++++++ .../buildtools/buildext/dsl/ParenthesisBalancer.kt | 101 +++++++++++++++++++++ gradle.properties | 4 +- src/main/config/checkstyle/checker.xml | 74 ++++++++------- src/main/config/checkstyle/suppressions.xml | 22 ----- 8 files changed, 216 insertions(+), 78 deletions(-) diff --git a/build.gradle.kts b/build.gradle.kts index 58003f9..ccbc819 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -25,12 +25,14 @@ import com.github.vlsi.gradle.properties.dsl.props import com.github.vlsi.gradle.release.RepositoryType import de.thetaphi.forbiddenapis.gradle.CheckForbiddenApis import de.thetaphi.forbiddenapis.gradle.CheckForbiddenApisExtension +import org.apache.calcite.buildtools.buildext.dsl.ParenthesisBalancer import org.gradle.api.tasks.testing.logging.TestExceptionFormat plugins { publishing // Verification checkstyle + calcite.buildext id("com.diffplug.gradle.spotless") id("org.nosphere.apache.rat") id("com.github.spotbugs") @@ -184,11 +186,11 @@ val buildSqllineClasspath by tasks.registering(Jar::class) { } } -val semaphore = `java.util.concurrent`.Semaphore(1) - val javaccGeneratedPatterns = arrayOf( "org/apache/calcite/jdbc/CalciteDriverVersion.java", - "**/parser/**/*ParserImpl*.*", + "**/parser/**/*ParserImpl.*", + "**/parser/**/*ParserImplConstants.*", + "**/parser/**/*ParserImplTokenManager.*", "**/parser/**/PigletParser.*", "**/parser/**/PigletParserConstants.*", "**/parser/**/ParseException.*", @@ -260,18 +262,14 @@ allprojects { } if (!skipCheckstyle) { apply<CheckstylePlugin>() - dependencies { - checkstyle("com.puppycrawl.tools:checkstyle:${"checkstyle".v}") - checkstyle("net.hydromatic:toolbox:${"hydromatic-toolbox".v}") - } checkstyle { - // Current one is ~8.8 - // https://github.com/julianhyde/toolbox/issues/3 + toolVersion = "checkstyle".v isShowViolations = true configDirectory.set(File(rootDir, "src/main/config/checkstyle")) configFile = configDirectory.get().file("checker.xml").asFile configProperties = mapOf( - "base_dir" to rootDir.toString() + "base_dir" to rootDir.toString(), + "cache_file" to buildDir.resolve("checkstyle/cacheFile") ) } tasks.register("checkstyleAll") { @@ -283,15 +281,6 @@ allprojects { // On the other hand, supporessions.xml still analyzes the file, and // then it recognizes it should suppress all the output. excludeJavaCcGenerated() - // There are concurrency issues with Checkstyle 7.8.2 - // It could be in Checkstyle, in CheckstyleAnt task or in Gradle's Checkstyle plugin - // The bug looks like as if suppression was not working - doFirst { - semaphore.acquire() - } - doLast { - semaphore.release() - } } } if (!skipSpotless || !skipCheckstyle) { @@ -430,7 +419,17 @@ allprojects { ) removeUnusedImports() trimTrailingWhitespace() - indentWithSpaces(4) + indentWithSpaces(2) + replaceRegex("@Override should not be on its own line", "(@Override)\\s{2,}", "\$1 ") + replaceRegex("@Test should not be on its own line", "(@Test)\\s{2,}", "\$1 ") + replaceRegex("Newline in string should be at end of line", """\\n" *\+""", "\\n\"\n +") + // (?-m) disables multiline, so $ matches the very end of the file rather than end of line + replaceRegex("Remove '// End file.java' trailer", "(?-m)\n// End [^\n]++.\\w++\\s*+$", "") + replaceRegex("<p> should not be placed a the end of the line", "(?-m)\\s*+<p> *+\n \\* ", "\n *\n * <p>") + bumpThisNumberIfACustomStepChanges(1) + custom("((() preventer") { contents: String -> + ParenthesisBalancer.apply(contents) + } endWithNewline() } } diff --git a/buildSrc/settings.gradle.kts b/buildSrc/settings.gradle.kts index 0e51e6a..727edf4 100644 --- a/buildSrc/settings.gradle.kts +++ b/buildSrc/settings.gradle.kts @@ -24,6 +24,7 @@ pluginManagement { include("javacc") include("fmpp") +include("buildext") val upperCaseLetters = "\\p{Upper}".toRegex() diff --git a/buildSrc/subprojects/buildext/buildext.gradle.kts b/buildSrc/subprojects/buildext/buildext.gradle.kts new file mode 100644 index 0000000..e074f48 --- /dev/null +++ b/buildSrc/subprojects/buildext/buildext.gradle.kts @@ -0,0 +1,26 @@ +/* + * 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. + * + */ + +gradlePlugin { + plugins { + register("buildext") { + id = "calcite.buildext" + implementationClass = "org.apache.calcite.buildtools.buildext.BuildExtPlugin" + } + } +} diff --git a/buildSrc/subprojects/buildext/src/main/kotlin/org/apache/calcite/buildtools/buildext/BuildExtPlugin.kt b/buildSrc/subprojects/buildext/src/main/kotlin/org/apache/calcite/buildtools/buildext/BuildExtPlugin.kt new file mode 100644 index 0000000..463fd08 --- /dev/null +++ b/buildSrc/subprojects/buildext/src/main/kotlin/org/apache/calcite/buildtools/buildext/BuildExtPlugin.kt @@ -0,0 +1,27 @@ +/* + * 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. + * + */ + +package org.apache.calcite.buildtools.buildext + +import org.gradle.api.Plugin +import org.gradle.api.Project + +class BuildExtPlugin : Plugin<Project> { + override fun apply(target: Project) { + } +} diff --git a/buildSrc/subprojects/buildext/src/main/kotlin/org/apache/calcite/buildtools/buildext/dsl/ParenthesisBalancer.kt b/buildSrc/subprojects/buildext/src/main/kotlin/org/apache/calcite/buildtools/buildext/dsl/ParenthesisBalancer.kt new file mode 100644 index 0000000..f73de1b --- /dev/null +++ b/buildSrc/subprojects/buildext/src/main/kotlin/org/apache/calcite/buildtools/buildext/dsl/ParenthesisBalancer.kt @@ -0,0 +1,101 @@ +/* + * 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. + * + */ + +package org.apache.calcite.buildtools.buildext.dsl + +import java.util.function.Function + +private const val SINGLE_LINE_COMMENT = "//.*+" +private const val MULTILINE_COMMENT = "/[*](?>\\\\[*]|[*][^/]|[^*])*+[*]/" +private const val STRING_LITERAL = "\"(?>\\\\.|[^\"])*+\"" +private const val CHAR_LITERAL = "'(?>\\\\.|[^'])'" + +private const val KEYWORDS = "\\b(?>for|if|return|switch|try|while)\\b" +private const val KEYWORD_BLOCK = "$KEYWORDS *\\(" +private const val WHITESPACE = "(?:(?!$KEYWORDS|[(),\"'/]).)++" + +// Below Regex matches one token at a time +// That is it breaks if (!canCastFrom(/*comment*/callBinding, throwOnFailure into the following sequence +// "if (", "!canCastFrom", "(", "/*comment*/", "callBinding", ",", " throwOnFailure" +// This enables to skip strings, comments, and capture the position of commas and parenthesis + +private val tokenizer = + Regex("(?>$SINGLE_LINE_COMMENT|$MULTILINE_COMMENT|$STRING_LITERAL|$CHAR_LITERAL|$KEYWORD_BLOCK|$WHITESPACE|.)") +private val looksLikeJavadoc = Regex("^ +\\* ") + +// Note: if you change the logic, please remember to update the value in +// build.gradle.kts / bumpThisNumberIfACustomStepChanges +// Otherwise spotless would assume the files are up to date +object ParenthesisBalancer : Function<String, String> { + override fun apply(v: String): String = v.lines().map { line -> + if ('(' !in line || looksLikeJavadoc.containsMatchIn(line)) { + return@map line + } + var balance = 0 + var seenOpen = false + var commaSplit = 0 + var lastOpen = 0 + for (m in tokenizer.findAll(line)) { + val range = m.range + if (range.last - range.first > 1) { + // parenthesis always take one char, so ignore long matches + continue + } + val c = line[range.first] + if (c == '(') { + seenOpen = true + if (balance == 0) { + lastOpen = range.first + 1 + } + balance += 1 + continue + } else if (!seenOpen) { + continue + } + if (c == ',' && balance == 0) { + commaSplit = range.first + 1 + } + if (c == ')') { + balance -= 1 + } + } + if (balance <= 1) { + line + } else { + val indent = line.indexOfFirst { it != ' ' } + val res = if (commaSplit == 0) { + // f1(1,f2(2,... pattern + // ^-- lastOpen, commaSplit=0 (no split) + // It is split right after (' + line.substring(0, lastOpen) + "\n" + " ".repeat(indent + 4) + + line.substring(lastOpen) + } else { + // f1(1), f2(2,... pattern + // ^ ^-- lastOpen + // '-- commaSplit + // It is split twice: right after the comma, and after ( + line.substring(0, commaSplit) + + "\n" + " ".repeat(indent) + + line.substring(commaSplit, lastOpen).trimStart(' ') + + "\n" + " ".repeat(indent + 4) + line.substring(lastOpen) + } + // println("---\n$line\n->\n$res") + res + } + }.joinToString("\n") +} diff --git a/gradle.properties b/gradle.properties index 890252e..1741fb5 100644 --- a/gradle.properties +++ b/gradle.properties @@ -56,10 +56,8 @@ org.owasp.dependencycheck.version=5.2.2 # docker-maven-plugin.version=1.2.0 # Tools -checkstyle.version=7.8.2 +checkstyle.version=8.27 spotbugs.version=3.1.11 -# For Checkstyle -hydromatic-toolbox.version=0.3 # We support Guava versions as old as 14.0.1 (the version used by Hive) # but prefer more recent versions. diff --git a/src/main/config/checkstyle/checker.xml b/src/main/config/checkstyle/checker.xml index b1c24a9..651e52c 100644 --- a/src/main/config/checkstyle/checker.xml +++ b/src/main/config/checkstyle/checker.xml @@ -32,6 +32,8 @@ limitations under the License. <module name="Checker"> <property name="localeLanguage" value="en"/> + <property name="cacheFile" value="${cache_file}"/> + <!-- Checks for headers --> <!-- See http://checkstyle.sf.net/config_header.html --> <!-- Verify that EVERY source file has the appropriate license --> @@ -65,9 +67,16 @@ limitations under the License. <!-- No tabs allowed! --> <module name="FileTabCharacter"/> - <module name="TreeWalker"> - <property name="cacheFile" value="build/checkstyle-cachefile"/> + <!-- Checks for Size Violations. --> + <!-- See http://checkstyle.sf.net/config_sizes.html --> + <!-- Lines cannot exceed 80 chars, except if they are hyperlinks + or strings (possibly preceded by '+' and followed by say '),'. --> + <module name="LineLength"> + <property name="max" value="100"/> + <property name="ignorePattern" value="^import|@see|@link|@BaseMessage|href|^[ +]*".*"[);,]*$"/> + </module> + <module name="TreeWalker"> <!-- Checks for blocks. You know, those {}'s --> <!-- See http://checkstyle.sf.net/config_blocks.html --> <!-- No empty blocks (i.e. catch); must contain at least a comment --> @@ -206,27 +215,31 @@ limitations under the License. <property name="message" value="developers names should be in pom file"/> </module> + <module name="Regexp"> + <property name="format" value="@param ++([^ ]++) *+[\r\n]"/> + <property name="illegalPattern" value="true"/> + <property name="message" value="@param with no description. Please add description"/> + </module> + + <module name="Regexp"> + <property name="format" value="\{@link[^\r\n}]++[\r\n]"/> + <property name="illegalPattern" value="true"/> + <property name="message" value="split @link. Please ensure {@link ..} is on a single line"/> + </module> + <!-- No multi-line C-style comments except at start of line. --> <module name="Regexp"> - <property name="format" value="^ +/\*[^*][^/]$"/> + <property name="format" value="^ ++/\*[^*][^/]$"/> <property name="illegalPattern" value="true"/> <property name="message" value="C-style comment"/> </module> <module name="Regexp"> - <property name="format" value="^ +/\*$"/> + <property name="format" value="^ ++/\*$"/> <property name="illegalPattern" value="true"/> <property name="message" value="C-style comment"/> </module> - <!-- Checks for Size Violations. --> - <!-- See http://checkstyle.sf.net/config_sizes.html --> - <!-- Lines cannot exceed 80 chars, except if they are hyperlinks - or strings (possibly preceded by '+' and followed by say '),'. --> - <module name="LineLength"> - <property name="max" value="100"/> - <property name="ignorePattern" value="^import|@see|@link|@BaseMessage|href|^[ +]*".*"[);,]*$"/> - </module> <!-- Over time, we will revise this down --> <module name="MethodLength"> <property name="max" value="370"/> @@ -254,31 +267,26 @@ limitations under the License. <!-- No extra whitespace around types --> <module name="GenericWhitespace"/> - <!-- Required for SuppressionCommentFilter below --> - <module name="FileContentsHolder"/> - </module> - - <!-- Setup special comments to suppress specific checks from source files --> - <module name="SuppressionCommentFilter"> - <property name="offCommentFormat" value="CHECKSTYLE\: stop ([\w\|]+)"/> - <property name="onCommentFormat" value="CHECKSTYLE\: resume ([\w\|]+)"/> - <property name="checkFormat" value="$1"/> - </module> + <!-- Setup special comments to suppress specific checks from source files --> + <module name="SuppressionCommentFilter"> + <property name="offCommentFormat" value="CHECKSTYLE\: stop ([\w\|]+)"/> + <property name="onCommentFormat" value="CHECKSTYLE\: resume ([\w\|]+)"/> + <property name="checkFormat" value="$1"/> + </module> - <!-- Turn off all checks between OFF and ON --> - <module name="SuppressionCommentFilter"> - <property name="offCommentFormat" value="CHECKSTYLE\: OFF"/> - <property name="onCommentFormat" value="CHECKSTYLE\: ON"/> - </module> + <!-- Turn off all checks between OFF and ON --> + <module name="SuppressionCommentFilter"> + <property name="offCommentFormat" value="CHECKSTYLE\: OFF"/> + <property name="onCommentFormat" value="CHECKSTYLE\: ON"/> + </module> - <!-- Turn off checks for the next N lines. --> - <module name="SuppressWithNearbyCommentFilter"> - <property name="commentFormat" value="CHECKSTYLE: +IGNORE (\d+)"/> - <property name="influenceFormat" value="$1"/> + <!-- Turn off checks for the next N lines. --> + <module name="SuppressWithNearbyCommentFilter"> + <property name="commentFormat" value="CHECKSTYLE: +IGNORE (\d+)"/> + <property name="influenceFormat" value="$1"/> + </module> </module> - <module name="net.hydromatic.toolbox.checkstyle.HydromaticFileSetCheck"/> - <module name="SuppressionFilter"> <property name="file" value="${base_dir}/src/main/config/checkstyle/suppressions.xml"/> </module> diff --git a/src/main/config/checkstyle/suppressions.xml b/src/main/config/checkstyle/suppressions.xml index d6f7818..8f597b1 100644 --- a/src/main/config/checkstyle/suppressions.xml +++ b/src/main/config/checkstyle/suppressions.xml @@ -19,24 +19,6 @@ See the License for the specific language governing permissions and limitations under the License. --> <suppressions> - <!-- Suppress checks on generated files. --> - <suppress checks="Header" files="LICENSE"/> - <suppress checks="Header" files="NOTICE"/> - <suppress checks=".*" files="Foo.java"/> - <suppress checks=".*" files="[/\\]target[/\\]maven-archiver[/\\]pom.properties"/> - <suppress checks=".*" files="[/\\]target[/\\]generated-sources[/\\]"/> - <suppress checks=".*" files="[/\\]target[/\\]generated-test-sources[/\\]"/> - <suppress checks=".*" files="[/\\]target[/\\]embeddedCassandra[/\\]log4j-embedded-cassandra.properties"/> - <suppress checks=".*" files="org[/\\]apache[/\\]calcite[/\\]runtime[/\\]Resources.java"/> - - <suppress checks=".*" files="git.properties"/> - <suppress checks=".*" files="release.properties"/> - <suppress checks=".*" files="auth-users.properties"/> - - <!-- This file triggers https://github.com/checkstyle/checkstyle/issues/92, - through no fault of its own. --> - <suppress checks=".*" files="SqlSimpleParser.java"/> - <!-- Don't complain about field names such as cust_id --> <suppress checks=".*Name" files="JdbcExample.java"/> @@ -46,10 +28,6 @@ limitations under the License. <!-- Suppress JavadocPackage in the test packages --> <suppress checks="JavadocPackage" files="src[/\\]test[/\\]java[/\\]"/> - <!-- And likewise in ubenchmark --> - <suppress checks="JavadocPackage" files="StatementTest.java"/> - <suppress checks="JavadocPackage" files="CodeGenerationBenchmark.java"/> - <!-- Method names in Resource can have underscores --> <suppress checks="MethodName" files="CalciteResource.java"/> </suppressions>
