Merge pull request #9 from Brooooooklyn/build-enhance

build: enhance the build pipeline

.github/workflows/linux.yaml

@@ -65,9 +65,9 @@ jobs:
       
       - name: test scripts
         run: |
+          yarn
           cd test_module
-          cargo build
-          cp target/debug/libtest_module.so target/debug/libtest_module.node
+          yarn build
           node tests.js
 
       - name: Clear the cargo caches

.github/workflows/macos.yaml

@@ -65,9 +65,9 @@ jobs:
 
       - name: test scripts
         run: |
+          yarn
           cd test_module
-          cargo build
-          cp target/debug/libtest_module.dylib target/debug/libtest_module.node
+          yarn build
           node tests.js
 
       - name: Clear the cargo caches

.github/workflows/windows.yaml

@@ -71,10 +71,12 @@ jobs:
       
       - name: test scripts
         run: |
+          yarn
           cd test_module
-          cargo build
-          cp target/debug/test_module.dll target/debug/libtest_module.node
+          yarn build
           node tests.js
+        env:
+          RUST_BACKTRACE: 1
 
       - name: Clear the cargo caches
         run: |

FUNDING.yml

@@ -0,0 +1 @@
+github: [Brooooooklyn]

README.md

@@ -28,40 +28,71 @@ One nice feature is that this crate allows you to build add-ons purely with the
 
 ## Building
 
-This repository is a Cargo crate *and* an npm module. Any napi-based add-on should also contain *both* `Cargo.toml` to make it a Cargo crate and a `package.json` to make it an npm module.
+This repository is a Cargo crate. Any napi-based add-on should contain `Cargo.toml` to make it a Cargo crate.
 
-In your `Cargo.toml` you need to set the `crate-type` to `"cdylib"` so that cargo builds a C-style shared library that can be dynamically loaded by the Node executable. You'll also want to add this crate as a dependency.
+In your `Cargo.toml` you need to set the `crate-type` to `"cdylib"` so that cargo builds a C-style shared library that can be dynamically loaded by the Node executable. You'll also need to add this crate as a dependency.
 
-```
+```toml
 [lib]
 crate-type = ["cdylib"]
+
+[dependencies]
+napi-rs = "0.1"
+
+[build-dependencies]
+napi-build = "0.1"
 ```
 
-Building napi-based add-ons directly with `cargo build` isn't recommended, because you'll need to provide a `NODE_INCLUDE_PATH` pointing to the `include` directory for the version of Node you're targeting, as well as some special linker flags that can't be specified in the Cargo configuration.
+And create `build.rs` in your own project:
 
-Instead, you'll want to use the `napi` script, which will be installed automatically at `node_modules/.bin/napi` if you include `napi` as a dependency in your add-on's `package.json`. The napi script supports the following subcommands.
+```rs
+// build.rs
+extern crate napi_build;
 
-* `napi build [--debug]` Runs `cargo build` with a `NODE_INCLUDE_PATH` based on the path of the Node executable used to run the script and the required linker flags. The optional `--debug` flag will build in debug mode. After building, the script renames the dynamic library to have the `.node` extension to match the convention in the Node.js ecosystem.
-* `napi check` Runs `cargo check` with a `NODE_INCLUDE_PATH` based on the Node executable used to run the script.
+fn main() {
+  napi_build::setup();
+}
+```
 
-The `napi` script will be available on the `PATH` of any scripts you define in the `scripts` section of your `package.json`, enabling a setup like this:
+So far, the `napi` build script has only been tested on `macOS` `Linux` and `Windows x64 MSVC`. 
+
+See the included [test_module](./test_module) for an example add-on.
+
+Run `cargo build` to produce the `Dynamic lib` file. And install the `napi-rs` to help you copy `Dynamic lib` file to `.node` file in case you can `require` it in your program.
 
 ```json
