From 3f8efe5289d88097ab49e7a8fcda763c2823376b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Tue, 23 Jul 2024 01:01:31 +0100 Subject: [PATCH] Revert "chore: move all node-api impl to ext (#24662)" (#24680) This reverts commit d00fbd70258a77a267fe20bdd2c4a028c799b693. Reverting because, it caused a failure during v1.45.3 publish: https://github.com/denoland/deno/actions/runs/10048730693/job/27773718095 --- Cargo.lock | 4 +- Cargo.toml | 4 +- cli/Cargo.toml | 1 + cli/build.rs | 2 +- cli/main.rs | 1 + cli/mainrt.rs | 1 + cli/napi/README.md | 114 ++++++++++++++++++ .../generated_symbol_exports_list_linux.def | 0 .../generated_symbol_exports_list_macos.def | 0 .../generated_symbol_exports_list_windows.def | 0 {ext => cli}/napi/js_native_api.rs | 10 +- cli/napi/mod.rs | 20 +++ {ext => cli}/napi/node_api.rs | 4 +- {ext => cli}/napi/sym/Cargo.toml | 0 {ext => cli}/napi/sym/README.md | 0 {ext => cli}/napi/sym/lib.rs | 2 +- {ext => cli}/napi/sym/symbol_exports.json | 0 {ext => cli}/napi/util.rs | 30 +++-- ext/napi/Cargo.toml | 3 - ext/napi/README.md | 114 ------------------ ext/napi/lib.rs | 15 --- tools/napi/generate_symbols_lists.js | 4 +- 22 files changed, 165 insertions(+), 164 deletions(-) create mode 100644 cli/napi/README.md rename {ext => cli}/napi/generated_symbol_exports_list_linux.def (100%) rename {ext => cli}/napi/generated_symbol_exports_list_macos.def (100%) rename {ext => cli}/napi/generated_symbol_exports_list_windows.def (100%) rename {ext => cli}/napi/js_native_api.rs (99%) create mode 100644 cli/napi/mod.rs rename {ext => cli}/napi/node_api.rs (99%) rename {ext => cli}/napi/sym/Cargo.toml (100%) rename {ext => cli}/napi/sym/README.md (100%) rename {ext => cli}/napi/sym/lib.rs (93%) rename {ext => cli}/napi/sym/symbol_exports.json (100%) rename {ext => cli}/napi/util.rs (89%) diff --git a/Cargo.lock b/Cargo.lock index b40f5a9b86..f381a5a89f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1164,6 +1164,7 @@ dependencies = [ "lsp-types", "memmem", "monch", + "napi_sym", "nix 0.26.2", "notify", "once_cell", @@ -1687,10 +1688,7 @@ version = "0.91.0" dependencies = [ "deno_core", "deno_permissions", - "libc", "libloading 0.7.4", - "log", - "napi_sym", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 6bb37d6561..17262aae5f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,6 +5,7 @@ resolver = "2" members = [ "bench_util", "cli", + "cli/napi/sym", "ext/broadcast_channel", "ext/cache", "ext/canvas", @@ -18,7 +19,6 @@ members = [ "ext/io", "ext/kv", "ext/napi", - "ext/napi/sym", "ext/net", "ext/node", "ext/url", @@ -52,7 +52,7 @@ deno_media_type = { version = "0.1.4", features = ["module_specifier"] } deno_permissions = { version = "0.21.0", path = "./runtime/permissions" } deno_runtime = { version = "0.169.0", path = "./runtime" } deno_terminal = "0.2.0" -napi_sym = { version = "0.91.0", path = "./ext/napi/sym" } +napi_sym = { version = "0.91.0", path = "./cli/napi/sym" } test_util = { package = "test_server", path = "./tests/util/server" } denokv_proto = "0.8.1" diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 671b2635d6..a1b0cd7dd1 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -78,6 +78,7 @@ deno_semver = "=0.5.7" deno_task_shell = "=0.17.0" deno_terminal.workspace = true eszip = "=0.72.2" +napi_sym.workspace = true async-trait.workspace = true base32.workspace = true diff --git a/cli/build.rs b/cli/build.rs index ecfc19dc06..f131bc1dc8 100644 --- a/cli/build.rs +++ b/cli/build.rs @@ -396,7 +396,7 @@ fn main() { } os => format!("generated_symbol_exports_list_{}.def", os), }; - let symbols_path = std::path::Path::new("../ext/napi") + let symbols_path = std::path::Path::new("napi") .join(symbols_file_name) .canonicalize() .expect( diff --git a/cli/main.rs b/cli/main.rs index c54c93b86e..4264e1610c 100644 --- a/cli/main.rs +++ b/cli/main.rs @@ -15,6 +15,7 @@ mod js; mod jsr; mod lsp; mod module_loader; +mod napi; mod node; mod npm; mod ops; diff --git a/cli/mainrt.rs b/cli/mainrt.rs index aafbf79320..ef163dd00f 100644 --- a/cli/mainrt.rs +++ b/cli/mainrt.rs @@ -15,6 +15,7 @@ mod errors; mod file_fetcher; mod http_util; mod js; +mod napi; mod node; mod npm; mod resolver; diff --git a/cli/napi/README.md b/cli/napi/README.md new file mode 100644 index 0000000000..7b359ac6ec --- /dev/null +++ b/cli/napi/README.md @@ -0,0 +1,114 @@ +# napi + +This directory contains source for Deno's Node-API implementation. It depends on +`napi_sym` and `deno_napi`. + +Files are generally organized the same as in Node.js's implementation to ease in +ensuring compatibility. + +## Adding a new function + +Add the symbol name to +[`cli/napi_sym/symbol_exports.json`](../napi_sym/symbol_exports.json). + +```diff +{ + "symbols": [ + ... + "napi_get_undefined", +- "napi_get_null" ++ "napi_get_null", ++ "napi_get_boolean" + ] +} +``` + +Determine where to place the implementation. `napi_get_boolean` is related to JS +values so we will place it in `js_native_api.rs`. If something is not clear, +just create a new file module. + +See [`napi_sym`](../napi_sym/) for writing the implementation: + +```rust +#[napi_sym::napi_sym] +pub fn napi_get_boolean( + env: *mut Env, + value: bool, + result: *mut napi_value, +) -> Result { + // ... + Ok(()) +} +``` + +Update the generated symbol lists using the script: + +``` +deno run --allow-write tools/napi/generate_symbols_lists.js +``` + +Add a test in [`/tests/napi`](../../tests/napi/). You can also refer to Node.js +test suite for Node-API. + +```js +// tests/napi/boolean_test.js +import { assertEquals, loadTestLibrary } from "./common.js"; +const lib = loadTestLibrary(); +Deno.test("napi get boolean", function () { + assertEquals(lib.test_get_boolean(true), true); + assertEquals(lib.test_get_boolean(false), false); +}); +``` + +```rust +// tests/napi/src/boolean.rs + +use napi_sys::Status::napi_ok; +use napi_sys::ValueType::napi_boolean; +use napi_sys::*; + +extern "C" fn test_boolean( + env: napi_env, + info: napi_callback_info, +) -> napi_value { + let (args, argc, _) = crate::get_callback_info!(env, info, 1); + assert_eq!(argc, 1); + + let mut ty = -1; + assert!(unsafe { napi_typeof(env, args[0], &mut ty) } == napi_ok); + assert_eq!(ty, napi_boolean); + + // Use napi_get_boolean here... + + value +} + +pub fn init(env: napi_env, exports: napi_value) { + let properties = &[crate::new_property!(env, "test_boolean\0", test_boolean)]; + + unsafe { + napi_define_properties(env, exports, properties.len(), properties.as_ptr()) + }; +} +``` + +```diff +// tests/napi/src/lib.rs + ++ mod boolean; + +... + +#[no_mangle] +unsafe extern "C" fn napi_register_module_v1( + env: napi_env, + exports: napi_value, +) -> napi_value { + ... ++ boolean::init(env, exports); + + exports +} +``` + +Run the test using `cargo test -p tests/napi`. diff --git a/ext/napi/generated_symbol_exports_list_linux.def b/cli/napi/generated_symbol_exports_list_linux.def similarity index 100% rename from ext/napi/generated_symbol_exports_list_linux.def rename to cli/napi/generated_symbol_exports_list_linux.def diff --git a/ext/napi/generated_symbol_exports_list_macos.def b/cli/napi/generated_symbol_exports_list_macos.def similarity index 100% rename from ext/napi/generated_symbol_exports_list_macos.def rename to cli/napi/generated_symbol_exports_list_macos.def diff --git a/ext/napi/generated_symbol_exports_list_windows.def b/cli/napi/generated_symbol_exports_list_windows.def similarity index 100% rename from ext/napi/generated_symbol_exports_list_windows.def rename to cli/napi/generated_symbol_exports_list_windows.def diff --git a/ext/napi/js_native_api.rs b/cli/napi/js_native_api.rs similarity index 99% rename from ext/napi/js_native_api.rs rename to cli/napi/js_native_api.rs index 9301c68ba7..5269d8d1d3 100644 --- a/ext/napi/js_native_api.rs +++ b/cli/napi/js_native_api.rs @@ -5,7 +5,7 @@ const NAPI_VERSION: u32 = 9; -use crate::*; +use deno_runtime::deno_napi::*; use libc::INT_MAX; use super::util::check_new_from_utf8; @@ -17,9 +17,9 @@ use super::util::napi_set_last_error; use super::util::v8_name_from_property_descriptor; use crate::check_arg; use crate::check_env; -use crate::function::create_function; -use crate::function::create_function_template; -use crate::function::CallbackInfo; +use deno_runtime::deno_napi::function::create_function; +use deno_runtime::deno_napi::function::create_function_template; +use deno_runtime::deno_napi::function::CallbackInfo; use napi_sym::napi_sym; use std::ptr::NonNull; @@ -1644,7 +1644,7 @@ fn napi_get_cb_info( check_arg!(env, argc); let argc = unsafe { *argc as usize }; for i in 0..argc { - let arg = args.get(i as _); + let mut arg = args.get(i as _); unsafe { *argv.add(i) = arg.into(); } diff --git a/cli/napi/mod.rs b/cli/napi/mod.rs new file mode 100644 index 0000000000..122d2ff060 --- /dev/null +++ b/cli/napi/mod.rs @@ -0,0 +1,20 @@ +// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. + +#![allow(unused_mut)] +#![allow(non_camel_case_types)] +#![allow(clippy::undocumented_unsafe_blocks)] + +//! Symbols to be exported are now defined in this JSON file. +//! The `#[napi_sym]` macro checks for missing entries and panics. +//! +//! `./tools/napi/generate_symbols_list.js` is used to generate the LINK `cli/exports.def` on Windows, +//! which is also checked into git. +//! +//! To add a new napi function: +//! 1. Place `#[napi_sym]` on top of your implementation. +//! 2. Add the function's identifier to this JSON list. +//! 3. Finally, run `tools/napi/generate_symbols_list.js` to update `cli/napi/generated_symbol_exports_list_*.def`. + +pub mod js_native_api; +pub mod node_api; +pub mod util; diff --git a/ext/napi/node_api.rs b/cli/napi/node_api.rs similarity index 99% rename from ext/napi/node_api.rs rename to cli/napi/node_api.rs index 44e4eae87c..81cb800a7d 100644 --- a/ext/napi/node_api.rs +++ b/cli/napi/node_api.rs @@ -9,10 +9,10 @@ use super::util::napi_set_last_error; use super::util::SendPtr; use crate::check_arg; use crate::check_env; -use crate::*; use deno_core::parking_lot::Condvar; use deno_core::parking_lot::Mutex; use deno_core::V8CrossThreadTaskSpawner; +use deno_runtime::deno_napi::*; use napi_sym::napi_sym; use std::sync::atomic::AtomicBool; use std::sync::atomic::AtomicU8; @@ -892,7 +892,7 @@ fn napi_create_threadsafe_function( }; let resource_name = resource_name.to_rust_string_lossy(&mut env.scope()); - let tsfn = Box::new(TsFn { + let mut tsfn = Box::new(TsFn { env, func, max_queue_size, diff --git a/ext/napi/sym/Cargo.toml b/cli/napi/sym/Cargo.toml similarity index 100% rename from ext/napi/sym/Cargo.toml rename to cli/napi/sym/Cargo.toml diff --git a/ext/napi/sym/README.md b/cli/napi/sym/README.md similarity index 100% rename from ext/napi/sym/README.md rename to cli/napi/sym/README.md diff --git a/ext/napi/sym/lib.rs b/cli/napi/sym/lib.rs similarity index 93% rename from ext/napi/sym/lib.rs rename to cli/napi/sym/lib.rs index 21edaa2896..e2826306b9 100644 --- a/ext/napi/sym/lib.rs +++ b/cli/napi/sym/lib.rs @@ -20,7 +20,7 @@ pub fn napi_sym(_attr: TokenStream, item: TokenStream) -> TokenStream { let name = &func.sig.ident; assert!( exports.symbols.contains(&name.to_string()), - "ext/napi/sym/symbol_exports.json is out of sync!" + "cli/napi/sym/symbol_exports.json is out of sync!" ); TokenStream::from(quote! { diff --git a/ext/napi/sym/symbol_exports.json b/cli/napi/sym/symbol_exports.json similarity index 100% rename from ext/napi/sym/symbol_exports.json rename to cli/napi/sym/symbol_exports.json diff --git a/ext/napi/util.rs b/cli/napi/util.rs similarity index 89% rename from ext/napi/util.rs rename to cli/napi/util.rs index 227b9f8d51..63d8effbf2 100644 --- a/ext/napi/util.rs +++ b/cli/napi/util.rs @@ -1,13 +1,13 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. -use crate::*; +use deno_runtime::deno_napi::*; use libc::INT_MAX; #[repr(transparent)] -pub(crate) struct SendPtr(pub(crate) *const T); +pub struct SendPtr(pub *const T); impl SendPtr { // silly function to get around `clippy::redundant_locals` - pub(crate) fn take(self) -> *const T { + pub fn take(self) -> *const T { self.0 } } @@ -15,9 +15,7 @@ impl SendPtr { unsafe impl Send for SendPtr {} unsafe impl Sync for SendPtr {} -pub(crate) fn get_array_buffer_ptr( - ab: v8::Local, -) -> *mut c_void { +pub fn get_array_buffer_ptr(ab: v8::Local) -> *mut c_void { match ab.data() { Some(p) => p.as_ptr(), None => std::ptr::null_mut(), @@ -39,7 +37,7 @@ impl Drop for BufferFinalizer { } } -pub(crate) extern "C" fn backing_store_deleter_callback( +pub extern "C" fn backing_store_deleter_callback( data: *mut c_void, _byte_length: usize, deleter_data: *mut c_void, @@ -52,7 +50,7 @@ pub(crate) extern "C" fn backing_store_deleter_callback( drop(finalizer); } -pub(crate) fn make_external_backing_store( +pub fn make_external_backing_store( env: *mut Env, data: *mut c_void, byte_length: usize, @@ -92,7 +90,9 @@ macro_rules! check_env { macro_rules! return_error_status_if_false { ($env: expr, $condition: expr, $status: ident) => { if !$condition { - return Err($crate::util::napi_set_last_error($env, $status).into()); + return Err( + $crate::napi::util::napi_set_last_error($env, $status).into(), + ); } }; } @@ -101,7 +101,7 @@ macro_rules! return_error_status_if_false { macro_rules! return_status_if_false { ($env: expr, $condition: expr, $status: ident) => { if !$condition { - return $crate::util::napi_set_last_error($env, $status); + return $crate::napi::util::napi_set_last_error($env, $status); } }; } @@ -222,7 +222,7 @@ macro_rules! check_arg { ($env: expr, $ptr: expr) => { $crate::return_status_if_false!( $env, - !$crate::util::Nullable::is_null(&$ptr), + !$crate::napi::util::Nullable::is_null(&$ptr), napi_invalid_arg ); }; @@ -233,7 +233,6 @@ macro_rules! napi_wrap { ( $( # $attr:tt )* fn $name:ident $( < $( $x:lifetime ),* > )? ( $env:ident : & $( $lt:lifetime )? mut Env $( , $ident:ident : $ty:ty )* $(,)? ) -> napi_status $body:block ) => { $( # $attr )* #[no_mangle] - #[allow(clippy::missing_safety_doc)] pub unsafe extern "C" fn $name $( < $( $x ),* > )? ( env_ptr : *mut Env , $( $ident : $ty ),* ) -> napi_status { let env: & $( $lt )? mut Env = $crate::check_env!(env_ptr); @@ -241,7 +240,7 @@ macro_rules! napi_wrap { return napi_pending_exception; } - $crate::util::napi_clear_last_error(env); + $crate::napi::util::napi_clear_last_error(env); let scope_env = unsafe { &mut *env_ptr }; let scope = &mut scope_env.scope(); @@ -260,11 +259,11 @@ macro_rules! napi_wrap { let env = unsafe { &mut *env_ptr }; let global = v8::Global::new(env.isolate(), exception); env.last_exception = Some(global); - return $crate::util::napi_set_last_error(env_ptr, napi_pending_exception); + return $crate::napi::util::napi_set_last_error(env_ptr, napi_pending_exception); } if result != napi_ok { - return $crate::util::napi_set_last_error(env_ptr, result); + return $crate::napi::util::napi_set_last_error(env_ptr, result); } return result; @@ -274,7 +273,6 @@ macro_rules! napi_wrap { ( $( # $attr:tt )* fn $name:ident $( < $( $x:lifetime ),* > )? ( $( $ident:ident : $ty:ty ),* $(,)? ) -> napi_status $body:block ) => { $( # $attr )* #[no_mangle] - #[allow(clippy::missing_safety_doc)] pub unsafe extern "C" fn $name $( < $( $x ),* > )? ( $( $ident : $ty ),* ) -> napi_status { #[inline(always)] fn inner $( < $( $x ),* > )? ( $( $ident : $ty ),* ) -> napi_status $body diff --git a/ext/napi/Cargo.toml b/ext/napi/Cargo.toml index ef8ab82d54..b00c308899 100644 --- a/ext/napi/Cargo.toml +++ b/ext/napi/Cargo.toml @@ -16,7 +16,4 @@ path = "lib.rs" [dependencies] deno_core.workspace = true deno_permissions.workspace = true -libc.workspace = true libloading = { version = "0.7" } -log.workspace = true -napi_sym.workspace = true diff --git a/ext/napi/README.md b/ext/napi/README.md index 7b359ac6ec..e69de29bb2 100644 --- a/ext/napi/README.md +++ b/ext/napi/README.md @@ -1,114 +0,0 @@ -# napi - -This directory contains source for Deno's Node-API implementation. It depends on -`napi_sym` and `deno_napi`. - -Files are generally organized the same as in Node.js's implementation to ease in -ensuring compatibility. - -## Adding a new function - -Add the symbol name to -[`cli/napi_sym/symbol_exports.json`](../napi_sym/symbol_exports.json). - -```diff -{ - "symbols": [ - ... - "napi_get_undefined", -- "napi_get_null" -+ "napi_get_null", -+ "napi_get_boolean" - ] -} -``` - -Determine where to place the implementation. `napi_get_boolean` is related to JS -values so we will place it in `js_native_api.rs`. If something is not clear, -just create a new file module. - -See [`napi_sym`](../napi_sym/) for writing the implementation: - -```rust -#[napi_sym::napi_sym] -pub fn napi_get_boolean( - env: *mut Env, - value: bool, - result: *mut napi_value, -) -> Result { - // ... - Ok(()) -} -``` - -Update the generated symbol lists using the script: - -``` -deno run --allow-write tools/napi/generate_symbols_lists.js -``` - -Add a test in [`/tests/napi`](../../tests/napi/). You can also refer to Node.js -test suite for Node-API. - -```js -// tests/napi/boolean_test.js -import { assertEquals, loadTestLibrary } from "./common.js"; -const lib = loadTestLibrary(); -Deno.test("napi get boolean", function () { - assertEquals(lib.test_get_boolean(true), true); - assertEquals(lib.test_get_boolean(false), false); -}); -``` - -```rust -// tests/napi/src/boolean.rs - -use napi_sys::Status::napi_ok; -use napi_sys::ValueType::napi_boolean; -use napi_sys::*; - -extern "C" fn test_boolean( - env: napi_env, - info: napi_callback_info, -) -> napi_value { - let (args, argc, _) = crate::get_callback_info!(env, info, 1); - assert_eq!(argc, 1); - - let mut ty = -1; - assert!(unsafe { napi_typeof(env, args[0], &mut ty) } == napi_ok); - assert_eq!(ty, napi_boolean); - - // Use napi_get_boolean here... - - value -} - -pub fn init(env: napi_env, exports: napi_value) { - let properties = &[crate::new_property!(env, "test_boolean\0", test_boolean)]; - - unsafe { - napi_define_properties(env, exports, properties.len(), properties.as_ptr()) - }; -} -``` - -```diff -// tests/napi/src/lib.rs - -+ mod boolean; - -... - -#[no_mangle] -unsafe extern "C" fn napi_register_module_v1( - env: napi_env, - exports: napi_value, -) -> napi_value { - ... -+ boolean::init(env, exports); - - exports -} -``` - -Run the test using `cargo test -p tests/napi`. diff --git a/ext/napi/lib.rs b/ext/napi/lib.rs index 503921634d..8298398389 100644 --- a/ext/napi/lib.rs +++ b/ext/napi/lib.rs @@ -5,21 +5,6 @@ #![allow(clippy::undocumented_unsafe_blocks)] #![deny(clippy::missing_safety_doc)] -//! Symbols to be exported are now defined in this JSON file. -//! The `#[napi_sym]` macro checks for missing entries and panics. -//! -//! `./tools/napi/generate_symbols_list.js` is used to generate the LINK `cli/exports.def` on Windows, -//! which is also checked into git. -//! -//! To add a new napi function: -//! 1. Place `#[napi_sym]` on top of your implementation. -//! 2. Add the function's identifier to this JSON list. -//! 3. Finally, run `tools/napi/generate_symbols_list.js` to update `cli/napi/generated_symbol_exports_list_*.def`. - -pub mod js_native_api; -pub mod node_api; -pub mod util; - use core::ptr::NonNull; use deno_core::error::type_error; use deno_core::error::AnyError; diff --git a/tools/napi/generate_symbols_lists.js b/tools/napi/generate_symbols_lists.js index efb0edc043..11cf1c434a 100755 --- a/tools/napi/generate_symbols_lists.js +++ b/tools/napi/generate_symbols_lists.js @@ -1,7 +1,7 @@ #!/usr/bin/env -S deno run --allow-read --allow-write // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. -import exports from "../../ext/napi/sym/symbol_exports.json" with { +import exports from "../../cli/napi/sym/symbol_exports.json" with { type: "json", }; @@ -17,7 +17,7 @@ const symbolExportLists = { for await (const [os, def] of Object.entries(symbolExportLists)) { const defUrl = new URL( - `../../ext/napi/generated_symbol_exports_list_${os}.def`, + `../../cli/napi/generated_symbol_exports_list_${os}.def`, import.meta.url, ); await Deno.writeTextFile(defUrl.pathname, def, { create: true });