v8 14.4 #1256
v8 14.4 #1256
Conversation
WalkthroughThis pull request updates the ICU data version from 74 to 77, switches the v8 dependency to use a local path (../rusty_v8) instead of a published version, and removes the V8_WRAPPER_TYPE_INDEX and V8_WRAPPER_OBJECT_INDEX constants from public exports. Additionally, module import phase handling is modified to treat deferred imports (kDefer) the same as evaluated imports, and ICU data initialization in isolate setup is updated to use the newer set_common_data_77 function. Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/runtime/setup.rs (1)
135-148: Windows snapshot mutex logic never executes as intendedThe comment says “We take a mutex the first time an isolate with a snapshot…”, but the code only takes the mutex when
FIRST_SNAPSHOT_INIT.load()istrue, and only sets it totrueinside that same branch. That branch is effectively unreachable, so the mutex is never taken.Consider flipping the condition to use
swapso the first snapshot initialization actually runs under the mutex:- if cfg!(windows) - && has_snapshot - && FIRST_SNAPSHOT_INIT.load(Ordering::SeqCst) - { - let _g = SNAPSHOW_INIT_MUT.lock().unwrap(); - let res = v8::Isolate::new(params); - FIRST_SNAPSHOT_INIT.store(true, Ordering::SeqCst); - res - } else { - v8::Isolate::new(params) - } + if cfg!(windows) && has_snapshot && !FIRST_SNAPSHOT_INIT.swap(true, Ordering::SeqCst) { + let _g = SNAPSHOW_INIT_MUT.lock().unwrap(); + v8::Isolate::new(params) + } else { + v8::Isolate::new(params) + }This preserves the “first time only” behavior described in the comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.toml(1 hunks)core/lib.rs(0 hunks)core/modules/map.rs(1 hunks)core/runtime/bindings.rs(1 hunks)core/runtime/mod.rs(0 hunks)core/runtime/setup.rs(2 hunks)
💤 Files with no reviewable changes (2)
- core/runtime/mod.rs
- core/lib.rs
🧰 Additional context used
🪛 GitHub Actions: CI
Cargo.toml
[error] 1-1: No such file or directory (os error 2) while loading dependency manifest.
🔇 Additional comments (4)
core/runtime/setup.rs (2)
18-21: ICU 77 init matches dependency bumpSwitching to
v8::icu::set_common_data_77(deno_core_icudata::ICU_DATA)lines up with thedeno_core_icudata = "0.77.0"bump and looks correct. Please just confirm this matches the ICU version yourv8build is using so we don’t mix data from different versions.
121-122:unwrap_or_default()forCreateParamsis fineDefaulting
v8::CreateParamswhenmaybe_create_paramsisNonekeeps the call sites simple and matches how the rest ofcreate_isolateconfiguresparams. No functional concerns here.core/runtime/bindings.rs (1)
705-709: kDefer mapped to Evaluation for dynamic importsMapping
v8::ModuleImportPhase::kDefertoModuleImportPhase::Evaluationmakes deferred dynamic imports behave like regular evaluated imports (including the cache short‑circuit inModuleMap::load_dynamic_import). This is consistent with the static import mapping inModuleMap::new_module_from_js_source. Please confirm this is the intended semantics for"defer"in your embedding.core/modules/map.rs (1)
807-811: Static kDefer imports treated as EvaluationMapping
v8::ModuleImportPhase::kDefertoModuleImportPhase::Evaluationfor static module requests aligns with the dynamic import mapping and keeps the internal phase model simple (Evaluation vs Source). Behavior looks coherent with the rest of the module loading pipeline.
| deno_core_icudata = "0.77.0" | ||
| deno_core_testing = { path = "./testing" } | ||
| deno_error = { version = "0.7.0", features = ["serde_json", "serde", "url", "tokio"] } | ||
| deno_ops = { version = "0.248.0", path = "./ops" } | ||
| deno_unsync = "0.4.2" | ||
| serde_v8 = { version = "0.281.0", path = "./serde_v8" } | ||
| v8 = { version = "142.2.0", default-features = false } | ||
| v8 = { path = "../rusty_v8" } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the Cargo.toml file to see the actual v8 dependency configuration
cat Cargo.tomlRepository: denoland/deno_core
Length of output: 2483
🏁 Script executed:
# Search for setup.rs to verify ICU data configuration
fd setup.rsRepository: denoland/deno_core
Length of output: 83
🏁 Script executed:
# Check repository structure for rusty_v8
ls -la ../ 2>/dev/null || echo "Parent directory not accessible in sandbox"
fd -t d rusty_v8Repository: denoland/deno_core
Length of output: 593
🏁 Script executed:
# Search for CI/pipeline configuration files
fd -t f "(\.github|\.gitlab|circleci|\.circleci|azure-pipelines)" -x cat {}Repository: denoland/deno_core
Length of output: 44
🏁 Script executed:
# Read setup.rs to verify set_common_data_77 reference
cat -n core/runtime/setup.rs | head -100Repository: denoland/deno_core
Length of output: 4044
🏁 Script executed:
# Search for set_common_data reference in the codebase
rg "set_common_data" -A 2 -B 2Repository: denoland/deno_core
Length of output: 290
🏁 Script executed:
# Check for .github/workflows or CI configuration
find .github -type f 2>/dev/null || echo "No .github directory found"Repository: denoland/deno_core
Length of output: 633
🏁 Script executed:
# Search for CI or workflow references
rg "rusty_v8" -A 2 -B 2Repository: denoland/deno_core
Length of output: 1907
🏁 Script executed:
# Check the main CI workflow to see how it handles the v8 dependency
cat -n .github/workflows/ci.yml | head -80Repository: denoland/deno_core
Length of output: 4208
🏁 Script executed:
# Check ci-job.yml for dependency checkout strategy
cat -n .github/workflows/ci-job.yml | head -100Repository: denoland/deno_core
Length of output: 3602
🏁 Script executed:
# Search for any error handling or documentation about rusty_v8 checkout requirements
rg "rusty_v8" --context=5Repository: denoland/deno_core
Length of output: 4063
ICU bump is consistent; v8 path dependency requires ../rusty_v8 to be present
deno_core_icudata = "0.77.0"matches the switch toset_common_data_77incore/runtime/setup.rs, so that part is correct.- Changing
v8to{ path = "../rusty_v8" }assumes the workspace is checked out alongsiderusty_v8. The CI workflow uses standard checkout which only fetches the current repository, not sibling directories. This will cause builds to fail with "No such file or directory … while loading dependency manifest" and makes publishing to crates.io impossible without additional configuration.
If this branch is meant only for a monorepo/dev setup, that's fine; otherwise you need to either restore a versioned v8 dependency, add CI logic to checkout rusty_v8, or configure a feature flag for path-based development.
🤖 Prompt for AI Agents
In Cargo.toml around lines 25 to 31, the v8 dependency was changed to a path
("../rusty_v8") which will break CI and publishing because sibling repo is not
checked out; either revert v8 to a versioned dependency (e.g. specify the
published crate version instead of a path), or add CI steps to checkout the
rusty_v8 repository into the expected ../rusty_v8 path, or gate the path
dependency behind a dev-only feature flag and use the published v8 on default to
ensure builds and crates.io publishing succeed.
denoland/rusty_v8#1889