+
 {
-  "name": "my-add-on",
-  "version": "1.0.0",
-  "scripts": {
-    "build": "napi build",
-    "build-debug": "napi build --debug",
-    "check": "napi check"
-  },
+  "package": "your pkg",
   "dependencies": {
-    "napi": "*"
+    "napi-rs": "^0.1"
+  },
+  "scripts": {
+    "build": "cargo build && napi",
+    "build-release": "cargo build --release && napi --release"
   }
 }
 ```
 
-So far, the `napi` build script has only been tested on macOS. See the included `test_module` for an example add-on.
+Then you can require your native binding:
+
+```js
+require('./target/debug|release/[module_name].node')
+```
+
+The `module_name` would be your `package` name in your `Cargo.toml`.
+
+`xxx => ./target/debug|release/xxx.node`
+
+`xxx-yyy => ./target/debug|release/xxx_yyy.node`
+
+You can also copy `Dynamic lib` file to an appointed location:
+
+```bash
+napi [--release] .
+napi [--release] ./mylib
+napi [--release] ./mylib.node
+```
 
 ## Testing
 

napi/build.rs

@@ -12,53 +12,13 @@ extern crate tar;
 
 use glob::glob;
 
-use std::borrow::Cow;
 use std::env;
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
 use std::process::Command;
 
 // https://stackoverflow.com/questions/37498864/finding-executable-in-path-with-rust
 
-#[cfg(not(target_os = "windows"))]
-fn enhance_exe_name(exe_name: &Path) -> Cow<Path> {
-  exe_name.into()
-}
-
-#[cfg(target_os = "windows")]
-fn enhance_exe_name(exe_name: &Path) -> Cow<Path> {
-  use std::ffi::OsStr;
-  use std::os::windows::ffi::OsStrExt;
-
-  let raw_input: Vec<_> = exe_name.as_os_str().encode_wide().collect();
-  let raw_extension: Vec<_> = OsStr::new(".exe").encode_wide().collect();
-
-  if raw_input.ends_with(&raw_extension) {
-    exe_name.into()
-  } else {
-    let mut with_exe = exe_name.as_os_str().to_owned();
-    with_exe.push(".exe");
-    PathBuf::from(with_exe).into()
-  }
-}
-
-fn find_it<P>(exe_name: P) -> Option<PathBuf>
-where
-  P: AsRef<Path>,
-{
-  let exe_name = enhance_exe_name(exe_name.as_ref());
-  env::var_os("PATH").and_then(|paths| {
-    env::split_paths(&paths)
-      .filter_map(|dir| {
-        let full_path = dir.join(&exe_name);
-        if full_path.is_file() {
-          Some(full_path)
-        } else {
-          None
-        }
-      })
-      .next()
-  })
-}
+const NODE_PRINT_EXEC_PATH: &'static str = "console.log(process.execPath)";
 
 fn main() {
   napi_build::setup();
@@ -130,11 +90,19 @@ fn main() {
 #[cfg(target_os = "windows")]
 fn find_node_include_path(node_full_version: &str) -> PathBuf {
   let mut node_exec_path = PathBuf::from(
-    find_it("node")
-      .expect("can not find executable node")
-      .parent()
-      .unwrap(),
-  );
+    String::from_utf8(
+      Command::new("node")
+        .arg("-e")
+        .arg(NODE_PRINT_EXEC_PATH)
+        .output()
+        .unwrap()
+        .stdout,
+    )
+    .expect("can not find executable node"),
+  )
+  .parent()
+  .unwrap()
+  .to_path_buf();
   node_exec_path.push(format!("node-headers-{}.tar.gz", node_full_version));
   let mut header_dist_path = PathBuf::from(&PathBuf::from(&node_exec_path).parent().unwrap());
   let unpack_path = PathBuf::from(&header_dist_path);
@@ -160,8 +128,16 @@ fn find_node_include_path(node_full_version: &str) -> PathBuf {
 
 #[cfg(not(target_os = "windows"))]
 fn find_node_include_path(_node_full_version: &str) -> PathBuf {
-  let node_exec_path = find_it("node").expect("can not find executable node");
-  node_exec_path
+  let node_exec_path = String::from_utf8(
+    Command::new("node")
+      .arg("-e")
+      .arg(NODE_PRINT_EXEC_PATH)
+      .output()
+      .unwrap()
+      .stdout,
+  )
+  .unwrap();
+  PathBuf::from(node_exec_path)
     .parent()
     .unwrap()
     .parent()

napi/src/executor.rs

@@ -4,6 +4,7 @@ use std::future::Future;
 use std::mem;
 use std::os::raw::c_void;
 use std::pin::Pin;
+use std::sync::atomic::{AtomicBool, Ordering};
 use std::task::{Context, RawWaker, RawWakerVTable, Waker};
 
 pub struct LibuvExecutor {
@@ -45,6 +46,7 @@ unsafe fn drop_uv_async(uv_async_t_ptr: *const ()) {
 struct Task<'a> {
   future: Pin<Box<dyn Future<Output = ()>>>,
   context: Context<'a>,
+  resolved: AtomicBool,
 }
 
 impl LibuvExecutor {
@@ -69,6 +71,7 @@ impl LibuvExecutor {
       let task = Box::leak(Box::new(Task {
         future: Box::pin(future),
         context,
+        resolved: AtomicBool::new(false),
       }));
       sys::uv_handle_set_data(
         uv_async_t_ref as *mut _ as *mut sys::uv_handle_t,
@@ -81,8 +84,14 @@ impl LibuvExecutor {
 
 impl<'a> Task<'a> {
   fn poll_future(&mut self) -> bool {
+    if self.resolved.load(Ordering::Relaxed) {
+      return true;
+    }
     match self.future.as_mut().poll(&mut self.context) {
-      Poll::Ready(_) => true,
+      Poll::Ready(_) => {
+        while !self.resolved.swap(true, Ordering::Relaxed) {}
+        true
+      }
       Poll::Pending => false,
     }
   }

napi/src/lib.rs

@@ -190,7 +190,9 @@ macro_rules! callback {
         Ok(Some(result)) => result.into_raw(),
         Ok(None) => env.get_undefined().into_raw(),
         Err(e) => {
-          let _ = writeln!(::std::io::stderr(), "Error calling function: {:?}", e);
+          if !cfg!(windows) {
+            let _ = writeln!(::std::io::stderr(), "Error calling function: {:?}", e);
+          }
           let message = format!("{:?}", e);
           unsafe {
             $crate::sys::napi_throw_error(raw_env, ptr::null(), message.as_ptr() as *const c_char);

package.json

@@ -22,7 +22,8 @@
   },
   "homepage": "https://github.com/Brooooooklyn/napi-rs#readme",
   "dependencies": {
-    "minimist": "^1.2.0"
+    "minimist": "^1.2.0",
+    "toml": "^3.0.0"
   },
   "prettier": {
     "printWidth": 80,

scripts/napi.js

@@ -1,41 +1,54 @@
 #!/usr/bin/env node
 
 const parseArgs = require('minimist')
-const cp = require('child_process')
 const path = require('path')
 const os = require('os')
-const parsedNodeVersion = process.versions.node.match(/^(\d+)\.(\d+)\.(\d+)$/)
-const nodeMajorVersion = parseInt(parsedNodeVersion[1])
+const toml = require('toml')
+const fs = require('fs')
 
-if (nodeMajorVersion < 10) {
-  console.error('This build script should be run on Node 10 or greater')
-  process.exit(1)
+
+let tomlContentString
+let tomlContent
+let moduleName
+
+try {
+  tomlContentString = fs.readFileSync(path.join(process.cwd(), 'Cargo.toml'), 'utf-8')
+} catch {
+  throw new TypeError('Can not find Cargo.toml in process.cwd')
+}
+
+try {
+  tomlContent = toml.parse(tomlContentString)
+} catch {
+  throw new TypeError('Can not parse the Cargo.toml')
+}
+
+if (tomlContent.package && tomlContent.package.name) {
+  moduleName = tomlContent.package.name.replace(/-/g, '_')
+} else {
+  throw new TypeError('No package.name field in Cargo.toml')
 }
 
 const argv = parseArgs(process.argv.slice(2), {
   boolean: ['release'],
 })
 
-const subcommand = argv._[0] || 'build'
-
-const moduleName = path.basename(process.cwd()).replace(/-/g, '_')
-
 const platform = os.platform()
-let libExt, platformArgs
+let libExt
+let dylibName = moduleName
 
 // Platform based massaging for build commands
 switch (platform) {
   case 'darwin':
     libExt = '.dylib'
-    platformArgs = '-undefined dynamic_lookup -export_dynamic'
+    dylibName = `lib${moduleName}`
     break
   case 'win32':
     libExt = '.dll'
-    platformArgs = '-undefined dynamic_lookup -export_dynamic'
     break
   case 'linux':
+    dylibName = `lib${moduleName}`
     libExt = '.so'
-    platformArgs = '-undefined=dynamic_lookup -export_dynamic'
     break
   default:
     console.error(
@@ -44,27 +57,28 @@ switch (platform) {
     process.exit(1)
 }
 
-switch (subcommand) {
-  case 'build':
-    const releaseFlag = argv.release ? '--release' : ''
-    const targetDir = argv.release ? 'release' : 'debug'
-    cp.execSync(
-      `cargo rustc ${releaseFlag} -- -Clink-args=\"${platformArgs}\"`,
-      { stdio: 'inherit' },
-    )
-    cp.execSync(`mkdir -p target/${targetDir}`)
-    cp.execSync(
-      `cp ${path.join(
-        process.cwd(),
-        'target',
-        targetDir,
-        'lib' + moduleName + libExt,
-      )}  target/${targetDir}/${moduleName}.node`,
-      { stdio: 'inherit' },
-    )
-    break
-  case 'check':
-    cp.execSync(`cargo check`, { stdio: 'inherit' })
-  case 'doc':
-    cp.execSync(`cargo doc`, { stdio: 'inherit' })
+const targetDir = argv.release ? 'release' : 'debug'
+
+let subcommand = argv._[0] || path.join('target', targetDir, `${moduleName}.node`)
+const parsedDist = path.parse(subcommand)
+
+if (parsedDist.ext && parsedDist.ext !== '.node') {
+  throw new TypeError('Dist file must be end with .node extension')
 }
