Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow plugins when building static modules #813

Merged
merged 4 commits into from
Nov 5, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/cli-features.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 8 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ members = [
"crates/plugin",
"crates/plugin-api",
"crates/test-macros",
"crates/test-plugin",
"crates/runner",
"fuzz",
]
Expand Down
6 changes: 5 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
Expand Down
5 changes: 5 additions & 0 deletions crates/cli/src/codegen/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,11 @@ impl CodeGenBuilder {
if js_runtime_config.has_configs() {
bail!("Cannot set JS runtime options when building a dynamic module")
}
// This is temporary. So I can make the change for dynamic modules
// separately because it will be a breaking change.
if let Plugin::User { .. } = self.plugin {
bail!("Cannot use plugins for building dynamic modules")
}
}
let mut generator = Generator::new(ty, js_runtime_config, self.plugin);
generator.source_compression = self.source_compression;
Expand Down
51 changes: 40 additions & 11 deletions crates/cli/src/codegen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,15 +66,15 @@ 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<FunctionId>,
invoke: FunctionId,
memory: MemoryId,
}

impl Identifiers {
fn new(
canonical_abi_realloc: FunctionId,
eval_bytecode: FunctionId,
eval_bytecode: Option<FunctionId>,
invoke: FunctionId,
memory: MemoryId,
) -> Self {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
))
Expand All @@ -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
Expand All @@ -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!(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My plan is to relax this assertion a bit after we have released a new version of javy_quickjs_provider_v3.

matches!(self.plugin, Plugin::User { .. }),
"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);
Expand Down Expand Up @@ -333,7 +358,11 @@ impl Generator {
CodeGenType::Static => {
// Remove no longer necessary exports.
module.exports.remove("canonical_abi_realloc")?;
module.exports.remove("eval_bytecode")?;
// `eval_bytecode` is not present in user plugins and `remove`
// returns an error if the export is not present.
if module.exports.get_func("eval_bytecode").is_ok() {
module.exports.remove("eval_bytecode")?;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true because we know the plugin API surface, however, I don't think anything is preventing a plugin from exporting a function named eval_bytecode for their own purposes. Probably unlikely, but not impossible. I'd suggest checking if the plugin is not a user provided plugin before removing this export.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the check

module.exports.remove("invoke")?;
module.exports.remove("compile_src")?;

Expand Down
4 changes: 4 additions & 0 deletions crates/cli/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@ pub struct BuildCommandOpts {
/// JavaScript runtime options.
/// Use `-J help` for more details.
pub js: Vec<JsGroupValue>,

#[arg(short, long)]
/// Path of the plugin.
pub plugin: Option<PathBuf>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why the plugin flag is not scoped? In the What are the changes? section of the RFC, the plugin handling is scoped under the -C option group, which is what I think makes most sense, since it's easier to validate the semantic meaning of the group as a whole (e.g., -C pluing=.. is required with -C dynamic)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was an oversight on my part

}

#[derive(Debug, Parser)]
Expand Down
10 changes: 7 additions & 3 deletions crates/cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 &opts.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 {
Expand Down
14 changes: 12 additions & 2 deletions crates/cli/src/plugins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use serde::Deserialize;
use std::{
fs,
io::{Read, Seek},
path::Path,
str,
};
use walrus::{ExportItem, ValType};
Expand Down Expand Up @@ -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<u8> },
}

impl Default for Plugin {
Expand All @@ -42,11 +45,18 @@ impl Default for Plugin {
}

impl Plugin {
pub fn new_user_plugin(path: &Path) -> Result<Self> {
Ok(Self::User {
bytes: fs::read(path)?,
})
}

/// 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,
}
}

Expand All @@ -59,7 +69,7 @@ impl Plugin {
pub fn import_namespace(&self) -> Result<String> {
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
Expand All @@ -81,7 +91,7 @@ impl Plugin {
/// The JS configuration properties supported by this plugin.
pub fn config_schema(&self) -> Result<Vec<JsConfigProperty>> {
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())?;
Expand Down
13 changes: 12 additions & 1 deletion crates/cli/tests/dynamic_linking_test.rs
Original file line number Diff line number Diff line change
@@ -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")]
Expand Down Expand Up @@ -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(())
}
28 changes: 27 additions & 1 deletion crates/cli/tests/integration_test.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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()?;
Expand Down
3 changes: 3 additions & 0 deletions crates/cli/tests/sample-scripts/plugin.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
if (plugin !== true) {
throw new Error("Not using the test_plugin");
}
1 change: 1 addition & 0 deletions crates/runner/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
test_plugin.wasm
Loading
Loading