tuxji commented on code in PR #919: URL: https://github.com/apache/daffodil/pull/919#discussion_r1087258375
########## scripts/refactor/rename-dirs.sh: ########## @@ -0,0 +1,126 @@ +#!/bin/bash -ex + +# 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. + +cd "$(dirname $0)/../.." +SRC_ROOT=src/main/scala/org/apache/daffodil +TEST_ROOT=src/test/scala/org/apache/daffodil + +#MV="cp -v -R" +MV="mv" + +function rename { + lib_name=$1 + target_dir=$2 + + ( + cd $lib_name + mkdir -p $SRC_ROOT/$target_dir + $MV $SRC_ROOT/* $SRC_ROOT/$target_dir || true + rm -rf $SRC_ROOT/$target_dir/$target_dir + + if [ -e $TEST_ROOT ]; then + mkdir -p $TEST_ROOT/$target_dir + $MV $TEST_ROOT/* $TEST_ROOT/$target_dir || true + rm -rf $TEST_ROOT/$target_dir/$target_dir + fi Review Comment: I pulled your branch's latest changes using `git pull --rebase` to include your force pushed changes. When I ran `scripts/refactor/refactor.sh`, I noticed some mv commands had error messages: ``` + scripts/refactor/rename-dirs.sh ++ dirname scripts/refactor/rename-dirs.sh + cd scripts/refactor/../.. + SRC_ROOT=src/main/scala/org/apache/daffodil + TEST_ROOT=src/test/scala/org/apache/daffodil + MV=mv + rename daffodil-cli cli + lib_name=daffodil-cli + target_dir=cli + cd daffodil-cli + mkdir -p src/main/scala/org/apache/daffodil/cli + mv src/main/scala/org/apache/daffodil/InfosetTypes.scala src/main/scala/org/apache/daffodil/Main.scala src/main/scala/org/apache/daffodil/cli src/main/scala/org/apache/daffodil/debugger src/main/scala/org/apache/daffodil/cli mv: cannot move 'src/main/scala/org/apache/daffodil/cli' to a subdirectory of itself, 'src/main/scala/org/apache/daffodil/cli/cli' + true ``` All in all, I found 15 of these mv error messages: ``` mv: cannot move 'src/main/scala/org/apache/daffodil/cli' to a subdirectory of itself, 'src/main/scala/org/apache/daffodil/cli/cli' mv: cannot move 'src/test/scala/org/apache/daffodil/cli' to a subdirectory of itself, 'src/test/scala/org/apache/daffodil/cli/cli' mv: cannot move 'src/main/scala/org/apache/daffodil/core' to a subdirectory of itself, 'src/main/scala/org/apache/daffodil/core/core' mv: cannot move 'src/test/scala/org/apache/daffodil/core' to a subdirectory of itself, 'src/test/scala/org/apache/daffodil/core/core' mv: cannot move 'src/main/scala/org/apache/daffodil/io' to a subdirectory of itself, 'src/main/scala/org/apache/daffodil/io/io' mv: cannot move 'src/test/scala/org/apache/daffodil/io' to a subdirectory of itself, 'src/test/scala/org/apache/daffodil/io/io' mv: cannot move 'src/main/scala/org/apache/daffodil/lib' to a subdirectory of itself, 'src/main/scala/org/apache/daffodil/lib/lib' mv: cannot move 'src/test/scala/org/apache/daffodil/lib' to a subdirectory of itself, 'src/test/scala/org/apache/daffodil/lib/lib' mv: cannot move 'src/main/scala/org/apache/daffodil/lib' to a subdirectory of itself, 'src/main/scala/org/apache/daffodil/lib/lib' mv: cannot move 'src/main/scala/org/apache/daffodil/runtime1' to a subdirectory of itself, 'src/main/scala/org/apache/daffodil/runtime1/runtime1' mv: cannot move 'src/test/scala/org/apache/daffodil/runtime1' to a subdirectory of itself, 'src/test/scala/org/apache/daffodil/runtime1/runtime1' mv: cannot move 'src/main/scala/org/apache/daffodil/layers' to a subdirectory of itself, 'src/main/scala/org/apache/daffodil/layers/runtime1/layers' mv: cannot move 'src/test/scala/org/apache/daffodil/layers' to a subdirectory of itself, 'src/test/scala/org/apache/daffodil/layers/runtime1/layers' mv: cannot move 'src/main/scala/org/apache/daffodil/unparsers' to a subdirectory of itself, 'src/main/scala/org/apache/daffodil/unparsers/runtime1/unparsers' mv: cannot move 'daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/runtime1' to a subdirectory of itself, 'daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/runtime1/runtime1' ``` These were the only error messages I noticed in the output. The script ran smoothly to completion due to the `|| true` after the mv commands. However, it would be nice if the output didn't have any error messages. I tried one way to avoid these error messages: ```shell cd $lib_name FILES_TO_MOVE="$(ls -1d $SRC_ROOT/* | grep -Fvx $SRC_ROOT/$target_dir)" mkdir -p $SRC_ROOT/$target_dir $MV $FILES_TO_MOVE $SRC_ROOT/$target_dir || true if [ -e $TEST_ROOT ]; then FILES_TO_MOVE="$(ls -1d $TEST_ROOT/* | grep -Fvx $TEST_ROOT/$target_dir)" mkdir -p $TEST_ROOT/$target_dir $MV $FILES_TO_MOVE $TEST_ROOT/$target_dir || true fi ``` I ran the refactor script again and 12 errors were gone, but 3 errors were still there. I suspect that the script has hidden logical flaws that were obscured by the || true (which I had to leave in because of these 3 errors). Here are excerpts from the output showing these 3 errors: ``` + rename daffodil-runtime1-layers layers/runtime1 + cd daffodil-runtime1-layers + mv src/main/scala/org/apache/daffodil/layers src/main/scala/org/apache/daffodil/layers/runtime1 mv: cannot move 'src/main/scala/org/apache/daffodil/layers' to a subdirectory of itself, 'src/main/scala/org/apache/daffodil/layers/runtime1/layers' + mv src/test/scala/org/apache/daffodil/layers src/test/scala/org/apache/daffodil/layers/runtime1 mv: cannot move 'src/test/scala/org/apache/daffodil/layers' to a subdirectory of itself, 'src/test/scala/org/apache/daffodil/layers/runtime1/layers' + rename daffodil-runtime1-unparser unparsers/runtime1 + cd daffodil-runtime1-unparser + mv daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/Base64Transformer.scala daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/ByteSwapTransformer.scala daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/GZipTransformer.scala daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/LineFoldedTransformer.scala daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/runtime1 daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/runtime1 mv: cannot move 'daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/runtime1' to a subdirectory of itself, 'daffodil-runtime1-layers/src/main/scala/org/apache/daffodil/layers/runtime1/runtime1' ``` ########## scripts/refactor/fix-imports.scala: ########## @@ -0,0 +1,202 @@ +/* + * 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.BufferedWriter +import java.io.FileWriter +import java.io.File +import scala.collection.mutable.ListBuffer +import scala.collection.mutable.Map +import scala.io.Source + +object FixImports extends App { + val inline = true + val importMap = Map( + "daffodil-cli" -> "cli", + "daffodil-core" -> "core", + "daffodil-io" -> "io", + "daffodil-lib" -> "lib", + "daffodil-macro-lib" -> "lib", + "daffodil-runtime1" -> "runtime1", + "daffodil-runtime1-layers" -> "layers.runtime1", + "daffodil-runtime1-unparser" -> "unparsers.runtime1" + ) + val removeSuffix = Map( + "daffodil-runtime1-layers" -> "layers", + "daffodil-runtime1-unparser" -> "processors.unparsers" + ) + + def loadSymbolTable(file: File): Map[String, String] = { + val source = Source.fromFile(file) + val symbols = Map.empty[String, String] + for (line <- source.getLines()) { + val parts = line.split(":") + val libName = parts(0) + var packageName = parts(1) + symbols += packageName -> libName + symbols += s"${packageName}._" -> libName + } + symbols ++= Map( + "org.apache.daffodil.Main.ExitCode" -> "daffodil-cli", + "org.apache.daffodil.CLI.Util._" -> "daffodil-cli", Review Comment: We have only one Scala file (Util.scala) in the CLI package. Let's just lowercase CLI to cli here too. ########## scripts/refactor/git-restore.sh: ########## @@ -0,0 +1,60 @@ +#!/bin/bash -ex + +# 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. + +BASE_DIR="$(dirname $0)/../.." +SRC_ROOT=src/main/scala/org/apache/daffodil +TEST_ROOT=src/test/scala/org/apache/daffodil + +cd $BASE_DIR + +function clean { + lib_name=$1 + target_dir=$2 + + rm -rf $lib_name/$SRC_ROOT/$target_dir + rm -rf $lib_name/$TEST_ROOT/$target_dir +} + +git restore daffodil-* + +clean daffodil-cli cli +clean daffodil-core core +# clean daffodil-io io (SKIP) +clean daffodil-lib lib +clean daffodil-macro-lib lib +clean daffodil-runtime1 runtime1 +# clean daffodil-runtime1-layers layers (SKIP) +clean daffodil-runtime1-unparser unparsers + +rm -rf daffodil-io/src/{main,test}/scala/org/apache/daffodil/io/{layers,processors} Review Comment: Is there a reason why the git-restore script wants to undo all the moves and remove all the added files individually? If there isn't a particular reason for enumerating all the directories and files, the following 3 lines would be sufficient to restore a checkout back to its "git clone" status: ``` git restore --staged . git restore . git clean -xfd . ``` Of course, these three lines are very dangerous when run unwittingly because they will delete all files that aren't under git source control and revert all git controlled files back to their original state. The only way to recover any lost work or lost IDE files would be to have a backup that you could copy files from. If you remove the -x from git clean, at least it won't delete ignored files (your IDE files will be preserved too if you've ignored them in your own git configuration). ########## project/Rat.scala: ########## @@ -38,6 +38,13 @@ object Rat { // generated_code.[ch] examples file("daffodil-runtime2/src/test/c/examples"), + // Apache Rat thinks these files are binary since the file name contains ".lib" + file("daffodil-schematron/src/main/resources/META-INF/services/org.apache.daffodil.lib.api.ValidatorFactory"), + file("daffodil-japi/src/test/resources/META-INF/services/org.apache.daffodil.lib.api.ValidatorFactory"), + file("daffodil-sapi/src/test/resources/META-INF/services/org.apache.daffodil.lib.api.ValidatorFactory"), + file("daffodil-lib/src/main/resources/META-INF/services/org.apache.daffodil.lib.api.ValidatorFactory"), + file("daffodil-lib/src/test/resources/META-INF/services/org.apache.daffodil.lib.api.ValidatorFactory"), + // test files that cannot include the Apache license without breaking tests file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/hextest.txt"), file("daffodil-cli/src/test/resources/org/apache/daffodil/CLI/input/input1.txt"), Review Comment: It would be nice if we could lowercase CLI to cli everywhere while we're at it. -- 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]