+
+if (!parsedDist.name || parsedDist.name === '.') {
+  subcommand = moduleName
+}
+
+if (!parsedDist.ext) {
+  subcommand = `${subcommand}.node`
+}
+
+const dylibContent = fs.readFileSync(path.join(
+  process.cwd(),
+  'target',
+  targetDir,
+  `${dylibName}${libExt}`,
+))
+
+fs.writeFileSync(subcommand, dylibContent)

test_module/index.js

@@ -0,0 +1,27 @@
+const testModule = require(`./target/debug/test_module.node`)
+
+function testSpawn() {
+  console.log('=== Test spawning a future on libuv event loop')
+  return testModule.testSpawn()
+}
+
+function testThrow() {
+  console.log('=== Test throwing from Rust')
+  try {
+    testModule.testThrow()
+  } catch (e) {
+    return
+  }
+  console.error('Expected function to throw an error')
+  process.exit(1)
+}
+
+const future = testSpawn()
+
+// https://github.com/nodejs/node/issues/29355
+setTimeout(() => {
+  future.then(testThrow).catch((e) => {
+    console.error(e)
+    process.exit(1)
+  })
+}, 1000)

test_module/package.json

@@ -2,12 +2,8 @@
   "name": "test-module",
   "version": "1.0.0",
   "scripts": {
-    "build": "../scripts/napi.js build",
-    "build-release": "../scripts/napi.js build --release",
-    "check": "../scripts/napi.js check",
+    "build": "cargo build && node ../scripts/napi.js",
+    "build-release": "cargo build --release && node ../scripts/napi.js --release",
     "test": "node ./tests.js"
-  },
-  "dependencies": {
-    "napi-rs": "^0.1.0"
   }
 }

