remove hard-coded target directory#3817
remove hard-coded target directory#3817dimalinux wants to merge 18 commits intosolana-foundation:masterfrom
target directory#3817Conversation
|
@dimalinux is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
dimalinux
force-pushed
the
workspace-compat
branch
2 times, most recently
from
00d7a07 to
7351001
Compare
dimalinux
force-pushed
the
workspace-compat
branch
from
7351001 to
9c13cf9
Compare
0x4ka5h
left a comment
0x4ka5h
left a comment
There was a problem hiding this comment.
yea, lgtm now. fixed the issued
|
@dimalinux can you resolve the conflicts so I can run tests |
There was a problem hiding this comment.
Pull request overview
This PR fixes compatibility issues between Anchor and Rust workspaces by dynamically detecting the cargo target directory instead of using hard-coded paths. The solution uses cargo metadata to query the actual build artifact location, which respects both Rust workspace hierarchies and the CARGO_TARGET_DIR environment variable.
Key Changes:
- Introduced dynamic target directory resolution via
cargo metadata --no-deps --format-version=1 - Added caching mechanism in Rust using
LazyLockto avoid repeated subprocess calls - Updated all hard-coded
"target"path references across TypeScript and Rust codebases
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
ts/packages/anchor/src/workspace.ts |
Added execSync call to query cargo metadata for dynamic target directory resolution in workspace proxy |
tests/bench/tests/binary-size.ts |
Updated binary size test to use dynamic target directory from cargo metadata |
cli/src/rust_template.rs |
Updated program keypair path generation to use new target_dir() helper function |
cli/src/lib.rs |
Implemented target_dir() and target_dir_no_cache() functions with LazyLock caching; updated all hard-coded target paths |
cli/src/config.rs |
Replaced hard-coded "target" paths with target_dir() function calls throughout config handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@0x4ka5h wrote
Done. Let me know if there is anything else that I can do. |
| fn target_dir_no_cache() -> Result<PathBuf> { | ||
| // `cargo metadata` produces a JSON blob from which we extract the | ||
| // `target_directory` field. | ||
| let output = std::process::Command::new("cargo") | ||
| .args(["metadata", "--no-deps", "--format-version=1"]) | ||
| .output() | ||
| .context("Failed to execute 'cargo metadata'")?; | ||
|
|
||
| if !output.status.success() { | ||
| let stderr_msg = String::from_utf8_lossy(&output.stderr); | ||
| if stderr_msg.contains("Cargo.toml") { | ||
| // `anchor init` starts populating the cargo artifacts dir | ||
| // before creating `Cargo.toml`, in which case "target" in | ||
| // the current dir is the desired behavior. | ||
| return Ok(PathBuf::from("target")); | ||
| } | ||
| eprintln!("'cargo metadata' failed with: {stderr_msg}"); | ||
| std::process::exit(output.status.code().unwrap_or(1)); | ||
| } |
There was a problem hiding this comment.
On a second review, I think this is a confusing design with 4 distinct results
- If the
cargoexecution fails, we return an error - If
cargoexecution succeeds, but the status is not success- We use a heuristic to special case the error from the
anchor initcase - Otherwise, we print an error and hard exit immediately
- We use a heuristic to special case the error from the
- Otherwise, the output is parsed and used
I would suggest
- Use
Path::from("target")directly as previously inanchor initas we know special handling is required, so calling this is pointless - Either always exit, or always return an error from this function
- I would probably suggest the former, as there's not much that can be done to handle this error, and it simplifies the API for callers
There was a problem hiding this comment.
@jamie-osec: I implemented both suggestions.
The anchor init handling to simplify the responsibilities of target_dir() is in this commit: dimalinux@27d6739
Having target_dir() exit the process in all failure scenarios is in this commit:
dimalinux@3bc3a0e
Anchor is not playing nice with Rust workspaces. If you try to place your Anchor based project as a member in a Rust workspace, the cargo build commands issued by Anchor will place build artifacts in the
target/directory at the top of the Rust workspace, while other Anchor tooling assumes that thetarget/directory is located at the top of the "Anchor workspace" (whereAnchor.tomlis located). The workaround people have been using before this pull request is to add a symbolic link fromrust-workspace/anchor-member/targettorust-workspace/target.Others in the PR issue were having problems because Anchor was not respecting the
CARGO_TARGET_DIRenvironment variable.This PR fixes both problems by asking
cargo(via thecargo metadatacommand) what directory is currently being used for build artifacts.Fixes #958