Copilot commented on code in PR #348:
URL:
https://github.com/apache/hugegraph-computer/pull/348#discussion_r3323900696
##########
vermeer/build.sh:
##########
@@ -33,6 +33,12 @@ go mod download
echo "Checking binary dependencies..."
./scripts/download_binaries.sh
+# Download UI assets if not exist
+echo "Checking UI assets..."
+ if [ ! -f "ui/ui/lib/jquery-3.5.1.min.js" ]; then
+ ./scripts/download_ui_assets.sh
+ fi
Review Comment:
These three lines are indented with a leading space, while the surrounding
shell statements start at column 1. The script still runs, but the inconsistent
indentation looks like a copy/paste artifact and is out of style with the rest
of `build.sh` (e.g., the `if [ ! -f "asset/assets_vfsdata.go" ]` block right
below).
##########
vermeer/Makefile:
##########
@@ -31,8 +31,13 @@ download-binaries:
@echo "Downloading binary dependencies..."
@./scripts/download_binaries.sh || (echo "Failed to download binaries"
&& exit 1)
+# Download UI assets (jQuery, Bootstrap, Glyphicons)
+download-ui-assets:
+ @echo "Downloading UI assets..."
+ @./scripts/download_ui_assets.sh || (echo "Failed to download UI
assets" && exit 1)
+
# Generate assets (vfsdata.go for web UI)
-generate-assets:
+generate-assets: download-ui-assets
Review Comment:
`generate-assets` now depends on `download-ui-assets`, which unconditionally
re-runs `npm install` and re-downloads Glyphicons from GitHub on every build
(and on every `make all`). For incremental builds this adds significant latency
and a hard network dependency even when `ui/ui/lib/` is already populated.
Consider guarding the target with a sentinel file check (as `build.sh` does
with `ui/ui/lib/jquery-3.5.1.min.js`) so the download is skipped when assets
already exist.
##########
vermeer/scripts/download_ui_assets.sh:
##########
@@ -0,0 +1,160 @@
+#!/bin/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.
+#
+
+set -e
+set -o pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
+PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)"
+UI_DIR="$PROJECT_ROOT/ui"
+LIB_DIR="$PROJECT_ROOT/ui/ui/lib"
+
+# Glyphicons source (GitHub raw) - pinned to specific commit for reproducible
builds
+GLYPHICONS_COMMIT="f7b1a17bbe64308d1d8b2b4bb2ba8a0ea621b377"
+GLYPHICONS_BASE="https://raw.githubusercontent.com/Darkseal/bootstrap4-glyphicons/${GLYPHICONS_COMMIT}/bootstrap4-glyphicons";
+
+# Colors for output
+RED='\033[0;31m'
+GREEN='\033[0;32m'
+YELLOW='\033[1;33m'
+NC='\033[0m'
+
+log_info() {
+ echo -e "${GREEN}[INFO]${NC} $1"
+}
+
+log_warn() {
+ echo -e "${YELLOW}[WARN]${NC} $1"
+}
+
+log_error() {
+ echo -e "${RED}[ERROR]${NC} $1"
+}
+
+# Download a single file from URL to target path
+download_file() {
+ local url=$1
+ local target=$2
+
+ mkdir -p "$(dirname "$target")"
+ if ! curl -sL -f "$url" -o "$target"; then
+ log_error "Failed to download: $url"
+ return 1
+ fi
+}
+
+# Main function
+main() {
+ log_info "Downloading UI assets..."
+
+ # Check npm
+ if ! command -v npm &> /dev/null; then
+ log_error "npm is required but not installed. Please install
Node.js/npm first."
+ exit 1
+ fi
+
+ # Step 1: npm install (jQuery + Bootstrap)
+ log_info "Installing npm dependencies (jQuery, Bootstrap)..."
+ cd "$UI_DIR"
+ npm install --no-audit --no-fund 2>&1 | tail -3
Review Comment:
`set -e` plus `npm install ... 2>&1 | tail -3` will not abort the build if
`npm install` fails: the pipeline's exit status is taken from `tail`, which
almost always succeeds, so a failed install silently proceeds to the `cp`
commands below and produces confusing "No such file" errors instead. Either
drop the `| tail -3` (or capture to a log file), or rely on `set -o pipefail`
actually working here by explicitly checking `${PIPESTATUS[0]}` after the
command.
##########
vermeer/scripts/download_ui_assets.sh:
##########
@@ -0,0 +1,160 @@
+#!/bin/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.
+#
+
+set -e
+set -o pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
+PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)"
+UI_DIR="$PROJECT_ROOT/ui"
+LIB_DIR="$PROJECT_ROOT/ui/ui/lib"
+
+# Glyphicons source (GitHub raw) - pinned to specific commit for reproducible
builds
+GLYPHICONS_COMMIT="f7b1a17bbe64308d1d8b2b4bb2ba8a0ea621b377"
+GLYPHICONS_BASE="https://raw.githubusercontent.com/Darkseal/bootstrap4-glyphicons/${GLYPHICONS_COMMIT}/bootstrap4-glyphicons";
+
+# Colors for output
+RED='\033[0;31m'
+GREEN='\033[0;32m'
+YELLOW='\033[1;33m'
+NC='\033[0m'
+
+log_info() {
+ echo -e "${GREEN}[INFO]${NC} $1"
+}
+
+log_warn() {
+ echo -e "${YELLOW}[WARN]${NC} $1"
+}
+
+log_error() {
+ echo -e "${RED}[ERROR]${NC} $1"
+}
+
+# Download a single file from URL to target path
+download_file() {
+ local url=$1
+ local target=$2
+
+ mkdir -p "$(dirname "$target")"
+ if ! curl -sL -f "$url" -o "$target"; then
+ log_error "Failed to download: $url"
+ return 1
+ fi
+}
+
+# Main function
+main() {
+ log_info "Downloading UI assets..."
+
+ # Check npm
+ if ! command -v npm &> /dev/null; then
+ log_error "npm is required but not installed. Please install
Node.js/npm first."
+ exit 1
+ fi
+
+ # Step 1: npm install (jQuery + Bootstrap)
+ log_info "Installing npm dependencies (jQuery, Bootstrap)..."
+ cd "$UI_DIR"
+ npm install --no-audit --no-fund 2>&1 | tail -3
+ cd "$PROJECT_ROOT"
+
+ # Step 2: Create lib directory structure
+ log_info "Copying files to ui/ui/lib/..."
+ mkdir -p "$LIB_DIR"
+
+ # Copy jQuery (npm names it jquery.min.js, rename to match original)
+ cp "$UI_DIR/node_modules/jquery/dist/jquery.min.js"
"$LIB_DIR/jquery-3.5.1.min.js"
+
+ # Copy Bootstrap CSS
+ mkdir -p "$LIB_DIR/bootstrap-4.3.1-dist/css"
+ cp "$UI_DIR/node_modules/bootstrap/dist/css/bootstrap.min.css"
"$LIB_DIR/bootstrap-4.3.1-dist/css/"
+ cp "$UI_DIR/node_modules/bootstrap/dist/css/bootstrap.min.css.map"
"$LIB_DIR/bootstrap-4.3.1-dist/css/"
+ cp "$UI_DIR/node_modules/bootstrap/dist/css/bootstrap-grid.min.css"
"$LIB_DIR/bootstrap-4.3.1-dist/css/"
+ cp "$UI_DIR/node_modules/bootstrap/dist/css/bootstrap-grid.min.css.map"
"$LIB_DIR/bootstrap-4.3.1-dist/css/"
+ cp "$UI_DIR/node_modules/bootstrap/dist/css/bootstrap-reboot.min.css"
"$LIB_DIR/bootstrap-4.3.1-dist/css/"
+ cp "$UI_DIR/node_modules/bootstrap/dist/css/bootstrap-reboot.min.css.map"
"$LIB_DIR/bootstrap-4.3.1-dist/css/"
+
+ # Copy Bootstrap JS
+ mkdir -p "$LIB_DIR/bootstrap-4.3.1-dist/js"
+ cp "$UI_DIR/node_modules/bootstrap/dist/js/bootstrap.min.js"
"$LIB_DIR/bootstrap-4.3.1-dist/js/"
+ cp "$UI_DIR/node_modules/bootstrap/dist/js/bootstrap.min.js.map"
"$LIB_DIR/bootstrap-4.3.1-dist/js/"
+ cp "$UI_DIR/node_modules/bootstrap/dist/js/bootstrap.bundle.min.js"
"$LIB_DIR/bootstrap-4.3.1-dist/js/"
+ cp "$UI_DIR/node_modules/bootstrap/dist/js/bootstrap.bundle.min.js.map"
"$LIB_DIR/bootstrap-4.3.1-dist/js/"
+
+ # Copy Bootstrap LICENSE
+ cp "$UI_DIR/node_modules/bootstrap/LICENSE"
"$LIB_DIR/bootstrap-4.3.1-dist/"
Review Comment:
The download script pins the jQuery filename to `jquery-3.5.1.min.js` and
Bootstrap directory to `bootstrap-4.3.1-dist/` (lines 82, 85–101), but the
source-of-truth version in `ui/package.json` is `"jquery": "3.5.1"` /
`"bootstrap": "4.3.1"`. If anyone bumps the version in `package.json`, the `cp`
commands here will silently produce mis-named files (or fail if the script
later assumes a fixed path), and the HTML pages that reference
`jquery-3.5.1.min.js` / `bootstrap-4.3.1-dist/` will break. Consider deriving
the version from `package.json` or at least leaving a clear comment that all
three locations must be updated together.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