test_module/src/lib.rs

@@ -41,7 +41,9 @@ fn test_spawn<'a>(
     pool.spawn_ok(fut_tx_result);
     let fut_values = rx.map(|v| v * 2).collect::<Vec<i32>>();
     let results = fut_values.await;
-    println!("Collected result lenght {}", results.len());
+    if !cfg!(windows) {
+      println!("Collected result lenght {}", results.len());
+    };
     async_context.enter(|env| {
       env.resolve_deferred(deferred, env.get_undefined());
     });

test_module/tests.js

@@ -1,27 +1,14 @@
-const testModule = require(`./target/debug/libtest_module.node`)
+const { platform } = require('os')
+const { fork } = require('child_process')
 
-function testSpawn() {
-  console.log('=== Test spawning a future on libuv event loop')
-  return testModule.testSpawn()
-}
-
-function testThrow() {
-  console.log('=== Test throwing from Rust')
-  try {
-    testModule.testThrow()
-  } catch (e) {
-    return
+fork('./index.js', {
+  stdio: 'inherit',
+}).on('exit', (code) => {
+  if (code !== 0) {
+    if (code === 3221225477 && platform() === 'win32') {
+      console.error(code)
+      process.exit(0)
+    }
+    process.exit(code)
   }
-  console.error('Expected function to throw an error')
-  process.exit(1)
-}
-
-const future = testSpawn()
-
-// https://github.com/nodejs/node/issues/29355
-setTimeout(() => {
-  future.then(testThrow).catch((e) => {
-    console.error(e)
-    process.exit(1)
-  })
-}, 10)
+})

yarn.lock

@@ -16,3 +16,8 @@ prettier@^1.12.1:
   version "1.19.1"
   resolved "https://registry.npmjs.org/prettier/-/prettier-1.19.1.tgz#f7d7f5ff8a9cd872a7be4ca142095956a60797cb"
   integrity sha512-s7PoyDv/II1ObgQunCbB9PdLmUcBZcnWOcxDh7O0N/UwDEsHyqkW+Qh28jW+mVuCdx7gLB0BotYI1Y6uI9iyew==
+
+toml@^3.0.0:
+  version "3.0.0"
+  resolved "https://registry.npmjs.org/toml/-/toml-3.0.0.tgz#342160f1af1904ec9d204d03a5d61222d762c5ee"
+  integrity sha512-y/mWCZinnvxjTKYhJ+pYxwD0mRLVvOtdS2Awbgxln6iEnt4rk0yBxeSBHkGJcPucRiG0e55mwWp+g/05rsrd6w==