stevedlawrence commented on code in PR #919: URL: https://github.com/apache/daffodil/pull/919#discussion_r1091865632
########## project/OsgiCheck.scala: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +import java.io.File +import sbt.MessageOnlyException +import scala.collection.mutable.Map + +object OsgiCheck { + val prog = "osgiCheck" + + // Directories in the repository root that are libraries which should be checked + val libDirMatchers = Seq("""^.*(daffodil-\S+)$""".r) + + // Only filenames that match here are considered + val fileMatcher = """.*\.(scala|java)""".r Review Comment: If this were a per subproject task, I think getting the list of files should be as simple as `sources.value`. This gets the list of all unmanaged and managed sources (i.e. everything in `src/main` and `src_managed/main`). This also doesn't include test files, so your logic to exclude tests goes away. ########## project/OsgiCheck.scala: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +import java.io.File +import sbt.MessageOnlyException +import scala.collection.mutable.Map + +object OsgiCheck { + val prog = "osgiCheck" + + // Directories in the repository root that are libraries which should be checked + val libDirMatchers = Seq("""^.*(daffodil-\S+)$""".r) + + // Only filenames that match here are considered + val fileMatcher = """.*\.(scala|java)""".r + + // Ignore these libraries, packages, and directory names + var ignoreLibs = Seq( + "daffodil-macro-lib", // multi-package + ) + val ignorePackages = Set("org.apache.daffodil") + var ignoreDirs = Set("test") + + // Statement that identifies package membership + val packageMatcher = """^\s*package (\S+);?.*$""".r + + // Keeps a mapping of (K, V) where V is a library name and K is a package known to be + // owned by that library. + val owners: Map[String, String] = Map() + var nErrors = 0 + var nWarnings = 0 + + def error(msg: String) = { + println(s"${prog}: error: ${msg}") + nErrors += 1 + } + + def warning(msg: String) = { + println(s"${prog}: warning: ${msg}") + nWarnings += 1 + } + + def checkPackage(fileName: String, libName: String, packageName: String): Unit = { + if (ignorePackages.contains(packageName)) return + owners.get(packageName) match { + case Some(ownerLib) => + if (ownerLib != libName) + error(s"${fileName}: package ${packageName} in ${libName} is already owned by ${ownerLib}") + case None => owners += packageName -> libName + } + } + + def processFile(libName: String, file: File): Unit = { + file.getName match { + case fileMatcher(_) => + case _ => return + } + var source = io.Source.fromFile(file) + try { + for (line <- source.getLines()) { + line match { + case packageMatcher(packageName) => { + checkPackage(file.getPath, libName, packageName) + return + } + case _ => + } + } + } finally source.close() + warning(s"${file.getPath}: could not find package statement") + } + + def processDir(libName: String, dir: File): Unit = { + if (ignoreDirs.contains(dir.getName)) return + for (file <- dir.listFiles()) { + if (file.isDirectory()) processDir(libName, file) else processFile(libName, file) + } + } + + def processLib(libName: String, dir: File): Unit = { + if (ignoreLibs.contains(libName)) return + processDir(libName, dir) + } + + def run(rootDir: File): Unit = { + for (file <- rootDir.listFiles()) { + for (matcher <- libDirMatchers) { + file.getPath match { + case matcher(libName) => processLib(libName, file) Review Comment: If this were a per-subproject task, you could get the libName from just `name.value`. ########## project/OsgiCheck.scala: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +import java.io.File +import sbt.MessageOnlyException +import scala.collection.mutable.Map + +object OsgiCheck { + val prog = "osgiCheck" + + // Directories in the repository root that are libraries which should be checked + val libDirMatchers = Seq("""^.*(daffodil-\S+)$""".r) + + // Only filenames that match here are considered + val fileMatcher = """.*\.(scala|java)""".r + + // Ignore these libraries, packages, and directory names + var ignoreLibs = Seq( + "daffodil-macro-lib", // multi-package + ) + val ignorePackages = Set("org.apache.daffodil") + var ignoreDirs = Set("test") + + // Statement that identifies package membership + val packageMatcher = """^\s*package (\S+);?.*$""".r + + // Keeps a mapping of (K, V) where V is a library name and K is a package known to be + // owned by that library. + val owners: Map[String, String] = Map() + var nErrors = 0 + var nWarnings = 0 + + def error(msg: String) = { + println(s"${prog}: error: ${msg}") + nErrors += 1 + } + + def warning(msg: String) = { + println(s"${prog}: warning: ${msg}") + nWarnings += 1 + } + + def checkPackage(fileName: String, libName: String, packageName: String): Unit = { + if (ignorePackages.contains(packageName)) return + owners.get(packageName) match { + case Some(ownerLib) => + if (ownerLib != libName) + error(s"${fileName}: package ${packageName} in ${libName} is already owned by ${ownerLib}") + case None => owners += packageName -> libName + } + } + + def processFile(libName: String, file: File): Unit = { + file.getName match { + case fileMatcher(_) => + case _ => return Review Comment: Return statements should really be avoided in scala. They are almost never used. ########## project/OsgiCheck.scala: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +import java.io.File +import sbt.MessageOnlyException +import scala.collection.mutable.Map + +object OsgiCheck { + val prog = "osgiCheck" + + // Directories in the repository root that are libraries which should be checked + val libDirMatchers = Seq("""^.*(daffodil-\S+)$""".r) + + // Only filenames that match here are considered + val fileMatcher = """.*\.(scala|java)""".r + + // Ignore these libraries, packages, and directory names + var ignoreLibs = Seq( + "daffodil-macro-lib", // multi-package + ) + val ignorePackages = Set("org.apache.daffodil") + var ignoreDirs = Set("test") + + // Statement that identifies package membership + val packageMatcher = """^\s*package (\S+);?.*$""".r + + // Keeps a mapping of (K, V) where V is a library name and K is a package known to be + // owned by that library. + val owners: Map[String, String] = Map() + var nErrors = 0 + var nWarnings = 0 + + def error(msg: String) = { + println(s"${prog}: error: ${msg}") + nErrors += 1 + } + + def warning(msg: String) = { + println(s"${prog}: warning: ${msg}") + nWarnings += 1 + } + + def checkPackage(fileName: String, libName: String, packageName: String): Unit = { Review Comment: The way to handle this logic in a per subproject task could be to make two tasks. One is something like "osgiOwnedPackages" and it returns a `Seq[String]` of packages that each subproject owns. Then an "osgiCheck" task would just access that task from each subproject and make sure there are no overlaps. ########## project/OsgiCheck.scala: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +import java.io.File +import sbt.MessageOnlyException +import scala.collection.mutable.Map + +object OsgiCheck { + val prog = "osgiCheck" + + // Directories in the repository root that are libraries which should be checked + val libDirMatchers = Seq("""^.*(daffodil-\S+)$""".r) Review Comment: The "sbt" way to do this is to make it so this is a single task that is added to only subprojects that we want to check. Because all our subprojects are aggregated, running the task on the root will recursively run on subprojects that define the task. ########## project/OsgiCheck.scala: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +import java.io.File +import sbt.MessageOnlyException +import scala.collection.mutable.Map + +object OsgiCheck { + val prog = "osgiCheck" + + // Directories in the repository root that are libraries which should be checked + val libDirMatchers = Seq("""^.*(daffodil-\S+)$""".r) + + // Only filenames that match here are considered + val fileMatcher = """.*\.(scala|java)""".r + + // Ignore these libraries, packages, and directory names + var ignoreLibs = Seq( + "daffodil-macro-lib", // multi-package + ) + val ignorePackages = Set("org.apache.daffodil") + var ignoreDirs = Set("test") + + // Statement that identifies package membership + val packageMatcher = """^\s*package (\S+);?.*$""".r + + // Keeps a mapping of (K, V) where V is a library name and K is a package known to be + // owned by that library. + val owners: Map[String, String] = Map() + var nErrors = 0 + var nWarnings = 0 + + def error(msg: String) = { + println(s"${prog}: error: ${msg}") + nErrors += 1 + } + + def warning(msg: String) = { + println(s"${prog}: warning: ${msg}") + nWarnings += 1 + } + + def checkPackage(fileName: String, libName: String, packageName: String): Unit = { + if (ignorePackages.contains(packageName)) return + owners.get(packageName) match { + case Some(ownerLib) => + if (ownerLib != libName) + error(s"${fileName}: package ${packageName} in ${libName} is already owned by ${ownerLib}") + case None => owners += packageName -> libName + } + } + + def processFile(libName: String, file: File): Unit = { + file.getName match { + case fileMatcher(_) => + case _ => return + } + var source = io.Source.fromFile(file) + try { + for (line <- source.getLines()) { + line match { + case packageMatcher(packageName) => { + checkPackage(file.getPath, libName, packageName) + return + } + case _ => + } + } + } finally source.close() + warning(s"${file.getPath}: could not find package statement") Review Comment: I suggest we either don't warn here, or make it an error if a package statement doesn't exist and we modify files to have a package statement, even if that's all that's in there. John pointed out the one case of this is a commented out file. If we require a package statemnt, it means if we ever uncomment that file, we can be assure the package name is correct. Or optionally, allow the package matcher to match commented out package names, and assume that's what would be used if the file were uncommented. ########## project/OsgiCheck.scala: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +import java.io.File +import sbt.MessageOnlyException +import scala.collection.mutable.Map + +object OsgiCheck { + val prog = "osgiCheck" + + // Directories in the repository root that are libraries which should be checked + val libDirMatchers = Seq("""^.*(daffodil-\S+)$""".r) + + // Only filenames that match here are considered + val fileMatcher = """.*\.(scala|java)""".r + + // Ignore these libraries, packages, and directory names + var ignoreLibs = Seq( + "daffodil-macro-lib", // multi-package + ) + val ignorePackages = Set("org.apache.daffodil") + var ignoreDirs = Set("test") + + // Statement that identifies package membership + val packageMatcher = """^\s*package (\S+);?.*$""".r + + // Keeps a mapping of (K, V) where V is a library name and K is a package known to be + // owned by that library. + val owners: Map[String, String] = Map() + var nErrors = 0 + var nWarnings = 0 + + def error(msg: String) = { + println(s"${prog}: error: ${msg}") + nErrors += 1 + } + + def warning(msg: String) = { + println(s"${prog}: warning: ${msg}") Review Comment: You can use `streams.value.log` to get the sbt logger, e.g. ```scala val logger = streams.value.log logger.warn(...) logger.error(...) ``` This way sbt knows the type of message and can colorize/hide it appropriately. ########## project/OsgiCheck.scala: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +import java.io.File +import sbt.MessageOnlyException +import scala.collection.mutable.Map + +object OsgiCheck { + val prog = "osgiCheck" + + // Directories in the repository root that are libraries which should be checked + val libDirMatchers = Seq("""^.*(daffodil-\S+)$""".r) + + // Only filenames that match here are considered + val fileMatcher = """.*\.(scala|java)""".r + + // Ignore these libraries, packages, and directory names + var ignoreLibs = Seq( + "daffodil-macro-lib", // multi-package + ) Review Comment: This also goes away with a per subproject task, you just don't add the task to projects that you want to ignore. ########## project/OsgiCheck.scala: ########## @@ -0,0 +1,112 @@ +/* + * 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. + */ + +import java.io.File +import sbt.MessageOnlyException +import scala.collection.mutable.Map + +object OsgiCheck { + val prog = "osgiCheck" + + // Directories in the repository root that are libraries which should be checked + val libDirMatchers = Seq("""^.*(daffodil-\S+)$""".r) + + // Only filenames that match here are considered + val fileMatcher = """.*\.(scala|java)""".r + + // Ignore these libraries, packages, and directory names + var ignoreLibs = Seq( + "daffodil-macro-lib", // multi-package + ) + val ignorePackages = Set("org.apache.daffodil") + var ignoreDirs = Set("test") + + // Statement that identifies package membership + val packageMatcher = """^\s*package (\S+);?.*$""".r + + // Keeps a mapping of (K, V) where V is a library name and K is a package known to be + // owned by that library. + val owners: Map[String, String] = Map() + var nErrors = 0 + var nWarnings = 0 + + def error(msg: String) = { + println(s"${prog}: error: ${msg}") + nErrors += 1 + } + + def warning(msg: String) = { + println(s"${prog}: warning: ${msg}") + nWarnings += 1 + } + + def checkPackage(fileName: String, libName: String, packageName: String): Unit = { + if (ignorePackages.contains(packageName)) return + owners.get(packageName) match { + case Some(ownerLib) => + if (ownerLib != libName) + error(s"${fileName}: package ${packageName} in ${libName} is already owned by ${ownerLib}") + case None => owners += packageName -> libName + } + } + + def processFile(libName: String, file: File): Unit = { + file.getName match { + case fileMatcher(_) => + case _ => return + } + var source = io.Source.fromFile(file) + try { + for (line <- source.getLines()) { + line match { + case packageMatcher(packageName) => { + checkPackage(file.getPath, libName, packageName) + return + } + case _ => + } + } + } finally source.close() + warning(s"${file.getPath}: could not find package statement") + } + + def processDir(libName: String, dir: File): Unit = { Review Comment: I think this processDir/Lib stuffthat recurses the filesystem goes away with per subpojrect task as well. -- 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]
