-
Notifications
You must be signed in to change notification settings - Fork 41
build: make custom cargo target dir work completely #856
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughModified .gitignore to broaden target ignore rules and restore .zed entry. Introduced symlink-target Makefile target to create cargo target directory symlinks. Refactored build orchestration to route superposition process control through make targets instead of direct binary invocations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
1060ea3 to
f8053db
Compare
| cargo test | ||
| @echo "Running superposition" | ||
| @$(CARGO_TARGET_DIR)/debug/superposition & | ||
| $(MAKE) run & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this repeats db frontend superposition though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally those should be no-ops right if already built. If not, they should be built right ?
In an ideal case all targets should be intelligent not to repeat if its dependencies did not change - but we do a lot of PHONY targets that some might be repeated and it is left to cargo to deal with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we can create a backend_run which does not build frontend.
Once we add FE tests it might become moot though.
|
|
||
| provider-template: setup superposition | ||
| @$(CARGO_TARGET_DIR)/debug/superposition & | ||
| $(MAKE) run & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would add frontend building as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ayushjain17 - can we address this separately - the dependencies are bit hay-wire. I promise I will refactor and fix the makefile :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@makefile`:
- Around line 180-183: The symlink-target Makefile recipe may create a
circular/self-referential symlink when CARGO_TARGET_DIR points to the default
./target; update the symlink-target logic to first resolve and compare the
absolute paths of $(CARGO_TARGET_DIR) and the literal target directory before
creating the link (i.e., get realpath of $(CARGO_TARGET_DIR) and of ./target or
use readlink -f) and only run ln -s when they differ and when target does not
already exist or is not a symlink; reference the symlink-target recipe and the
CARGO_TARGET_DIR variable when making this change.
🧹 Nitpick comments (1)
makefile (1)
234-246: Minor inefficiency: redundant dependency resolution.The
testtarget already depends onfrontendandsuperposition, but$(MAKE) run &re-evaluates these PHONY targets. While functionally correct (builds are idempotent), this causes redundant work.Consider extracting a lighter
run-onlytarget that skips rebuild deps, or refactoring sorundoesn't rebuild whattestalready built.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
makefile (1)
186-188:⚠️ Potential issue | 🟠 MajorRelative path for node_modules symlink may break with custom CARGO_TARGET_DIR.
The relative path
../node_modulesassumesCARGO_TARGET_DIRis exactly one directory level below the project root. IfCARGO_TARGET_DIRis set to an arbitrary location (e.g.,/tmp/cargo-targetor a shared cache), the symlink will point to the wrong location.Consider using an absolute path:
🐛 Proposed fix using absolute path
superposition: CARGO_FLAGS += --features='$(FEATURES)' superposition: cargo build $(CARGO_FLAGS) --bin superposition - `@cd` $(CARGO_TARGET_DIR) && ln -s ../node_modules node_modules || true + `@cd` $(CARGO_TARGET_DIR) && ln -s $(CURDIR)/node_modules node_modules || true
$(CURDIR)is a built-in Make variable that holds the absolute path of the directory where Make was invoked.
🧹 Nitpick comments (1)
makefile (1)
180-184: Consider adding a guard against creating a self-referential symlink.If
CARGO_TARGET_DIRresolves to the project's owntargetdirectory (the default), this would attempt to create a symlink pointing to itself. Whilecargo metadatareturns absolute paths, comparing the resolved paths would prevent unexpected behavior.🔧 Proposed fix to add a guard
symlink-target: - `@if` [ ! -e target ] && [ ! -L target ]; then \ + `@if` [ ! -e target ] && [ ! -L target ] && [ "$$(cd $(CARGO_TARGET_DIR) && pwd -P)" != "$$(pwd -P)/target" ]; then \ ln -s $(CARGO_TARGET_DIR) target; \ fiAlternatively, a simpler check using
realpathif available:symlink-target: - `@if` [ ! -e target ] && [ ! -L target ]; then \ + `@if` [ ! -e target ] && [ ! -L target ] && [ "$(CARGO_TARGET_DIR)" != "$$(pwd)/target" ]; then \ ln -s $(CARGO_TARGET_DIR) target; \ fi
f8053db to
2ecde93
Compare
Problem
The shared CARGO_TARGET_DIR approach was not possible because Leptos depended on files copied to the
targetdirectory relative to project root.Solution
This PR enables symlinking the target in case the CARGO_TARGET_DIR is set to a different value.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.