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

Add wasm32-unknown-unknown target support #4411

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

Spxg
Copy link

@Spxg Spxg commented Dec 29, 2024

I recently created a rust wrapper crate for sqlite-wasm and now diesel can run successfully on the wasm32-unknown-unknown platform.

Diesel’s integration tests and unit tests all run successfully,except for a few tests that required std::fs::* and doc test (wasm-bindgen-test currently does not support this).
Branch: https://github.com/Spxg/diesel/tree/wasm32-test (There are many changes, not planning to merge them at the moment).

cargo install wasm-pack
cargo install wasm-bindgen-cli
git clone https://github.com/Spxg/diesel -b wasm32-test
cd diesel/diesel
cargo xtask run-tests --wasm
CHROMEDRIVER="your_chrome_driver_path" cargo +nightly test --doc --target wasm32-unknown-unknown --features sqlite -Zdoctest-xcompile

You will see output similar to this: https://raw.githubusercontent.com/Spxg/Spxg/refs/heads/master/resources/sqlitest.gif

@Spxg Spxg marked this pull request as draft January 2, 2025 02:25
@insipx
Copy link
Contributor

insipx commented Jan 2, 2025

nice, this seems to be cleaner than re-implementing the backend in a separate crate, and maybe better than storing them on function pointers like in the discussion?

How would SQLite updates work with this. Further, it would be most efficient to implement a VFS in rust rather than go to JS and back, would this wrapper crate allow for that from diesel?

@Spxg
Copy link
Author

Spxg commented Jan 3, 2025

nice, this seems to be cleaner than re-implementing the backend in a separate crate, and maybe better than storing them on function pointers like in the discussion?

How would SQLite updates work with this. Further, it would be most efficient to implement a VFS in rust rather than go to JS and back, would this wrapper crate allow for that from diesel?

The current implementation of sqlite-wasm-rs is actually the same as what the "JS side" in the sqlite-wasm project does (using various wrappers to implement calls, including type conversion and wasm memory allocation).

It would be most efficient if a VFS could be implemented in rust and registered using sqlite3_vfs_register. It can be implemented directly in libsqlite3-sys, and ORM frameworks such as diesel and sqlx do not even need to modify the implementation.

