From 947dceb04778c644554aea003a22f164f243153a Mon Sep 17 00:00:00 2001 From: Jeffrey Charles Date: Tue, 5 Nov 2024 12:06:56 -0500 Subject: [PATCH] Allow plugins when building static modules (#813) * Allow plugins when building static modules * Make plugin a `-C` option * Get lint passing * Change check when removing exports --- .github/workflows/ci.yml | 6 +++ .github/workflows/cli-features.yml | 6 +++ Cargo.lock | 8 ++++ Cargo.toml | 1 + Makefile | 6 ++- crates/cli/src/codegen/mod.rs | 51 ++++++++++++++++++----- crates/cli/src/commands.rs | 15 ++++++- crates/cli/src/main.rs | 10 +++-- crates/cli/src/plugins.rs | 20 ++++++++- crates/cli/tests/dynamic_linking_test.rs | 13 +++++- crates/cli/tests/integration_test.rs | 28 ++++++++++++- crates/cli/tests/sample-scripts/plugin.js | 3 ++ crates/runner/.gitignore | 1 + crates/runner/src/lib.rs | 15 +++++++ crates/test-plugin/Cargo.toml | 14 +++++++ crates/test-plugin/src/lib.rs | 19 +++++++++ 16 files changed, 196 insertions(+), 20 deletions(-) create mode 100644 crates/cli/tests/sample-scripts/plugin.js create mode 100644 crates/runner/.gitignore create mode 100644 crates/test-plugin/Cargo.toml create mode 100644 crates/test-plugin/src/lib.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c6a5f8b6..10c2c015 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -73,6 +73,12 @@ jobs: name: plugin path: target/wasm32-wasip1/release/ + - name: Build test-plugin + run: | + cargo build --package=javy-test-plugin --release --target=wasm32-wasip1 + CARGO_PROFILE_RELEASE_LTO=off cargo build --package=javy-cli --release + target/release/javy init-plugin target/wasm32-wasip1/release/test_plugin.wasm -o crates/runner/test_plugin.wasm + - name: Test CLI run: CARGO_PROFILE_RELEASE_LTO=off cargo test --package=javy-cli --release -- --nocapture diff --git a/.github/workflows/cli-features.yml b/.github/workflows/cli-features.yml index 86cae131..98d41e8d 100644 --- a/.github/workflows/cli-features.yml +++ b/.github/workflows/cli-features.yml @@ -17,6 +17,12 @@ jobs: with: os: linux + - name: Build test-plugin + run: | + cargo build --package=javy-test-plugin --release --target=wasm32-wasip1 + CARGO_PROFILE_RELEASE_LTO=off cargo build --package=javy-cli --release + target/release/javy init-plugin target/wasm32-wasip1/release/test_plugin.wasm -o crates/runner/test_plugin.wasm + - name: Test `experimental_event_loop` run: | cargo build --package=javy-plugin --target=wasm32-wasip1 --release --features=experimental_event_loop diff --git a/Cargo.lock b/Cargo.lock index 479283aa..61cb4f48 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1631,6 +1631,14 @@ dependencies = [ "syn 2.0.87", ] +[[package]] +name = "javy-test-plugin" +version = "0.1.0" +dependencies = [ + "anyhow", + "javy-plugin-api", +] + [[package]] name = "jobserver" version = "0.1.31" diff --git a/Cargo.toml b/Cargo.toml index ed664e29..1f51dfcf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -5,6 +5,7 @@ members = [ "crates/plugin", "crates/plugin-api", "crates/test-macros", + "crates/test-plugin", "crates/runner", "fuzz", ] diff --git a/Makefile b/Makefile index 2c644419..fcc36d52 100644 --- a/Makefile +++ b/Makefile @@ -17,6 +17,10 @@ cli: plugin plugin: cargo build --package=javy-plugin --release --target=wasm32-wasip1 --features=$(PLUGIN_FEATURES) +build-test-plugin: cli + cargo build --package=javy-test-plugin --release --target=wasm32-wasip1 + target/release/javy init-plugin target/wasm32-wasip1/release/test_plugin.wasm -o crates/runner/test_plugin.wasm + docs: cargo doc --package=javy-cli --open cargo doc --package=javy-plugin --open --target=wasm32-wasip1 @@ -33,7 +37,7 @@ test-plugin: # Test in release mode to skip some debug assertions # Note: to make this faster, the engine should be optimized beforehand (wasm-strip + wasm-opt). # Disabling LTO substantially improves compile time -test-cli: plugin +test-cli: plugin build-test-plugin CARGO_PROFILE_RELEASE_LTO=off cargo test --package=javy-cli --release --features=$(CLI_FEATURES) -- --nocapture test-runner: diff --git a/crates/cli/src/codegen/mod.rs b/crates/cli/src/codegen/mod.rs index 5fac5e3a..0e26f8e4 100644 --- a/crates/cli/src/codegen/mod.rs +++ b/crates/cli/src/codegen/mod.rs @@ -66,7 +66,7 @@ pub(crate) enum CodeGenType { // This is an internal detail of this module. pub(crate) struct Identifiers { canonical_abi_realloc: FunctionId, - eval_bytecode: FunctionId, + eval_bytecode: Option, invoke: FunctionId, memory: MemoryId, } @@ -74,7 +74,7 @@ pub(crate) struct Identifiers { impl Identifiers { fn new( canonical_abi_realloc: FunctionId, - eval_bytecode: FunctionId, + eval_bytecode: Option, invoke: FunctionId, memory: MemoryId, ) -> Self { @@ -173,7 +173,7 @@ impl Generator { match self.ty { CodeGenType::Static => { let canonical_abi_realloc_fn = module.exports.get_func("canonical_abi_realloc")?; - let eval_bytecode = module.exports.get_func("eval_bytecode")?; + let eval_bytecode = module.exports.get_func("eval_bytecode").ok(); let invoke = module.exports.get_func("invoke")?; let ExportItem::Memory(memory) = module .exports @@ -226,7 +226,7 @@ impl Generator { Ok(Identifiers::new( canonical_abi_realloc_fn_id, - eval_bytecode_fn_id, + Some(eval_bytecode_fn_id), invoke_fn_id, memory_id, )) @@ -247,7 +247,8 @@ impl Generator { let mut main = FunctionBuilder::new(&mut module.types, &[], &[]); let bytecode_ptr_local = module.locals.add(ValType::I32); - main.func_body() + let mut instructions = main.func_body(); + instructions // Allocate memory in plugin instance for bytecode array. .i32_const(0) // orig ptr .i32_const(0) // orig size @@ -259,11 +260,35 @@ impl Generator { .i32_const(0) // offset into data segment for mem.init .i32_const(bytecode_len) // size to copy from data segment // top-2: dest addr, top-1: offset into source, top-0: size of memory region in bytes. - .memory_init(imports.memory, bytecode_data) - // Evaluate bytecode. - .local_get(bytecode_ptr_local) // ptr to bytecode - .i32_const(bytecode_len) - .call(imports.eval_bytecode); + .memory_init(imports.memory, bytecode_data); + // Evaluate top level scope. + if let Some(eval_bytecode) = imports.eval_bytecode { + instructions + .local_get(bytecode_ptr_local) // ptr to bytecode + .i32_const(bytecode_len) + .call(eval_bytecode); + } else { + // Assert we're not emitting a call with a null function to + // invoke for `javy_quickjs_provider_v*`. + // `javy_quickjs_provider_v2` will never support calling `invoke` + // with a null function. Older `javy_quickjs_provider_v3`'s do not + // support being called with a null function. User plugins and + // newer `javy_quickjs_provider_v3`s do support being called with a + // null function. + // Using `assert!` instead of `debug_assert!` because integration + // tests are executed with Javy built with the release profile so + // `debug_assert!`s are stripped out. + assert!( + self.plugin.is_user_plugin(), + "Using invoke with null function only supported for user plugins" + ); + instructions + .local_get(bytecode_ptr_local) // ptr to bytecode + .i32_const(bytecode_len) + .i32_const(0) // set function name ptr to null + .i32_const(0) // set function name len to 0 + .call(imports.invoke); + } let main = main.finish(vec![], &mut module.funcs); module.exports.add("_start", main); @@ -333,7 +358,11 @@ impl Generator { CodeGenType::Static => { // Remove no longer necessary exports. module.exports.remove("canonical_abi_realloc")?; - module.exports.remove("eval_bytecode")?; + // User plugins won't have an `eval_bytecode` function that + // Javy "owns". + if !self.plugin.is_user_plugin() { + module.exports.remove("eval_bytecode")?; + } module.exports.remove("invoke")?; module.exports.remove("compile_src")?; diff --git a/crates/cli/src/commands.rs b/crates/cli/src/commands.rs index 33500603..47cf4b33 100644 --- a/crates/cli/src/commands.rs +++ b/crates/cli/src/commands.rs @@ -1,5 +1,5 @@ use crate::{js_config::JsConfig, option::OptionMeta, option_group, plugins::Plugin}; -use anyhow::{anyhow, Result}; +use anyhow::{anyhow, bail, Result}; use clap::{ builder::{StringValueParser, TypedValueParser, ValueParserFactory}, error::ErrorKind, @@ -179,6 +179,7 @@ pub struct CodegenOptionGroup { pub dynamic: bool, pub wit: WitOptions, pub source_compression: bool, + pub plugin: Option, } impl Default for CodegenOptionGroup { @@ -187,6 +188,7 @@ impl Default for CodegenOptionGroup { dynamic: false, wit: WitOptions::default(), source_compression: true, + plugin: None, } } } @@ -206,6 +208,10 @@ option_group! { /// Enable source code compression, which generates smaller WebAssembly /// files at the cost of increased compile time. SourceCompression(bool), + /// Optional path to Javy plugin Wasm module. Not supported for + /// dynamically linked modules. JavaScript config options are also not + /// supported when using this parameter. + Plugin(PathBuf), } } @@ -223,10 +229,17 @@ impl TryFrom>> for CodegenOptionGroup { CodegenOption::Wit(path) => wit = Some(path), CodegenOption::WitWorld(world) => wit_world = Some(world), CodegenOption::SourceCompression(enabled) => options.source_compression = *enabled, + CodegenOption::Plugin(path) => options.plugin = Some(path.clone()), } } options.wit = WitOptions::from_tuple((wit.cloned(), wit_world.cloned()))?; + + // This is temporary. So I can make the change for dynamic modules + // separately because it will be a breaking change. + if options.dynamic && options.plugin.is_some() { + bail!("Cannot use plugins for building dynamic modules"); + } Ok(options) } } diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 733ae85f..f4bf245f 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -63,12 +63,16 @@ fn main() -> Result<()> { Command::Build(opts) => { let js = JS::from_file(&opts.input)?; let codegen: CodegenOptionGroup = opts.codegen.clone().try_into()?; + let plugin = match codegen.plugin { + Some(path) => Plugin::new_user_plugin(&path)?, + None => Plugin::Default, + }; + let js_opts = JsConfig::from_group_values(&plugin, opts.js.clone())?; let mut builder = CodeGenBuilder::new(); builder .wit_opts(codegen.wit) - .source_compression(codegen.source_compression); - - let js_opts = JsConfig::from_group_values(&Plugin::Default, opts.js.clone())?; + .source_compression(codegen.source_compression) + .plugin(plugin); let mut gen = if codegen.dynamic { builder.build(CodeGenType::Dynamic, js_opts)? } else { diff --git a/crates/cli/src/plugins.rs b/crates/cli/src/plugins.rs index a54306ac..c94d002c 100644 --- a/crates/cli/src/plugins.rs +++ b/crates/cli/src/plugins.rs @@ -4,6 +4,7 @@ use serde::Deserialize; use std::{ fs, io::{Read, Seek}, + path::Path, str, }; use walrus::{ExportItem, ValType}; @@ -33,6 +34,8 @@ pub enum Plugin { Default, /// A plugin for use with the `compile` to maintain backward compatibility. V2, + /// A plugin provided by the user. + User { bytes: Vec }, } impl Default for Plugin { @@ -42,11 +45,24 @@ impl Default for Plugin { } impl Plugin { + /// Creates a new user plugin. + pub fn new_user_plugin(path: &Path) -> Result { + Ok(Self::User { + bytes: fs::read(path)?, + }) + } + + /// Returns true if the plugin is a user plugin. + pub fn is_user_plugin(&self) -> bool { + matches!(&self, Plugin::User { .. }) + } + /// Returns the plugin Wasm module as a byte slice. pub fn as_bytes(&self) -> &[u8] { match self { Self::Default => PLUGIN_MODULE, Self::V2 => QUICKJS_PROVIDER_V2_MODULE, + Self::User { bytes } => bytes, } } @@ -59,7 +75,7 @@ impl Plugin { pub fn import_namespace(&self) -> Result { match self { Self::V2 => Ok("javy_quickjs_provider_v2".to_string()), - Self::Default => { + Self::Default | Self::User { .. } => { let module = walrus::Module::from_buffer(self.as_bytes())?; let import_namespace = module .customs @@ -81,7 +97,7 @@ impl Plugin { /// The JS configuration properties supported by this plugin. pub fn config_schema(&self) -> Result> { match self { - Self::V2 => Ok(vec![]), + Self::V2 | Self::User { .. } => Ok(vec![]), Self::Default => { let engine = Engine::default(); let module = wasmtime::Module::new(&engine, self.as_bytes())?; diff --git a/crates/cli/tests/dynamic_linking_test.rs b/crates/cli/tests/dynamic_linking_test.rs index 57376060..5d975a09 100644 --- a/crates/cli/tests/dynamic_linking_test.rs +++ b/crates/cli/tests/dynamic_linking_test.rs @@ -1,5 +1,5 @@ use anyhow::Result; -use javy_runner::Builder; +use javy_runner::{Builder, Plugin}; use javy_test_macros::javy_cli_test; #[javy_cli_test(dyn = true, root = "tests/dynamic-linking-scripts")] @@ -129,3 +129,14 @@ fn javy_json_identity(builder: &mut Builder) -> Result<()> { Ok(()) } + +#[javy_cli_test(dyn = true, commands(not(Compile)))] +fn test_using_plugin_with_dynamic_build_fails(builder: &mut Builder) -> Result<()> { + let result = builder.plugin(Plugin::User).input("plugin.js").build(); + let err = result.err().unwrap(); + assert!(err + .to_string() + .contains("Cannot use plugins for building dynamic modules")); + + Ok(()) +} diff --git a/crates/cli/tests/integration_test.rs b/crates/cli/tests/integration_test.rs index 698f060f..b9d2c028 100644 --- a/crates/cli/tests/integration_test.rs +++ b/crates/cli/tests/integration_test.rs @@ -1,5 +1,5 @@ use anyhow::{bail, Result}; -use javy_runner::{Builder, Runner, RunnerError}; +use javy_runner::{Builder, Plugin, Runner, RunnerError}; use std::{path::PathBuf, process::Command, str}; use wasi_common::sync::WasiCtxBuilder; use wasmtime::{AsContextMut, Engine, Linker, Module, Store}; @@ -136,6 +136,32 @@ fn test_javy_json_disabled(builder: &mut Builder) -> Result<()> { Ok(()) } +#[javy_cli_test(commands(not(Compile)))] +fn test_using_plugin_with_static_build(builder: &mut Builder) -> Result<()> { + let mut runner = builder.plugin(Plugin::User).input("plugin.js").build()?; + + let result = runner.exec(&[]); + assert!(result.is_ok()); + + Ok(()) +} + +#[javy_cli_test(commands(not(Compile)))] +fn test_using_plugin_with_static_build_fails_with_runtime_config( + builder: &mut Builder, +) -> Result<()> { + let result = builder + .plugin(Plugin::User) + .simd_json_builtins(true) + .build(); + let err = result.err().unwrap(); + assert!(err + .to_string() + .contains("Property simd-json-builtins is not supported for runtime configuration")); + + Ok(()) +} + #[javy_cli_test] fn test_readme_script(builder: &mut Builder) -> Result<()> { let mut runner = builder.input("readme.js").build()?; diff --git a/crates/cli/tests/sample-scripts/plugin.js b/crates/cli/tests/sample-scripts/plugin.js new file mode 100644 index 00000000..bfb21a34 --- /dev/null +++ b/crates/cli/tests/sample-scripts/plugin.js @@ -0,0 +1,3 @@ +if (plugin !== true) { + throw new Error("Not using the test_plugin"); +} diff --git a/crates/runner/.gitignore b/crates/runner/.gitignore new file mode 100644 index 00000000..2cfbeb78 --- /dev/null +++ b/crates/runner/.gitignore @@ -0,0 +1 @@ +test_plugin.wasm diff --git a/crates/runner/src/lib.rs b/crates/runner/src/lib.rs index c79ce4ad..8c7251ec 100644 --- a/crates/runner/src/lib.rs +++ b/crates/runner/src/lib.rs @@ -22,6 +22,7 @@ pub enum JavyCommand { pub enum Plugin { V2, Default, + User, } impl Plugin { @@ -31,6 +32,7 @@ impl Plugin { // Could try and derive this but not going to for now since tests // will break if it changes. Self::Default => "javy_quickjs_provider_v3", + Self::User { .. } => "test_plugin", } } } @@ -205,6 +207,7 @@ impl Builder { simd_json_builtins, text_encoding, preload, + plugin, ), } } @@ -280,6 +283,7 @@ impl Runner { override_json_parse_and_stringify: Option, text_encoding: Option, preload: Option<(String, PathBuf)>, + plugin: Plugin, ) -> Result { // This directory is unique and will automatically get deleted // when `tempdir` goes out of scope. @@ -299,6 +303,7 @@ impl Runner { &javy_stream_io, &override_json_parse_and_stringify, &text_encoding, + &plugin, ); Self::exec_command(bin, root, args)?; @@ -527,6 +532,7 @@ impl Runner { javy_stream_io: &Option, simd_json_builtins: &Option, text_encoding: &Option, + plugin: &Plugin, ) -> Vec { let mut args = vec![ "build".to_string(), @@ -581,6 +587,11 @@ impl Runner { args.push(format!("text-encoding={}", if enabled { "y" } else { "n" })); } + if let Plugin::User = plugin { + args.push("-C".to_string()); + args.push(format!("plugin={}", Self::plugin_path().to_str().unwrap())); + } + args } @@ -804,6 +815,10 @@ impl Runner { .into()), } } + + fn plugin_path() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("test_plugin.wasm") + } } #[derive(Debug)] diff --git a/crates/test-plugin/Cargo.toml b/crates/test-plugin/Cargo.toml new file mode 100644 index 00000000..2f82247b --- /dev/null +++ b/crates/test-plugin/Cargo.toml @@ -0,0 +1,14 @@ +[package] +name = "javy-test-plugin" +version = "0.1.0" +authors.workspace = true +edition.workspace = true +license.workspace = true + +[lib] +name = "test_plugin" +crate-type = ["cdylib"] + +[dependencies] +anyhow = { workspace = true } +javy-plugin-api = { path = "../plugin-api", features = ["json"] } diff --git a/crates/test-plugin/src/lib.rs b/crates/test-plugin/src/lib.rs new file mode 100644 index 00000000..4988b26a --- /dev/null +++ b/crates/test-plugin/src/lib.rs @@ -0,0 +1,19 @@ +//! Plugin used for testing. We need a plugin with slightly different behavior +//! to validate a plugin is actually used when it should be. + +use javy_plugin_api::{import_namespace, javy::Config}; + +import_namespace!("test_plugin"); + +#[export_name = "initialize_runtime"] +pub extern "C" fn initialize_runtime() { + let config = Config::default(); + javy_plugin_api::initialize_runtime(config, |runtime| { + runtime + .context() + .with(|ctx| ctx.globals().set("plugin", true)) + .unwrap(); + runtime + }) + .unwrap(); +}