chore: add makefile for tools #2743
Conversation
Summary of ChangesHello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This PR introduces a dedicated Makefile ( Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Claude finished @tac0turtle's task —— View job Code Review CompleteI've reviewed the PR and identified several areas for improvement. Overall, this is a good addition that provides useful build targets for the tools, but there are some maintainability and correctness issues to address. Critical Issues1. LDFLAGS Variable Conflict (tools/tools.mk:12-14)Issue: The LDFLAGS := \
-X github.com/evstack/ev-node/pkg/cmd.Version=$(VERSION) \
-X github.com/evstack/ev-node/pkg/cmd.GitSHA=$(GITSHA)This targets Impact: Version information won't be correctly embedded in the tool binaries. The linker flags will try to set variables in a package path that doesn't match the tools' main packages. Recommendation: Use a dedicated variable name like TOOLS_LDFLAGS := \
-X main.Version=$(VERSION) \
-X main.GitSHA=$(GITSHA)Then update all build/install commands to use 2. Missing Version Variables in Tool BinariesIssue: None of the tool main packages (da-debug, blob-decoder, cache-analyzer) define Impact: Even with correct LDFLAGS, the version injection won't work because the target variables don't exist. Recommendation: Add version variables to each tool's main.go: var (
Version = "dev"
GitSHA = "unknown"
)Then expose them through a version command or Maintainability Issues3. Significant Code Duplication (tools/tools.mk:16-73)Issue: The build and install targets are highly repetitive. Each tool has nearly identical target definitions, differing only in the tool name. This violates the DRY principle and makes maintenance harder:
Current pattern (repeated 3 times for build, 3 times for install): build-tool-da-debug:
@echo "--> Building da-debug tool"
@mkdir -p $(TOOLS_BUILD_DIR)
@cd tools/da-debug && go build -ldflags "$(LDFLAGS)" -o $(TOOLS_BUILD_DIR)/da-debug .
@echo "--> da-debug built: $(TOOLS_BUILD_DIR)/da-debug"
.PHONY: build-tool-da-debugRecommendation: Use Make's # Define build template
define BUILD_TOOL_template
## build-tool-$(1): Build $(1) tool
build-tool-$(1):
@echo "--> Building $(1) tool"
@mkdir -p $$(TOOLS_BUILD_DIR)
@cd tools/$(1) && go build -ldflags "$$(TOOLS_LDFLAGS)" -o $$(TOOLS_BUILD_DIR)/$(1) .
@echo "--> $(1) built: $$(TOOLS_BUILD_DIR)/$(1)"
.PHONY: build-tool-$(1)
endef
# Define install template
define INSTALL_TOOL_template
## install-tool-$(1): Install $(1) tool to Go bin
install-tool-$(1):
@echo "--> Installing $(1) tool"
@cd tools/$(1) && go install -ldflags "$$(TOOLS_LDFLAGS)" .
@echo "--> $(1) installed to Go bin"
.PHONY: install-tool-$(1)
endef
# Generate all targets
$(foreach tool,$(TOOLS),$(eval $(call BUILD_TOOL_template,$(tool))))
$(foreach tool,$(TOOLS),$(eval $(call INSTALL_TOOL_template,$(tool))))Benefits:
Documentation & Quality4. README Improvements Look Good ✅The additions to the README files are excellent:
These provide clear installation instructions using 5. Good Build System Integration ✅The integration into the main Makefile (line 6) follows the existing pattern and the targets are well-organized with:
Recommendations SummaryPriority: High
Priority: Medium Optional Enhancements Testing NotesI recommend testing the build targets to verify: make build-tools # Should build all three tools
make install-tools # Should install all three tools
make build-tool-da-debug # Should build individual tool
make list-tools # Should list available toolsAfter fixing the LDFLAGS issues, verify version injection works: ./build/da-debug --version # Should show actual version |
|
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
Thank you for adding this Makefile for the tools. I've found a couple of areas for improvement related to maintainability and a bug in how version information is handled. My main suggestion combines a fix for the bug with a refactoring to make the Makefile much cleaner and easier to maintain in the future. Please take a look at the detailed comment.
| LDFLAGS ?= \ | ||
| -X main.Version=$(VERSION) \ | ||
| -X main.GitSHA=$(GITSHA) | ||
|
|
||
| # Individual tool build targets | ||
| ## build-tool-da-debug: Build da-debug tool | ||
| build-tool-da-debug: | ||
| @echo "--> Building da-debug tool" | ||
| @mkdir -p $(TOOLS_BUILD_DIR) | ||
| @cd tools/da-debug && go build -ldflags "$(LDFLAGS)" -o $(TOOLS_BUILD_DIR)/da-debug . | ||
| @echo "--> da-debug built: $(TOOLS_BUILD_DIR)/da-debug" | ||
| .PHONY: build-tool-da-debug | ||
|
|
||
| ## build-tool-blob-decoder: Build blob-decoder tool | ||
| build-tool-blob-decoder: | ||
| @echo "--> Building blob-decoder tool" | ||
| @mkdir -p $(TOOLS_BUILD_DIR) | ||
| @cd tools/blob-decoder && go build -ldflags "$(LDFLAGS)" -o $(TOOLS_BUILD_DIR)/blob-decoder . | ||
| @echo "--> blob-decoder built: $(TOOLS_BUILD_DIR)/blob-decoder" | ||
| .PHONY: build-tool-blob-decoder | ||
|
|
||
| ## build-tool-cache-analyzer: Build cache-analyzer tool | ||
| build-tool-cache-analyzer: | ||
| @echo "--> Building cache-analyzer tool" | ||
| @mkdir -p $(TOOLS_BUILD_DIR) | ||
| @cd tools/cache-analyzer && go build -ldflags "$(LDFLAGS)" -o $(TOOLS_BUILD_DIR)/cache-analyzer . | ||
| @echo "--> cache-analyzer built: $(TOOLS_BUILD_DIR)/cache-analyzer" | ||
| .PHONY: build-tool-cache-analyzer | ||
|
|
||
| # Build all tools | ||
| ## build-tools: Build all tools | ||
| build-tools: $(addprefix build-tool-, $(TOOLS)) | ||
| @echo "--> All tools built successfully!" | ||
| .PHONY: build-tools | ||
|
|
||
| # Install individual tools | ||
| ## install-tool-da-debug: Install da-debug tool to Go bin | ||
| install-tool-da-debug: | ||
| @echo "--> Installing da-debug tool" | ||
| @cd tools/da-debug && go install -ldflags "$(LDFLAGS)" . | ||
| @echo "--> da-debug installed to Go bin" | ||
| .PHONY: install-tool-da-debug | ||
|
|
||
| ## install-tool-blob-decoder: Install blob-decoder tool to Go bin | ||
| install-tool-blob-decoder: | ||
| @echo "--> Installing blob-decoder tool" | ||
| @cd tools/blob-decoder && go install -ldflags "$(LDFLAGS)" . | ||
| @echo "--> blob-decoder installed to Go bin" | ||
| .PHONY: install-tool-blob-decoder | ||
|
|
||
| ## install-tool-cache-analyzer: Install cache-analyzer tool to Go bin | ||
| install-tool-cache-analyzer: | ||
| @echo "--> Installing cache-analyzer tool" | ||
| @cd tools/cache-analyzer && go install -ldflags "$(LDFLAGS)" . | ||
| @echo "--> cache-analyzer installed to Go bin" | ||
| .PHONY: install-tool-cache-analyzer | ||
|
|
||
| # Install all tools | ||
| ## install-tools: Install all tools to Go bin | ||
| install-tools: $(addprefix install-tool-, $(TOOLS)) | ||
| @echo "--> All tools installed successfully!" | ||
| .PHONY: install-tools |
There was a problem hiding this comment.
This new Makefile for tools has a couple of areas for improvement regarding maintainability and correctness.
- Code Duplication: The
build-tool-*andinstall-tool-*targets are highly repetitive. This makes the file verbose and harder to maintain. Adding a new tool requires duplicating code blocks, which is error-prone. - Incorrect Linker Flags: The
LDFLAGSvariable is defined with?=and is being overridden by a definition inscripts/build.mkmeant for a different application. This means the version information is not being correctly embedded into the tool binaries. Furthermore, the tool'smainpackages are missing theVersionandGitSHAvariables that the linker flags are trying to set.
I suggest a refactoring that addresses both issues. This involves:
- Using
defineandforeachto generate the build/install targets from a template, adhering to the DRY (Don't Repeat Yourself) principle. - Using a dedicated variable
TOOLS_LDFLAGSfor the tools' linker flags to avoid conflicts.
Here is a suggested replacement that incorporates these improvements. Please note that you will still need to add the Version and GitSHA variables to the main package of each Go tool for the version injection to work.
TOOLS_LDFLAGS := \
-X main.Version=$(VERSION) \
-X main.GitSHA=$(GITSHA)
# --- Template for building a tool ---
define BUILD_TOOL_template
## build-tool-$(1): Build $(1) tool
build-tool-$(1):
@echo "--> Building $(1) tool"
@mkdir -p $$(TOOLS_BUILD_DIR)
@cd tools/$(1) && go build -ldflags "$$(TOOLS_LDFLAGS)" -o $$(TOOLS_BUILD_DIR)/$(1) .
@echo "--> $(1) built: $$(TOOLS_BUILD_DIR)/$(1)"
.PHONY: build-tool-$(1)
endef
# --- Template for installing a tool ---
define INSTALL_TOOL_template
## install-tool-$(1): Install $(1) tool to Go bin
install-tool-$(1):
@echo "--> Installing $(1) tool"
@cd tools/$(1) && go install -ldflags "$$(TOOLS_LDFLAGS)" .
@echo "--> $(1) installed to Go bin"
.PHONY: install-tool-$(1)
endef
# --- Generate targets from templates ---
$(foreach tool,$(TOOLS),$(eval $(call BUILD_TOOL_template,$(tool))))
$(foreach tool,$(TOOLS),$(eval $(call INSTALL_TOOL_template,$(tool))))
# Build all tools
## build-tools: Build all tools
build-tools: $(addprefix build-tool-, $(TOOLS))
@echo "--> All tools built successfully!"
.PHONY: build-tools
# Install all tools
## install-tools: Install all tools to Go bin
install-tools: $(addprefix install-tool-, $(TOOLS))
@echo "--> All tools installed successfully!"
.PHONY: install-tools
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2743 +/- ##
=======================================
Coverage 62.17% 62.17%
=======================================
Files 79 79
Lines 8497 8497
=======================================
Hits 5283 5283
Misses 2721 2721
Partials 493 493
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tac0turtle
force-pushed
the
marko/tools-make
branch
from
eaafd01 to
882f535
Compare
* main: feat(store)!: add batching for atomicity (#2746) refactor(apps): rollback cmd updates (#2744) chore: add makefile for tools (#2743) chore: fix markdown lint (#2742) build(deps): Bump the all-go group across 5 directories with 6 updates (#2738) refactor(block): improve cancellation (#2741) chore: make the prompt go oriented (#2739) perf(block): use `sync/atomic` instead of mutexes (#2735)
Overview