imbajin commented on code in PR #339: URL: https://github.com/apache/incubator-hugegraph-computer/pull/339#discussion_r2517267319
########## vermeer/Makefile: ########## @@ -0,0 +1,85 @@ +# +# 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. +# + +.PHONY: all init download-binaries generate-assets build clean help + +# Default target +all: generate-assets build + +# Initialize project (first time setup) +init: download-binaries + @echo "Installing Go dependencies..." + go mod download + @echo "Project initialized successfully!" + +# Download binary dependencies (supervisord, protoc) +download-binaries: + @echo "Downloading binary dependencies..." + @./scripts/download_binaries.sh + +# Generate assets (vfsdata.go for web UI) +generate-assets: + @echo "Generating assets..." Review Comment: ‼️ **Critical - 缺少错误处理** Makefile 中的多个目标没有进行错误检查。建议在关键步骤后添加错误处理，特别是 `download-binaries` 和 `generate-assets` 命令。 建议改进： ```suggestion download-binaries: @echo "Downloading binary dependencies..." @./scripts/download_binaries.sh || (echo "Failed to download binaries" && exit 1) generate-assets: @echo "Generating assets..." @cd asset && go generate || (echo "Failed to generate assets" && exit 1) @echo "Assets generated successfully!" ``` ########## vermeer/.gitignore: ########## @@ -87,3 +87,14 @@ node_modules/ # Others # ###################### test/case/ + +# Downloaded binaries (should be downloaded via scripts/download_binaries.sh) # +###################### +tools/supervisord/*/supervisord Review Comment: ⚠️ **建议补充 .gitignore 模式** 当前的 .gitignore 只忽略了特定架构的文件。建议添加通配符模式以支持更多平台： ```suggestion # Downloaded binaries (should be downloaded via scripts/download_binaries.sh) # ###################### tools/supervisord/*/supervisord* tools/protoc/*/protoc* tools/protoc/*/bin/ tools/protoc/*/include/ ``` 这样可以覆盖 Windows 下的 .exe 文件以及其他平台变体。 ########## vermeer/Makefile: ########## @@ -0,0 +1,85 @@ +# +# 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. +# + +.PHONY: all init download-binaries generate-assets build clean help + +# Default target +all: generate-assets build + +# Initialize project (first time setup) +init: download-binaries + @echo "Installing Go dependencies..." + go mod download + @echo "Project initialized successfully!" + +# Download binary dependencies (supervisord, protoc) +download-binaries: + @echo "Downloading binary dependencies..." + @./scripts/download_binaries.sh + +# Generate assets (vfsdata.go for web UI) +generate-assets: + @echo "Generating assets..." + @cd asset && go generate + @echo "Assets generated successfully!" + +# Build vermeer binary +build: + @echo "Building vermeer..." + @go build -o vermeer + @echo "Build completed: ./vermeer" + +# Build for specific platform +build-linux-amd64: + @echo "Building for linux/amd64..." + @CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o vermeer + +build-linux-arm64: + @echo "Building for linux/arm64..." + @CGO_ENABLED=0 GOOS=linux GOARCH=arm64 go build -o vermeer + +# Clean generated files and binaries +clean: + @echo "Cleaning..." + @rm -f vermeer + @rm -f asset/assets_vfsdata.go + @echo "Clean completed!" + +# Clean including downloaded binaries +clean-all: clean + @echo "Cleaning downloaded binaries..." + @rm -rf tools/supervisord/*/supervisord + @rm -rf tools/protoc/*/protoc + @rm -rf tools/protoc/*/bin + @rm -rf tools/protoc/*/include + @echo "All clean completed!" + +# Help +help: + @echo "Vermeer Build System" + @echo "" + @echo "Usage:" + @echo " make init - First time setup (download binaries + go mod download)" + @echo " make download-binaries - Download supervisord and protoc binaries" + @echo " make generate-assets - Generate assets_vfsdata.go from web UI" + @echo " make build - Build vermeer for current platform" + @echo " make build-linux-amd64 - Build for Linux AMD64" + @echo " make build-linux-arm64 - Build for Linux ARM64" + @echo " make clean - Remove generated files and binaries" + @echo " make clean-all - Remove everything including downloaded tools" + @echo " make all - Generate assets and build (default)" + @echo " make help - Show this help message" Review Comment: 🧹 **文档完善建议** help 目标中的描述可以更详细一些，建议添加使用场景说明： ```suggestion help: @echo "Vermeer Build System" @echo "" @echo "Usage:" @echo " make init - First time setup (download binaries + go mod download)" @echo " make download-binaries - Download supervisord and protoc binaries for your platform" @echo " make generate-assets - Generate assets_vfsdata.go from web UI (required before build)" @echo " make build - Build vermeer for current platform (default: local architecture)" @echo " make build-linux-amd64 - Build for Linux AMD64 (for deployment)" @echo " make build-linux-arm64 - Build for Linux ARM64 (for ARM servers)" @echo " make clean - Remove generated files and binaries (keep downloaded tools)" @echo " make clean-all - Remove everything including downloaded tools" @echo " make all - Generate assets and build (default target)" @echo " make help - Show this help message" ``` ########## vermeer/README.md: ########## @@ -48,42 +48,72 @@ Configuration file reference config/supervisor.conf ./supervisord -c supervisor.conf -d ```` -## Compile -Required -* go 1.23 +## Build from Source -### Install dependencies +### Requirements Review Comment: ⚠️ **跨平台兼容性说明不足** README 中提到需要 `curl` 和 `unzip` 工具，但没有说明不同操作系统（Linux、macOS、Windows）的具体要求和差异。 建议补充： ```markdown ### Requirements * Go 1.23 or later * `curl` and `unzip` utilities (for downloading dependencies) - Linux: Usually pre-installed, or install via package manager - macOS: Pre-installed - Windows: Install via WSL, Git Bash, or use native alternatives * Internet connection (for first-time setup) ``` ########## vermeer/README.md: ########## @@ -48,42 +48,72 @@ Configuration file reference config/supervisor.conf ./supervisord -c supervisor.conf -d ```` -## Compile -Required -* go 1.23 +## Build from Source -### Install dependencies +### Requirements +* Go 1.23 or later +* `curl` and `unzip` utilities (for downloading dependencies) +* Internet connection (for first-time setup) +### Quick Start + +**Recommended**: Use Makefile for building: + +```bash +# First time setup (downloads binary dependencies) +make init + +# Build vermeer +make ``` -go mod tidy + +**Alternative**: Use the build script: Review Comment: ‼️ **build.sh 使用说明不完整** 文档中提到可以使用 `./build.sh amd64` 或 `./build.sh arm64`，但是： 1. 没有说明 build.sh 的完整参数列表 2. 没有说明如果不传参数会发生什么（默认行为是什么？） 3. 没有说明构建失败时如何排查 建议补充： ```markdown **Alternative**: Use the build script: ```bash # For AMD64 (default if no parameter provided) ./build.sh amd64 # For ARM64 ./build.sh arm64 # The script will: # - Auto-detect your OS and architecture if no parameter is provided # - Download required tools if not present # - Generate assets and build the binary # - Exit with error message if any step fails ``` ``` ########## vermeer/Makefile: ########## @@ -0,0 +1,85 @@ +# +# 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. +# + +.PHONY: all init download-binaries generate-assets build clean help Review Comment: 🧹 **Makefile 最佳实践** .PHONY 声明建议按功能分组，并添加注释说明每个目标的用途，提高可维护性： ```suggestion # Build targets .PHONY: all init build build-linux-amd64 build-linux-arm64 # Setup and generation targets .PHONY: download-binaries generate-assets # Cleanup targets .PHONY: clean clean-all # Help target .PHONY: help ``` ########## vermeer/README.md: ########## @@ -48,42 +48,72 @@ Configuration file reference config/supervisor.conf ./supervisord -c supervisor.conf -d ```` -## Compile -Required -* go 1.23 +## Build from Source -### Install dependencies +### Requirements +* Go 1.23 or later +* `curl` and `unzip` utilities (for downloading dependencies) +* Internet connection (for first-time setup) +### Quick Start + +**Recommended**: Use Makefile for building: + +```bash +# First time setup (downloads binary dependencies) +make init + +# Build vermeer +make ``` -go mod tidy + +**Alternative**: Use the build script: + +```bash +# For AMD64 +./build.sh amd64 + +# For ARM64 +./build.sh arm64 ``` -### Local compile +The build process will automatically: +1. Download required binary tools (supervisord, protoc) +2. Generate web UI assets +3. Build the vermeer binary + +### Build Targets +```bash +make build # Build for current platform +make build-linux-amd64 # Build for Linux AMD64 +make build-linux-arm64 # Build for Linux ARM64 +make clean # Clean generated files +make help # Show all available targets ``` -go build + +### Development Build + +For development with hot-reload of web UI: + Review Comment: ⚠️ **开发构建说明不清晰** 文档中提到 `go build -tags=dev` 用于开发环境的热重载，但是： 1. 没有说明 `dev` tag 的具体作用机制 2. 没有说明如何实现热重载（需要额外的工具吗？） 3. 与 assets_vfsdata.go 的关系不明确 建议改进： ```markdown ### Development Build For development with hot-reload of web UI: ```bash # Build with dev tag - serves UI files directly from disk instead of embedded assets go build -tags=dev # This allows you to modify UI files and see changes immediately without rebuilding # Note: assets_vfsdata.go is not used when dev tag is enabled ``` ``` ########## vermeer/README.zh-CN.md: ########## @@ -48,44 +48,72 @@ worker: ./vermeer --env=worker01 ./supervisord -c supervisor.conf -d ```` -## 编译 +## 从源码编译 -* Go 1.23 +### 环境要求 +* Go 1.23 或更高版本 +* `curl` 和 `unzip` 工具(用于下载依赖) +* 互联网连接(首次构建时需要) Review Comment: ⚠️ **中文文档与英文文档一致性** 中文文档和英文文档存在相同的问题： 1. 跨平台兼容性说明不足 2. build.sh 使用说明不完整 3. 开发构建（dev tag）的说明不够清晰 建议参考英文 README 的改进建议，在中文文档中也做相应补充。保持两个版本的文档同步更新。 ########## vermeer/.gitignore: ########## @@ -87,3 +87,14 @@ node_modules/ # Others # ###################### test/case/ + +# Downloaded binaries (should be downloaded via scripts/download_binaries.sh) # +###################### +tools/supervisord/*/supervisord +tools/protoc/*/protoc +tools/protoc/*/bin/ +tools/protoc/*/include/ + +# Generated files (should be generated via go generate) # +###################### +asset/assets_vfsdata.go Review Comment: ✅ **正确处理生成文件** 删除 assets_vfsdata.go 并将其加入 .gitignore 是正确的做法。这个文件应该在构建时动态生成，而不应该提交到版本控制中。 这样可以： - 减少代码库大小 - 避免每次 UI 更新时产生大量 diff - 确保使用最新的 UI 资源构建 -- 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]