However, I know very little about VFS used by the web, such as OPFS. The opfs used by sqlite-wasm is not intended to be exported as an API (https://github.com/sqlite/sqlite/blob/04364cb3cc108a044a0d9dc7162f4d550adb2f99/ext/wasm/api/sqlite3-vfs-opfs.c-pp.js#L54), but the wa-sqlite project has other VFS for reference (https://github.com/rhashimoto/wa-sqlite/tree/master/src/examples).

In short, if there is a VFS solution, I am happy to try it.

Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR. Overall I'm very much in favor of this approach as it looks really clean in terms of code changes. I've added two questions about some details.

That written: I personally would prefer to run at least the tests in CI to ensure that this does not break in the future.

As for the VFS discussion: I can imaging that diesel exposes the necessary traits to implement a custom VFS. A quick search on crates.io brought up https://docs.rs/sqlite-vfs/latest/sqlite_vfs/, which might serve as basis for such an implementation.

Comment on lines +771 to +777
// Before using C-API, you must initialize sqlite.
//
// Initializing the database is a one-time operation during
// the life of the program.
#[cfg(feature = "sqlite")]
#[cfg(all(target_family = "wasm", not(target_os = "wasi")))]
pub use sqlite_wasm_rs::init_sqlite;
Copy link
Member

Choose a reason for hiding this comment

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

I would rather not expose this function from diesel as this would couple diesel's API stability to that one of sqlite_wasm_rs. Ideally we would just call that function internally one, or even better sqlite_wasm_rs would provide a singelton helper function to do this.

We do something similar to this for the mysql backend already:

/// > In a non-multi-threaded environment, `mysql_init()` invokes
/// > `mysql_library_init()` automatically as necessary. However,
/// > `mysql_library_init()` is not thread-safe in a multi-threaded environment,
/// > and thus neither is `mysql_init()`. Before calling `mysql_init()`, either
/// > call `mysql_library_init()` prior to spawning any threads, or use a mutex
/// > to protect the `mysql_library_init()` call. This should be done prior to
/// > any other client library call.
///
/// <https://dev.mysql.com/doc/c-api/8.4/en/mysql-init.html>
static MYSQL_THREAD_UNSAFE_INIT: Once = Once::new();
fn perform_thread_unsafe_library_initialization() {
MYSQL_THREAD_UNSAFE_INIT.call_once(|| {
// mysql_library_init is defined by `#define mysql_library_init mysql_server_init`
// which isn't picked up by bindgen
let error_code = unsafe { ffi::mysql_server_init(0, ptr::null_mut(), ptr::null_mut()) };
if error_code != 0 {
// FIXME: This is documented as Nonzero if an error occurred.
// Presumably the value has some sort of meaning that we should
// reflect in this message. We are going to panic instead of return
// an error here, since the documentation does not indicate whether
// it is safe to call this function twice if the first call failed,
// so I will assume it is not.
panic!("Unable to perform MySQL global initialization");
}
})
}

Copy link
Author

@Spxg Spxg Jan 4, 2025

Choose a reason for hiding this comment

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

Ideally we would just call that function internally one, or even better sqlite_wasm_rs would provide a singelton helper function to do this.

Yes, the best way is to implement a singleton function call internally. But unlike mysql_server_init, init_sqlite is asynchronous, and since the diesel function is called synchronously, there is no opportunity to execute init_sqlite.
Any good suggestions for this?

[target.'cfg(not(all(target_family = "wasm", not(target_os = "wasi"))))'.dependencies]
libsqlite3-sys = { version = ">=0.17.2, <0.31.0", optional = true, features = ["bundled_bindings"] }

[target.'cfg(all(target_family = "wasm", not(target_os = "wasi")))'.dependencies]
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering whether this predicate essentially boils down to target = "wasm32-unknown-unknow in practice. Are you aware of any other target that fulfills this constraints?

Copy link
Author

@Spxg Spxg Jan 4, 2025

Choose a reason for hiding this comment

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

I will change it to #[cfg(all(target_family = "wasm", target_os = "unknown"))], which is recommended by the rustc book:
https://doc.rust-lang.org/beta/rustc/platform-support/wasm32-unknown-unknown.html#conditionally-compiling-code

@Spxg
Copy link
Author

Spxg commented Jan 4, 2025

That written: I personally would prefer to run at least the tests in CI to ensure that this does not break in the future.

I agree with you, but in order to solve the differences between native and wasm tests, I created the #[test_derive::test] macro to do this. Is it acceptable to replace #[test] with #[test_derive::test] in future maintenance?

In addition, some changes are required to run rustdoc tests

@insipx
Copy link
Contributor

insipx commented Jan 4, 2025

It would be most efficient if a VFS could be implemented in rust and registered using sqlite3_vfs_register. It can be implemented directly in libsqlite3-sys, and ORM frameworks such as diesel and sqlx do not even need to modify the implementation.

However, I know very little about VFS used by the web, such as OPFS. The opfs used by sqlite-wasm is not intended to be exported as an API (https://github.com/sqlite/sqlite/blob/04364cb3cc108a044a0d9dc7162f4d550adb2f99/ext/wasm/api/sqlite3-vfs-opfs.c-pp.js#L54), but the wa-sqlite project has other VFS for reference (https://github.com/rhashimoto/wa-sqlite/tree/master/src/examples).

The VFS Implementation is only interesting for web because of the variety of ways to accomplish a VFS, the immaturity of OPFS vs IndexedDb, as well as the newness of SQLite Wasm build itself. Therefore (and maybe my opinion only) it is useful to expose methods for a flexible VFS implementation rather than forcing a specific implementation lower in the stack. At least, until standards and best practices are formed.

Off the top, certain VFS implementations may play better with multiple open tabs to the same db, for instance, while other may completely disallow that kind of behavior.

I'm satisfied with Weiznich's suggestion, though, didn't even know about that crate! An API to simply register the vfs is all that would be needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants