ctubbsii commented on a change in pull request #1910: URL: https://github.com/apache/accumulo/pull/1910#discussion_r582920014
########## File path: assemble/conf/create-jshell.sh ########## @@ -0,0 +1,134 @@ +#! /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. +# + +function addLicenseHeader(){ + export header="$1" + export jPath="$2" + + # Does an auto-generated JShell config file exists? + if [[ -e "$jPath" ]]; then + printf "%s\n" "/*" >> "$jPath" + + # Load in license header + while IFS= read -r line; do + echo "$line" >> "$jPath" + done < "$header" + + printf "%s\n" "*/" >> "$jPath" + echo " " >> "$jPath" + else + echo "Cannot add license header in $jPath" + echo "Please ensure jshell-init.jsh exists" + exit 1 + fi Review comment: The generated file doesn't hold any meaningful/copyrightable content. It's just generated import statements, so it's not strictly necessary to add the license header. The rat check error from the missing license header can be avoided by generating the file in the build tree, rather than the source tree. For maven, that means generating it in `assemble/target/` directory rather than `assemble/conf/` That should simplify things a bit. ########## File path: assemble/pom.xml ########## @@ -520,6 +520,16 @@ </arguments> </configuration> </execution> + <execution> + <id>create-jshell</id> + <goals> + <goal>exec</goal> + </goals> + <phase>compile</phase> + <configuration> + <executable>${basedir}/conf/create-jshell.sh</executable> Review comment: Since this file should not be included as part of the distribution tarball, but is instead part of the build tools, this `create-jshell.sh` file should live in `assemble/src/main/scripts/` ```suggestion <executable>src/main/scripts/create-jshell.sh</executable> ``` ########## File path: assemble/conf/create-jshell.sh ########## @@ -0,0 +1,134 @@ +#! /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. +# + +function addLicenseHeader(){ + export header="$1" + export jPath="$2" + + # Does an auto-generated JShell config file exists? + if [[ -e "$jPath" ]]; then + printf "%s\n" "/*" >> "$jPath" + + # Load in license header + while IFS= read -r line; do + echo "$line" >> "$jPath" + done < "$header" + + printf "%s\n" "*/" >> "$jPath" + echo " " >> "$jPath" + else + echo "Cannot add license header in $jPath" + echo "Please ensure jshell-init.jsh exists" + exit 1 + fi +} + +function addAccumuloAPI(){ + export srcDir="$1" + export jPath="$2" + export client="accumulo/core/client" + export data="accumulo/core/data" + export security="accumulo/core/security" + export mini="accumulo/minicluster" + export hadoop="accumulo/hadoop" + + # Does an auto-generated JShell config file exists? + if [[ ! -e "$jPath" ]]; then + echo "Cannot add APIs in $jPath" + echo "Please ensure jshell-init.jsh exists" + exit 1 + fi + + # Is the source directory valid? + if [[ ! -d "$srcDir" ]]; then + echo "$srcDir is not a valid directory. Please make sure it exists." + rm "$jPath" + exit 1 + fi + + # Does a valid JShell path and source directory exists? + if [[ -e "$jPath" ]] && [[ -d "$srcDir" ]]; then + # Add API category designator in jshell-init.jsh + case "$srcDir" in + *"$client"*) echo "// Accumulo Client API" >> "$jPath";; + *"$data"*) echo "// Accumulo Data API" >> "$jPath";; + *"$security"*) echo "// Accumulo Security API" >> "$jPath";; + *"$mini"*) echo "// Accumulo Minicluster API" >> "$jPath";; + *"$hadoop"*) echo "// Accumulo Hadoop API" >> "$jPath";; Review comment: I like how you added section headers to the generated file! ########## File path: assemble/conf/create-jshell.sh ########## @@ -0,0 +1,134 @@ +#! /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. +# + +function addLicenseHeader(){ + export header="$1" + export jPath="$2" + + # Does an auto-generated JShell config file exists? + if [[ -e "$jPath" ]]; then + printf "%s\n" "/*" >> "$jPath" + + # Load in license header + while IFS= read -r line; do + echo "$line" >> "$jPath" + done < "$header" + + printf "%s\n" "*/" >> "$jPath" + echo " " >> "$jPath" + else + echo "Cannot add license header in $jPath" + echo "Please ensure jshell-init.jsh exists" + exit 1 + fi +} + +function addAccumuloAPI(){ + export srcDir="$1" + export jPath="$2" + export client="accumulo/core/client" + export data="accumulo/core/data" + export security="accumulo/core/security" + export mini="accumulo/minicluster" + export hadoop="accumulo/hadoop" + + # Does an auto-generated JShell config file exists? + if [[ ! -e "$jPath" ]]; then + echo "Cannot add APIs in $jPath" + echo "Please ensure jshell-init.jsh exists" + exit 1 + fi + + # Is the source directory valid? + if [[ ! -d "$srcDir" ]]; then + echo "$srcDir is not a valid directory. Please make sure it exists." + rm "$jPath" + exit 1 + fi + + # Does a valid JShell path and source directory exists? + if [[ -e "$jPath" ]] && [[ -d "$srcDir" ]]; then + # Add API category designator in jshell-init.jsh + case "$srcDir" in + *"$client"*) echo "// Accumulo Client API" >> "$jPath";; + *"$data"*) echo "// Accumulo Data API" >> "$jPath";; + *"$security"*) echo "// Accumulo Security API" >> "$jPath";; + *"$mini"*) echo "// Accumulo Minicluster API" >> "$jPath";; + *"$hadoop"*) echo "// Accumulo Hadoop API" >> "$jPath";; + *) echo "// Other API" >> "$jPath";; + esac + + # Extract API info from provided source directory + mapfile -t api < <(find "$srcDir" -type f -name '*.java'| + xargs -n1 dirname| sort -u) + + # Load in API and format source directory into Java import statements + for apiPath in "${api[@]}"; do + printf "%s\n" "import ${apiPath##*/java/}.*" >> "$jPath" + done + sed -i '/^ *import / s#/#.#g' "$jPath" + echo " " >> "$jPath" + fi +} + +function main(){ + # Establish Accumulo's main base directory + SOURCE="${BASH_SOURCE[0]}" + while [[ -h "${SOURCE}" ]]; do + bin="$( cd -P "$( dirname "${SOURCE}" )" && pwd )" + SOURCE="$(readlink "${SOURCE}")" + [[ "${SOURCE}" != /* ]] && SOURCE="${bin}/${SOURCE}" + done + + # Establish file and folder paths for JShell config + export conf="$( cd -P "$( dirname "${SOURCE}" )" && pwd )"; + export jPath="$conf/jshell-init.jsh" Review comment: You should generate this in the `assemble/target` directory during the build, and then update the `assemble/src/main/assemblies/component.xml` file to ensure it is copied to the appropriate `conf/` directory by the `maven-assembly-plugin` for the distribution tarball. ########## File path: assemble/bin/accumulo ########## @@ -58,7 +58,30 @@ function main() { echo "$CLASSPATH" exit 0 fi + + # Set up path variable for default import config file + export jShellPath="$conf/jshell-init.jsh" + + # Verify what file(s) is loaded into JShell + if [[ $cmd == "jshell" ]] && [[ "$#" -eq 1 ]]; then + # Load in default Accumulo APIs into JShell + CLASSPATH="$CLASSPATH" "$@" "$jShellPath" + exit 0 + elif [[ $cmd == "jshell" ]] && [[ "$#" -ne 1 ]]; then + cmdArr=( "$@" ) + # Traverse through user bash command and verify -startup argument exists + for arg in "${!cmdArr[@]}"; do + if [[ "${cmdArr[arg]}" =~ "-startup" ]]; then + # Insert "DEFAULT" JShell script after -startup argument + cmdArr=( "${cmdArr[@]:0:arg}" "DEFAULT" "${cmdArr[@]:arg+1}" ) + fi + done + # Load in default java libraries first followed by custom file(s) + CLASSPATH="$CLASSPATH" "${cmdArr[@]}" + exit 0 + fi Review comment: CLASSPATH is probably already exported, so you might not need that part. If you do, then `export CLASSPATH` on the preceding line before the exec might be more clear. Also, you can probably inject the startup file (if it exists) before the user's own arguments, as in: ```suggestion jShellPath="$conf/jshell-init.jsh" if [[ $cmd == "jshell" ]]; then shift if [[ -f "$jShellPath" ]]; then exec "$cmd" --startup "$jShellPath" "$@" else exec "$cmd" "$@" fi fi ``` ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
