fix(wasm): respect CARGO_TARGET_DIR in WASM build scripts#3155
Open
fix(wasm): respect CARGO_TARGET_DIR in WASM build scripts#3155
Conversation
WASM build scripts hardcoded `../../target` as the Cargo output directory. When CARGO_TARGET_DIR is set to a custom location, builds fail because wasm-bindgen cannot find the compiled .wasm artifacts. Use CARGO_TARGET_DIR with fallback to the repo's target/ directory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe pull request adds a configurable Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
Consistent with the legacy build scripts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/wasm-dpp/scripts/build-wasm.sh`:
- Around line 19-20: REPO_ROOT is only going up two levels so it resolves to
packages/ instead of the repo root, causing CARGO_OUT_DIR fallback to point at
$REPO_ROOT/target inside packages; update the REPO_ROOT computation used by
CARGO_OUT_DIR by adding one more parent traversal (change the path in the
REPO_ROOT assignment from "$(dirname "${BASH_SOURCE[0]}")/../.." to use three
".." segments so REPO_ROOT points to the repository root and CARGO_OUT_DIR
defaults to $REPO_ROOT/target as intended), keeping the rest of the script
(including the later wasm-bindgen usage) unchanged.
In `@packages/wasm-drive-verify/scripts/build-wasm.sh`:
- Around line 19-20: REPO_ROOT is computed with "/../.." which is off-by-one for
scripts located in packages/*/scripts; update the path in the REPO_ROOT
assignment to use "/../../.." so it resolves to the repository root (change the
assignment that sets REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." &&
pwd)" to use "/../../.." instead), leaving the CARGO_OUT_DIR fallback logic
(CARGO_OUT_DIR="${CARGO_TARGET_DIR:-$REPO_ROOT/target}") unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/scripts/build-wasm.shpackages/wasm-dpp/scripts/build-wasm.shpackages/wasm-drive-verify/scripts/analyze-module-combinations.shpackages/wasm-drive-verify/scripts/build-separate-modules.shpackages/wasm-drive-verify/scripts/build-wasm.shpackages/wasm-drive-verify/scripts/quick-size-check.sh
Scripts under packages/*/scripts/ are 3 levels deep, not 2. The previous /../.. resolved to packages/ instead of the repo root, which would break the CARGO_OUT_DIR fallback when CARGO_TARGET_DIR is unset. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/wasm-drive-verify/scripts/build-wasm.sh`:
- Around line 19-22: The BUILD_COMMAND and BINDGEN_COMMAND strings use unquoted
CARGO_OUT_DIR (and other vars) which will break if CARGO_TARGET_DIR contains
spaces; change these to build command arrays or ensure each variable is properly
quoted/expanded when constructing/executing the command (update BUILD_COMMAND
and BINDGEN_COMMAND usage to use arrays or wrap
"${CARGO_OUT_DIR}/${TARGET}/${PROFILE}/wasm_drive_verify.wasm" and other
variables in quotes) so word-splitting is avoided when running the commands.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/wasm-dpp/scripts/build-wasm.shpackages/wasm-drive-verify/scripts/analyze-module-combinations.shpackages/wasm-drive-verify/scripts/build-separate-modules.shpackages/wasm-drive-verify/scripts/build-wasm.shpackages/wasm-drive-verify/scripts/quick-size-check.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/wasm-drive-verify/scripts/quick-size-check.sh
- packages/wasm-dpp/scripts/build-wasm.sh
- packages/wasm-drive-verify/scripts/analyze-module-combinations.sh
Comment on lines
+19
to
+22
| REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../../.." && pwd)" | ||
| CARGO_OUT_DIR="${CARGO_TARGET_DIR:-$REPO_ROOT/target}" | ||
| BUILD_COMMAND="cargo build --config net.git-fetch-with-cli=true --target=${TARGET} ${PROFILE_ARG}" | ||
| BINDGEN_COMMAND="wasm-bindgen --out-dir=${OUTPUT_DIR} --target=web --omit-default-module-path ../../target/${TARGET}/${PROFILE}/wasm_drive_verify.wasm" | ||
| BINDGEN_COMMAND="wasm-bindgen --out-dir=${OUTPUT_DIR} --target=web --omit-default-module-path ${CARGO_OUT_DIR}/${TARGET}/${PROFILE}/wasm_drive_verify.wasm" |
Contributor
There was a problem hiding this comment.
Quote CARGO_OUT_DIR to avoid word-splitting. If CARGO_TARGET_DIR contains spaces, the current string-based command will break. Consider using an array.
🛠️ Suggested fix
-BINDGEN_COMMAND="wasm-bindgen --out-dir=${OUTPUT_DIR} --target=web --omit-default-module-path ${CARGO_OUT_DIR}/${TARGET}/${PROFILE}/wasm_drive_verify.wasm"
+BINDGEN_COMMAND=(wasm-bindgen --out-dir="${OUTPUT_DIR}" --target=web --omit-default-module-path "${CARGO_OUT_DIR}/${TARGET}/${PROFILE}/wasm_drive_verify.wasm")
@@
- AR=${AR_PATH} CC=${CLANG_PATH} ${BINDGEN_COMMAND}
+ AR=${AR_PATH} CC=${CLANG_PATH} "${BINDGEN_COMMAND[@]}"
@@
- ${BINDGEN_COMMAND}
+ "${BINDGEN_COMMAND[@]}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/wasm-drive-verify/scripts/build-wasm.sh` around lines 19 - 22, The
BUILD_COMMAND and BINDGEN_COMMAND strings use unquoted CARGO_OUT_DIR (and other
vars) which will break if CARGO_TARGET_DIR contains spaces; change these to
build command arrays or ensure each variable is properly quoted/expanded when
constructing/executing the command (update BUILD_COMMAND and BINDGEN_COMMAND
usage to use arrays or wrap
"${CARGO_OUT_DIR}/${TARGET}/${PROFILE}/wasm_drive_verify.wasm" and other
variables in quotes) so word-splitting is avoided when running the commands.
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.
Issue being fixed or feature implemented
WASM build scripts (
build-wasm.shand related utility scripts) hardcode../../targetas the Cargo output directory. WhenCARGO_TARGET_DIRis set to a custom path (e.g./home/user/.cache/rust-target),wasm-bindgenfails because it cannot find the compiled.wasmartifacts at the expected location.This primarily affects
wasm-drive-verify(which usescargo builddirectly) andwasm-dpp(which has its own legacy build script), but all WASM-related scripts were affected.What was done?
Replaced all hardcoded
../../targetreferences in WASM build scripts with aCARGO_OUT_DIRvariable that respectsCARGO_TARGET_DIR, falling back to the repo'starget/directory when the env var is not set.Files changed:
packages/scripts/build-wasm.sh— unified build script (3 path references)packages/wasm-dpp/scripts/build-wasm.sh— wasm-dpp legacy build scriptpackages/wasm-drive-verify/scripts/build-wasm.sh— wasm-drive-verify legacy build scriptpackages/wasm-drive-verify/scripts/build-separate-modules.sh— module build utilitypackages/wasm-drive-verify/scripts/quick-size-check.sh— size check utilitypackages/wasm-drive-verify/scripts/analyze-module-combinations.sh— analysis utilityHow Has This Been Tested?
yarn setupend-to-end withCARGO_TARGET_DIR=/home/ubuntu/.cache/rust-targetset — all WASM packages (wasm-drive-verify,wasm-dpp,wasm-dpp2,wasm-sdk) build successfully.CARGO_TARGET_DIRis not set, the scripts fall back to the default<repo>/target/directory.Breaking Changes
None
Checklist:
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit