chore: add Makefile and MakefileEc2.mk, update README#78
Open
chore: add Makefile and MakefileEc2.mk, update README#78
Conversation
- Add Makefile with build, test, lint, docker and clean targets - Add MakefileEc2.mk for internal S3 deployment (mainnet, hoodi, qanet) - Update README: chainspecs now scheduled through Jade (not Emerald) - Update README: clarify ZK-trie state root validation behavior - Update README: add --rpc.eth-proof-window to CLI flags table - Add .claude/ and .omc/ to .gitignore
📝 WalkthroughWalkthroughVersion bumped to 0.2.0 in workspace configuration. Gitignore patterns expanded for Changes
Sequence Diagram(s)(omitted — changes are build/docs/configuration additions without new multi-component runtime control flow) Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Makefile (1)
21-23: Add a requiredallphony target to satisfycheckmake(minphony) rule.The Makefile is missing the required
alltarget, which triggers a minphony violation. This target should be the default aggregate entrypoint.Proposed fix
.PHONY: help help: ## Display this help. `@awk` 'BEGIN {FS = ":.*##"; printf "Usage:\n make \033[36m<target>\033[0m\n"} /^[a-zA-Z_0-9-]+:.*?##/ { printf " \033[36m%-30s\033[0m %s\n", $$1, $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } ' $(MAKEFILE_LIST) + +.PHONY: all +all: build ## Default aggregate target.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 21 - 23, Add a default aggregate phony target named "all" to satisfy the checkmake minphony rule: declare it as phony (extend the existing .PHONY list to include all) and implement an "all" target that depends on the top-level entrypoint (e.g., make all: help or other primary targets) so running `make` runs the aggregate; update any existing .PHONY declarations and ensure "help" remains untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Line 5: GIT_TAG assignment in the Makefile fails when the repo has no tags;
change the GIT_TAG ?= $(shell git describe --tags --abbrev=0) assignment to a
shell command that falls back to a safe default when describe fails (for example
return a fallback string like "v0.0.0" or the short commit hash) so the
build-release-tarballs, docker-build-push and docker-build-push-latest targets
that depend on GIT_TAG do not break; update the GIT_TAG definition accordingly
to try git describe and emit the fallback on error.
In `@MakefileEc2.mk`:
- Around line 10-11: MakefileEc2.mk currently hardcodes the release target and
path; update the EC2 build macro to use the configurable variables used in the
main Makefile: invoke cargo build with --target-dir $(CARGO_TARGET_DIR) and
--profile $(PROFILE) (instead of --release) while keeping --bin $(BINARY), and
change the copy source to $(CARGO_TARGET_DIR)/$(PROFILE)/$(BINARY) when copying
to $(DIST_DIR); modify the two lines that call cargo build and cp accordingly so
the macro respects CARGO_TARGET_DIR, PROFILE, BINARY, and DIST_DIR.
---
Nitpick comments:
In `@Makefile`:
- Around line 21-23: Add a default aggregate phony target named "all" to satisfy
the checkmake minphony rule: declare it as phony (extend the existing .PHONY
list to include all) and implement an "all" target that depends on the top-level
entrypoint (e.g., make all: help or other primary targets) so running `make`
runs the aggregate; update any existing .PHONY declarations and ensure "help"
remains untouched.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f366b392-271a-40f7-8e6b-449357fc331b
📒 Files selected for processing (5)
.gitignoreCargo.tomlMakefileMakefileEc2.mkREADME.md
- Add GIT_TAG fallback to short SHA when no tags exist - Use configurable CARGO_TARGET_DIR and PROFILE in MakefileEc2.mk
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Makefile (1)
128-131: Consider adding a phonyalltarget to satisfycheckmakeminphony.This is optional, but it will remove the current linter warning and provides a conventional entrypoint.
Proposed fix
+.PHONY: all +all: build ## Default build target. + .PHONY: clean clean: ## Clean build artifacts and the dist directory. cargo clean rm -rf $(BIN_DIR)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 128 - 131, Add a phony "all" target to the Makefile to satisfy checkmake's minphony rule: declare .PHONY: all (alongside the existing .PHONY: clean) and add a minimal all target (e.g., all: build or all: default) as the conventional entrypoint so the linter warning disappears; update any existing .PHONY line to include "all" and ensure the new target references the appropriate build target (such as the same target that produces $(BIN_DIR) or the existing build workflow).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 57-59: The Makefile 'test' target description says "unit only,
fast" but currently runs `cargo test --all`; update the target so the command
matches the description by replacing `cargo test --all` with a unit-only
invocation such as `cargo test --lib --bins` (or alternatively change the
description to reflect that it runs all tests if you prefer to keep `cargo test
--all`); edit the `test` target to use `cargo test --lib --bins` (or update the
comment) so `test` and its command are consistent.
---
Nitpick comments:
In `@Makefile`:
- Around line 128-131: Add a phony "all" target to the Makefile to satisfy
checkmake's minphony rule: declare .PHONY: all (alongside the existing .PHONY:
clean) and add a minimal all target (e.g., all: build or all: default) as the
conventional entrypoint so the linter warning disappears; update any existing
.PHONY line to include "all" and ensure the new target references the
appropriate build target (such as the same target that produces $(BIN_DIR) or
the existing build workflow).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f65e48de-f20d-4184-9ac7-8c670af544e4
📒 Files selected for processing (2)
MakefileMakefileEc2.mk
✅ Files skipped from review due to trivial changes (1)
- MakefileEc2.mk
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makefilewith build, test (unit + e2e), lint, docker, and clean targetsMakefileEc2.mkfor internal S3 deployment (mainnet, hoodi, qanet)--rpc.eth-proof-windowto CLI flags.claude/and.omc/to.gitignoreTest plan
make helpdisplays all targets correctlymake buildcompiles morph-reth binarymake fmtandmake clippypassmake testruns unit testsSummary by CodeRabbit
New Features
--rpc.eth-proof-windowCLI flag for configuring historical block availability ineth_getProof.Documentation
Chores