Skip to content

Conversation

@Pengzna
Copy link
Contributor

@Pengzna Pengzna commented Nov 8, 2025

New files added:

  1. scripts/download_binaries.sh - Scripts for automatically downloading supervisord and protoc
  2. Makefile - Provides build commands such as make init and make build

Modified files:

  1. build.sh - Added steps for automatically downloading and generating assets
  2. README.md - Updated build instructions
  3. README.zh-CN.md - Updated Chinese build instructions

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. feature New feature labels Nov 8, 2025
Copy link
Contributor

@Ethereal-O Ethereal-O left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for fixing this bug.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 9, 2025
Copy link
Contributor

@Ethereal-O Ethereal-O left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some issues. I am working on these.

TOOLS_DIR="$PROJECT_ROOT/tools"

# Versions
SUPERVISORD_VERSION="4.2.5"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

version is 0.6.9


# Download supervisord for different platforms
# MD5 checksums for supervisord v4.2.5
download_supervisord "linux_amd64" "x86_64" "" # Add MD5 if available
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct arch is 64-bit

# Download supervisord for different platforms
# MD5 checksums for supervisord v4.2.5
download_supervisord "linux_amd64" "x86_64" "" # Add MD5 if available
download_supervisord "linux_arm64" "aarch64" "" # Add MD5 if available
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct arch is ARM64

# MD5 checksums for protoc v21.12
download_protoc "linux64" "linux-x86_64" "" # Add MD5 if available
download_protoc "osxm1" "osx-aarch_64" "" # Add MD5 if available
download_protoc "win64" "win64" "" # Add MD5 if available
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will be fail because the file downloaded is named 'proto.exe'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be no need

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file can not be generated automatically. I am working on this.


# Generate assets (vfsdata.go for web UI)
generate-assets:
@echo "Generating assets..."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ Critical - 缺少错误处理

Makefile 中的多个目标没有进行错误检查。建议在关键步骤后添加错误处理,特别是 download-binariesgenerate-assets 命令。

建议改进:

Suggested change
@echo "Generating assets..."
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!"


# Downloaded binaries (should be downloaded via scripts/download_binaries.sh) #
######################
tools/supervisord/*/supervisord
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 建议补充 .gitignore 模式

当前的 .gitignore 只忽略了特定架构的文件。建议添加通配符模式以支持更多平台:

Suggested change
tools/supervisord/*/supervisord
# Downloaded binaries (should be downloaded via scripts/download_binaries.sh) #
######################
tools/supervisord/*/supervisord*
tools/protoc/*/protoc*
tools/protoc/*/bin/
tools/protoc/*/include/

这样可以覆盖 Windows 下的 .exe 文件以及其他平台变体。

@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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 文档完善建议

help 目标中的描述可以更详细一些,建议添加使用场景说明:

Suggested change
@echo " make help - Show this help message"
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"

## Build from Source

### Install dependencies
### Requirements
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 跨平台兼容性说明不足

README 中提到需要 curlunzip 工具,但没有说明不同操作系统(Linux、macOS、Windows)的具体要求和差异。

建议补充:

### 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)

```
go mod tidy

**Alternative**: Use the build script:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

‼️ build.sh 使用说明不完整

文档中提到可以使用 ./build.sh amd64./build.sh arm64,但是:

  1. 没有说明 build.sh 的完整参数列表
  2. 没有说明如果不传参数会发生什么(默认行为是什么?)
  3. 没有说明构建失败时如何排查

建议补充:

**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

# limitations under the License.
#

.PHONY: all init download-binaries generate-assets build clean help
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Makefile 最佳实践

.PHONY 声明建议按功能分组,并添加注释说明每个目标的用途,提高可维护性:

Suggested change
.PHONY: all init download-binaries generate-assets build clean help
# 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

### Development Build

For development with hot-reload of web UI:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 开发构建说明不清晰

文档中提到 go build -tags=dev 用于开发环境的热重载,但是:

  1. 没有说明 dev tag 的具体作用机制
  2. 没有说明如何实现热重载(需要额外的工具吗?)
  3. 与 assets_vfsdata.go 的关系不明确

建议改进:

### 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

### 环境要求
* Go 1.23 或更高版本
* `curl``unzip` 工具(用于下载依赖)
* 互联网连接(首次构建时需要)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 中文文档与英文文档一致性

中文文档和英文文档存在相同的问题:

  1. 跨平台兼容性说明不足
  2. build.sh 使用说明不完整
  3. 开发构建(dev tag)的说明不够清晰

建议参考英文 README 的改进建议,在中文文档中也做相应补充。保持两个版本的文档同步更新。


# Generated files (should be generated via go generate) #
######################
asset/assets_vfsdata.go
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

正确处理生成文件

删除 assets_vfsdata.go 并将其加入 .gitignore 是正确的做法。这个文件应该在构建时动态生成,而不应该提交到版本控制中。

这样可以:

  • 减少代码库大小
  • 避免每次 UI 更新时产生大量 diff
  • 确保使用最新的 UI 资源构建

@Pengzna
Copy link
Contributor Author

Pengzna commented Nov 15, 2025

see #340

@Pengzna Pengzna closed this Nov 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants