From c91eca779850fc2ace8ba9319b9900f50196f34d Mon Sep 17 00:00:00 2001 From: Jeff Charles Date: Fri, 22 Nov 2024 14:43:31 -0500 Subject: [PATCH] Make event loop a runtime config instead of a Cargo feature --- .github/workflows/cli-features.yml | 29 ----------------------- .github/workflows/wpt.yml | 2 +- Cargo.lock | 2 +- Makefile | 6 +---- crates/cli/Cargo.toml | 1 - crates/cli/tests/integration_test.rs | 25 +++++++++++--------- crates/plugin-api/CHANGELOG.md | 4 ++++ crates/plugin-api/Cargo.toml | 3 +-- crates/plugin-api/README.md | 2 +- crates/plugin-api/src/config.rs | 32 ++++++++++++++++++++++++++ crates/plugin-api/src/lib.rs | 18 ++++++++------- crates/plugin/Cargo.toml | 3 --- crates/plugin/src/lib.rs | 3 +-- crates/plugin/src/shared_config/mod.rs | 8 ++++++- crates/runner/src/lib.rs | 21 +++++++++++++++++ crates/test-plugin/src/lib.rs | 2 +- docs/docs-contributing-architecture.md | 11 ++------- docs/docs-using-extending.md | 5 ++-- wpt/README.md | 1 - wpt/package.json | 2 +- 20 files changed, 100 insertions(+), 80 deletions(-) delete mode 100644 .github/workflows/cli-features.yml create mode 100644 crates/plugin-api/src/config.rs diff --git a/.github/workflows/cli-features.yml b/.github/workflows/cli-features.yml deleted file mode 100644 index b2f25d6a..00000000 --- a/.github/workflows/cli-features.yml +++ /dev/null @@ -1,29 +0,0 @@ -# Tests extra CLI features and their dependency with plugin features. -name: Test CLI Features -on: - push: - branches: - - main - pull_request: - -jobs: - cli: - name: Test CLI Features - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - - uses: ./.github/actions/ci-shared-setup - - - name: Build test-plugin - # Need to build Javy default plugin and Javy CLI to run `javy init-plugin` on the test plugin. - run: | - cargo build --package=javy-plugin --release --target=wasm32-wasip1 - CARGO_PROFILE_RELEASE_LTO=off cargo build --package=javy-cli --release - 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 - - - name: Test `experimental_event_loop` - run: | - cargo build --package=javy-plugin --target=wasm32-wasip1 --release --features=experimental_event_loop - cargo test --package=javy-cli --features=experimental_event_loop --release -- --nocapture diff --git a/.github/workflows/wpt.yml b/.github/workflows/wpt.yml index b126bcc2..bcc8c6a7 100644 --- a/.github/workflows/wpt.yml +++ b/.github/workflows/wpt.yml @@ -19,7 +19,7 @@ jobs: - name: WPT run: | - cargo build --package=javy-plugin --release --target=wasm32-wasip1 --features=experimental_event_loop + cargo build --package=javy-plugin --release --target=wasm32-wasip1 CARGO_PROFILE_RELEASE_LTO=off cargo build --package=javy-cli --release npm install --prefix wpt npm test --prefix wpt diff --git a/Cargo.lock b/Cargo.lock index 77c2b1d0..8c89b322 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1602,7 +1602,7 @@ dependencies = [ [[package]] name = "javy-plugin-api" -version = "1.0.1-alpha.1" +version = "2.0.0-alpha.1" dependencies = [ "anyhow", "javy", diff --git a/Makefile b/Makefile index fcc36d52..124d1a08 100644 --- a/Makefile +++ b/Makefile @@ -43,11 +43,7 @@ test-cli: plugin build-test-plugin test-runner: cargo test --package=javy-runner -- --nocapture -# WPT requires a Javy build with the experimental_event_loop feature to pass -test-wpt: export PLUGIN_FEATURES ?= experimental_event_loop -test-wpt: -# Can't use a prerequisite here b/c a prequisite will not cause a rebuild of the CLI - $(MAKE) cli +test-wpt: cli npm install --prefix wpt npm test --prefix wpt diff --git a/crates/cli/Cargo.toml b/crates/cli/Cargo.toml index 97331902..de2e0251 100644 --- a/crates/cli/Cargo.toml +++ b/crates/cli/Cargo.toml @@ -12,7 +12,6 @@ path = "src/main.rs" [features] dump_wat = ["dep:wasmprinter"] -experimental_event_loop = [] [dependencies] wizer = { workspace = true } diff --git a/crates/cli/tests/integration_test.rs b/crates/cli/tests/integration_test.rs index 62aedc8c..3cc7ed40 100644 --- a/crates/cli/tests/integration_test.rs +++ b/crates/cli/tests/integration_test.rs @@ -181,19 +181,20 @@ fn test_readme_script(builder: &mut Builder) -> Result<()> { Ok(()) } -#[cfg(feature = "experimental_event_loop")] -#[javy_cli_test] -fn test_promises(builder: &mut Builder) -> Result<()> { - let mut runner = builder.input("promise.js").build()?; +#[javy_cli_test(commands(not(Compile)))] +fn test_promises_with_event_loop(builder: &mut Builder) -> Result<()> { + let mut runner = builder + .input("promise.js") + .experimental_event_loop(true) + .build()?; let (output, _, _) = run(&mut runner, &[]); assert_eq!("\"foo\"\"bar\"".as_bytes(), output); Ok(()) } -#[cfg(not(feature = "experimental_event_loop"))] #[javy_cli_test] -fn test_promises(builder: &mut Builder) -> Result<()> { +fn test_promises_without_event_loop(builder: &mut Builder) -> Result<()> { use javy_runner::RunnerError; let mut runner = builder.input("promise.js").build()?; @@ -204,10 +205,12 @@ fn test_promises(builder: &mut Builder) -> Result<()> { Ok(()) } -#[cfg(feature = "experimental_event_loop")] -#[javy_cli_test] +#[javy_cli_test(commands(not(Compile)))] fn test_promise_top_level_await(builder: &mut Builder) -> Result<()> { - let mut runner = builder.input("top-level-await.js").build()?; + let mut runner = builder + .input("top-level-await.js") + .experimental_event_loop(true) + .build()?; let (out, _, _) = run(&mut runner, &[]); assert_eq!("bar", String::from_utf8(out)?); @@ -229,13 +232,13 @@ fn test_exported_functions(builder: &mut Builder) -> Result<()> { Ok(()) } -#[cfg(feature = "experimental_event_loop")] -#[javy_cli_test] +#[javy_cli_test(commands(not(Compile)))] fn test_exported_promises(builder: &mut Builder) -> Result<()> { let mut runner = builder .input("exported-promise-fn.js") .wit("exported-promise-fn.wit") .world("exported-promise-fn") + .experimental_event_loop(true) .build()?; let (_, logs, _) = run_fn(&mut runner, "foo", &[]); assert_eq!("Top-level\ninside foo\n", logs); diff --git a/crates/plugin-api/CHANGELOG.md b/crates/plugin-api/CHANGELOG.md index 0488663d..7a54c69f 100644 --- a/crates/plugin-api/CHANGELOG.md +++ b/crates/plugin-api/CHANGELOG.md @@ -8,6 +8,10 @@ Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Changed + +- `initialize_runtime` accepts a `javy_plugin_api::Config` instead of a `javy_plugin_api::javy::Config` + ## [1.0.0] - 2024-11-12 Initial release diff --git a/crates/plugin-api/Cargo.toml b/crates/plugin-api/Cargo.toml index 2fec798a..e2a33392 100644 --- a/crates/plugin-api/Cargo.toml +++ b/crates/plugin-api/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "javy-plugin-api" -version = "1.0.1-alpha.1" +version = "2.0.0-alpha.1" authors.workspace = true edition.workspace = true license.workspace = true @@ -14,5 +14,4 @@ anyhow = { workspace = true } javy = { workspace = true, features = ["export_alloc_fns"] } [features] -experimental_event_loop = [] json = ["javy/json"] diff --git a/crates/plugin-api/README.md b/crates/plugin-api/README.md index c57bba49..6cb5b4b8 100644 --- a/crates/plugin-api/README.md +++ b/crates/plugin-api/README.md @@ -15,7 +15,7 @@ Example usage: ```rust use javy_plugin_api::import_namespace; -use javy_plugin_api::javy::Config; +use javy_plugin_api::Config; // Dynamically linked modules will use `my_javy_plugin_v1` as the import // namespace. diff --git a/crates/plugin-api/src/config.rs b/crates/plugin-api/src/config.rs new file mode 100644 index 00000000..36b1831c --- /dev/null +++ b/crates/plugin-api/src/config.rs @@ -0,0 +1,32 @@ +use std::ops::{Deref, DerefMut}; + +#[derive(Default)] +/// A configuration for the Javy plugin API. +pub struct Config { + /// The runtime config. + pub(crate) runtime_config: javy::Config, + /// Whether to enable the experimental event loop. + pub(crate) experimental_event_loop: bool, +} + +impl Config { + /// Whether to enable the experimental event loop. + pub fn experimental_event_loop(&mut self, enabled: bool) -> &mut Self { + self.experimental_event_loop = enabled; + self + } +} + +impl Deref for Config { + type Target = javy::Config; + + fn deref(&self) -> &Self::Target { + &self.runtime_config + } +} + +impl DerefMut for Config { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.runtime_config + } +} diff --git a/crates/plugin-api/src/lib.rs b/crates/plugin-api/src/lib.rs index a3953df2..f473a377 100644 --- a/crates/plugin-api/src/lib.rs +++ b/crates/plugin-api/src/lib.rs @@ -3,7 +3,7 @@ //! Example usage: //! ```rust //! use javy_plugin_api::import_namespace; -//! use javy_plugin_api::javy::Config; +//! use javy_plugin_api::Config; //! //! // Dynamically linked modules will use `my_javy_plugin_v1` as the import //! // namespace. @@ -28,21 +28,21 @@ //! * [`import_namespace`] - required to provide an import namespace when the //! plugin is used to generate dynamically linked modules. //! * [`initialize_runtime`] - used to configure the QuickJS runtime with a -//! [`javy::Config`] and to add behavior to the created [`javy::Runtime`]. +//! [`Config`] to add behavior to the created [`javy::Runtime`]. //! //! # Features -//! * `experimental_event_loop` - enables the JS event loop. A number of -//! important things are not yet supported so be careful when enabling. //! * `json` - enables the `json` feature in the `javy` crate. use anyhow::{anyhow, bail, Error, Result}; +pub use config::Config; use javy::quickjs::{self, Ctx, Error as JSError, Function, Module, Value}; -use javy::{from_js_error, Config, Runtime}; +use javy::{from_js_error, Runtime}; use std::cell::OnceCell; use std::{process, slice, str}; pub use javy; +mod config; mod namespace; const FUNCTION_MODULE_NAME: &str = "function.mjs"; @@ -50,6 +50,7 @@ const FUNCTION_MODULE_NAME: &str = "function.mjs"; static mut COMPILE_SRC_RET_AREA: [u32; 2] = [0; 2]; static mut RUNTIME: OnceCell = OnceCell::new(); +static mut EVENT_LOOP_ENABLED: bool = false; static EVENT_LOOP_ERR: &str = r#" Pending jobs in the event queue. @@ -62,7 +63,7 @@ pub fn initialize_runtime(config: Config, modify_runtime: F) -> Result<()> where F: FnOnce(Runtime) -> Runtime, { - let runtime = Runtime::new(config).unwrap(); + let runtime = Runtime::new(config.runtime_config).unwrap(); let runtime = modify_runtime(runtime); unsafe { RUNTIME.take(); // Allow re-initializing. @@ -73,6 +74,7 @@ where // implement `Debug`. .map_err(|_| anyhow!("Could not pre-initialize javy::Runtime")) .unwrap(); + EVENT_LOOP_ENABLED = config.experimental_event_loop; }; Ok(()) } @@ -182,7 +184,7 @@ pub fn run_bytecode(bytecode: &[u8], fn_name: Option<&str>) { fn handle_maybe_promise(this: Ctx, value: Value) -> quickjs::Result<()> { match value.as_promise() { Some(promise) => { - if cfg!(feature = "experimental_event_loop") { + if unsafe { EVENT_LOOP_ENABLED } { // If the experimental event loop is enabled, trigger it. let resolved = promise.finish::(); // `Promise::finish` returns Err(Wouldblock) when the all @@ -205,7 +207,7 @@ fn handle_maybe_promise(this: Ctx, value: Value) -> quickjs::Result<()> { } fn ensure_pending_jobs(rt: &Runtime) -> Result<()> { - if cfg!(feature = "experimental_event_loop") { + if unsafe { EVENT_LOOP_ENABLED } { rt.resolve_pending_jobs() } else if rt.has_pending_jobs() { bail!(EVENT_LOOP_ERR); diff --git a/crates/plugin/Cargo.toml b/crates/plugin/Cargo.toml index ef35ed80..b7df06fe 100644 --- a/crates/plugin/Cargo.toml +++ b/crates/plugin/Cargo.toml @@ -14,6 +14,3 @@ anyhow = { workspace = true } javy-plugin-api = { path = "../plugin-api", features = ["json"] } serde = { workspace = true } serde_json = { workspace = true } - -[features] -experimental_event_loop = ["javy-plugin-api/experimental_event_loop"] diff --git a/crates/plugin/src/lib.rs b/crates/plugin/src/lib.rs index d0ed0083..2cc6c463 100644 --- a/crates/plugin/src/lib.rs +++ b/crates/plugin/src/lib.rs @@ -1,5 +1,4 @@ -use javy_plugin_api::import_namespace; -use javy_plugin_api::javy::Config; +use javy_plugin_api::{import_namespace, Config}; use shared_config::SharedConfig; use std::io; use std::io::Read; diff --git a/crates/plugin/src/shared_config/mod.rs b/crates/plugin/src/shared_config/mod.rs index 8e6c257b..c9e6c53b 100644 --- a/crates/plugin/src/shared_config/mod.rs +++ b/crates/plugin/src/shared_config/mod.rs @@ -1,12 +1,13 @@ //! APIs and data structures for receiving runtime configuration from the Javy CLI. use anyhow::Result; +use javy_plugin_api::Config; use serde::Deserialize; use std::io::{stdout, Write}; mod runtime_config; -use crate::{runtime_config, Config}; +use crate::runtime_config; runtime_config! { #[derive(Debug, Default, Deserialize)] @@ -25,6 +26,8 @@ runtime_config! { /// Whether to enable support for the `TextEncoder` and `TextDecoder` /// APIs. text_encoding: Option, + /// Whether to enable the experimental event loop. + experimental_event_loop: Option, } } @@ -49,6 +52,9 @@ impl SharedConfig { if let Some(enable) = self.text_encoding { config.text_encoding(enable); } + if let Some(enable) = self.experimental_event_loop { + config.experimental_event_loop(enable); + } } } diff --git a/crates/runner/src/lib.rs b/crates/runner/src/lib.rs index aaabb129..0f344ba4 100644 --- a/crates/runner/src/lib.rs +++ b/crates/runner/src/lib.rs @@ -83,6 +83,8 @@ pub struct Builder { simd_json_builtins: Option, /// Whether to enable the `TextEncoder` and `TextDecoder` APIs. text_encoding: Option, + /// Whether to enable the experimental event loop. + experimental_event_loop: Option, built: bool, /// Preload the module at path, using the given instance name. preload: Option<(String, PathBuf)>, @@ -109,6 +111,7 @@ impl Default for Builder { javy_json: None, simd_json_builtins: None, text_encoding: None, + experimental_event_loop: None, plugin: Plugin::Default, } } @@ -170,6 +173,11 @@ impl Builder { self } + pub fn experimental_event_loop(&mut self, enabled: bool) -> &mut Self { + self.experimental_event_loop = Some(enabled); + self + } + pub fn command(&mut self, command: JavyCommand) -> &mut Self { self.command = command; self @@ -202,6 +210,7 @@ impl Builder { javy_stream_io, simd_json_builtins, text_encoding, + experimental_event_loop, built: _, preload, command, @@ -229,6 +238,7 @@ impl Builder { javy_stream_io, simd_json_builtins, text_encoding, + experimental_event_loop, preload, plugin, ), @@ -305,6 +315,7 @@ impl Runner { javy_stream_io: Option, override_json_parse_and_stringify: Option, text_encoding: Option, + experimental_event_loop: Option, preload: Option<(String, PathBuf)>, plugin: Plugin, ) -> Result { @@ -326,6 +337,7 @@ impl Runner { &javy_stream_io, &override_json_parse_and_stringify, &text_encoding, + &experimental_event_loop, &plugin, ); @@ -554,6 +566,7 @@ impl Runner { javy_stream_io: &Option, simd_json_builtins: &Option, text_encoding: &Option, + experimental_event_loop: &Option, plugin: &Plugin, ) -> Vec { let mut args = vec![ @@ -609,6 +622,14 @@ impl Runner { args.push(format!("text-encoding={}", if enabled { "y" } else { "n" })); } + if let Some(enabled) = *experimental_event_loop { + args.push("-J".to_string()); + args.push(format!( + "experimental-event-loop={}", + if enabled { "y" } else { "n" } + )); + } + if matches!(plugin, Plugin::User | Plugin::DefaultAsUser) { args.push("-C".to_string()); args.push(format!("plugin={}", plugin.path().to_str().unwrap())); diff --git a/crates/test-plugin/src/lib.rs b/crates/test-plugin/src/lib.rs index 4988b26a..3029531a 100644 --- a/crates/test-plugin/src/lib.rs +++ b/crates/test-plugin/src/lib.rs @@ -1,7 +1,7 @@ //! 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}; +use javy_plugin_api::{import_namespace, Config}; import_namespace!("test_plugin"); diff --git a/docs/docs-contributing-architecture.md b/docs/docs-contributing-architecture.md index 3beaeb3a..ef110734 100644 --- a/docs/docs-contributing-architecture.md +++ b/docs/docs-contributing-architecture.md @@ -117,11 +117,7 @@ You should gate your feature with a cargo feature if your feature/change: - You want to have integration tests in the `javy-cli` crate that should only run when the `javy-plugin` crate is built with a non-default configuration (that - is, with different cargo features enabled). For example, we introduced the - `experimental_event_loop` cargo feature in the `javy-cli` crate since we test - for different expected outputs when using a promise when the - `experimental_event_loop` cargo feature is enabled on the `javy-plugin` crate - compared to when that cargo feature is disabled. + is, with different cargo features enabled). ### `javy-plugin` @@ -134,10 +130,7 @@ allow the CLI to set various JS runtime configuration options. #### When to add a `cargo` feature You should gate your feature with a cargo feature if you want to support -building a Wasm module with an experimental configuration of the runtime. We do -this for the event loop because the current implementation has not been -thoroughly vetted. We also need a build of Javy with event loop support to run -a number of web platform tests for text encoding. +building a Wasm module with a very unusual configuration of the runtime. ### `javy-plugin-api` diff --git a/docs/docs-using-extending.md b/docs/docs-using-extending.md index bc619048..36e269da 100644 --- a/docs/docs-using-extending.md +++ b/docs/docs-using-extending.md @@ -21,15 +21,14 @@ name = "my_plugin_name" crate-type = ["cdylib"] [dependencies] -javy-plugin-api = "1.0.0" +javy-plugin-api = "2.0.0" ``` And `src/lib.rs` should look like: ```rust -use javy_plugin_api::import_namespace; +use javy_plugin_api::{Config, import_namespace}; use javy_plugin_api::javy::quickjs::prelude::Func; -use javy_plugin_api::javy::Config; import_namespace!("my_plugin_name"); diff --git a/wpt/README.md b/wpt/README.md index ff244524..d82a8eb5 100644 --- a/wpt/README.md +++ b/wpt/README.md @@ -31,7 +31,6 @@ Test suites can be added in `test_spec.js`. Individual tests can be ignored by i ## Tips for getting tests to pass - Adding tests to the ignored list is acceptable if there is no intent to support the feature the test is testing. -- We highly recommend running the WPT suite with the `experimental_event_loop` Cargo feature enabled on the `javy-plugin` crate so tests relying on the event loop are able to pass. - Strongly consider adding tests in Rust for APIs you're adding to get faster feedback on failures the WPT suite catches while working on a fix. ### If you need to change upstream tests diff --git a/wpt/package.json b/wpt/package.json index 674b5bd8..f1848609 100644 --- a/wpt/package.json +++ b/wpt/package.json @@ -5,7 +5,7 @@ "type": "module", "scripts": { "bundle": "rollup -c rollup.config.js runner.js", - "javy": "../target/release/javy compile -o bundle.wasm bundle.js", + "javy": "../target/release/javy build -J experimental-event-loop -o bundle.wasm bundle.js", "wasmtime": "wasmtime bundle.wasm", "test": "npm run bundle && npm run javy && npm run wasmtime" },