Copilot commented on code in PR #348:
URL:
https://github.com/apache/hugegraph-computer/pull/348#discussion_r3328517983
##########
vermeer/Makefile:
##########
@@ -66,20 +79,25 @@ clean-all: clean
@rm -rf tools/protoc/*/protoc
@rm -rf tools/protoc/*/bin
@rm -rf tools/protoc/*/include
+ @echo "Cleaning downloaded UI assets..."
+ @rm -rf ui/node_modules ui/package-lock.json
+ @rm -f ui/ui/lib/.downloaded ui/ui/lib/jquery-license
+ @rm -rf ui/ui/lib/bootstrap-4.3.1-dist ui/ui/lib/bootstrap4-glyphicons
Review Comment:
The cleanup hardcodes `bootstrap-4.3.1-dist`, but the download script
(`scripts/download_ui_assets.sh`) derives the directory name dynamically from
`ui/package.json` (`bootstrap-${BOOTSTRAP_VERSION}-dist`). If the Bootstrap
version in `package.json` is ever bumped, `make clean-all` will silently leave
the old downloaded directory behind. Consider using a glob like
`ui/ui/lib/bootstrap-*-dist` (and `ui/ui/lib/jquery-*.min.js` to stay
consistent) so the cleanup tracks the script.
##########
vermeer/Makefile:
##########
@@ -31,11 +31,24 @@ 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:
+ @if [ ! -f "ui/ui/lib/.downloaded" ]; then \
+ echo "Downloading UI assets..."; \
+ ./scripts/download_ui_assets.sh || (echo "Failed to download UI
assets" && exit 1); \
+ else \
+ echo "UI assets already exist, skipping download."; \
+ fi
+
# Generate assets (vfsdata.go for web UI)
-generate-assets:
- @echo "Generating assets..."
- @cd asset && go generate || (echo "Failed to generate assets" && exit 1)
- @echo "Assets generated successfully!"
+generate-assets: download-ui-assets
+ @if [ ! -f "asset/assets_vfsdata.go" ]; then \
+ echo "Generating assets..."; \
+ cd asset && go generate || (echo "Failed to generate assets" &&
exit 1); \
+ echo "Assets generated successfully!"; \
+ else \
+ echo "Assets already generated, skipping."; \
+ fi
Review Comment:
`generate-assets` now skips `go generate` whenever `asset/assets_vfsdata.go`
already exists. This breaks incremental rebuilds: if a developer changes UI
source files (e.g. `ui/ui/master.html`, `lib/functions.js`) or re-downloads new
asset versions, the embedded `assets_vfsdata.go` will not be regenerated and
the built binary will continue to serve the previous bundled assets. The skip
should at minimum depend on whether the asset sources are newer than
`assets_vfsdata.go`, or be removed so `go generate` always runs (it is fast).
Otherwise users will be silently surprised by stale UI.
##########
vermeer/scripts/download_ui_assets.sh:
##########
@@ -0,0 +1,167 @@
+#!/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
+ cd "$PROJECT_ROOT"
+
+ # Read actual versions from package.json (source of truth)
+ JQUERY_VERSION=$(node -p
"require('$UI_DIR/package.json').dependencies.jquery")
+ BOOTSTRAP_VERSION=$(node -p
"require('$UI_DIR/package.json').dependencies.bootstrap")
Review Comment:
`set -e` plus `set -o pipefail` is set at the top of the script, but
immediately after `cd "$UI_DIR"` the script runs `npm install` and then `cd
"$PROJECT_ROOT"`. `$UI_DIR` is `$PROJECT_ROOT/ui`, while `cd "$PROJECT_ROOT"`
returns to the project root — that is fine. However, the subsequent `node -p
"require('$UI_DIR/package.json')..."` calls will fail if `node` is not on PATH
(only `npm` was checked for via `command -v npm`). On minimal images (including
the Alpine builder added to the Dockerfile, where only `npm` is installed via
`apk add npm` — which does pull in nodejs, so that case is fine), but the
precondition check should still verify `node` since the script invokes it
directly. Consider adding a `command -v node` check alongside the existing
`npm` check, or parse the versions with a tool that's already guaranteed (e.g.
grep/sed) to avoid the extra runtime dependency.
--
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]
