Better implementation using sqlite-wasm-rs

[Spxg]

sqlite-wasm-rs has been updated to a major version, providing a better implementation. For details, see #4473

There are some breaking changes, such as no longer exporting init_sqlite(). But I realized that the last merged commit has not been released yet, and I think it is appropriate to merge it now.

[Spxg]

❤️ Any suggestions? @weiznich

[weiznich]

Thanks for opening this PR. There is generally no need to ping me after having a PR open for a day. If I don't response imitatively that usually means I have no capacity to deal with a PR/question/whatever now. I often answer soon, but sometimes I'm just away on holidays or have other things to do and an answer can take time then. Pinging me doesn't make this faster in any way, often it makes things slower as I have yet another notification to deal with, so please don't do that in the future, at least not after a day. That means I will look into this possibly later this week or next week. If you don't hear something until then feel free to ping me again.

[weiznich]

So I had some time to look into this. The change on diesels slide looks ok for me.

I also had a look at the changes in sqlite-wasm-rs and I found there a few in my opinion problematic things. Possibly that could be fixed or handled in a better way:

• The crate contains binary data (the wasm module itself in src/wrapper/jswam/sqlite3.wasm and two static libraries in library) See https://diff.rs/sqlite-wasm-rs/0.1.5/0.2.4/ to inspect these files. It's not great to include binary files as they make it really hard to see what's actually included in your final binary. In my opinion it would be desirable to build all three files as part of the build process via the build.rs file from source instead.
• There are two sets of bindings in the source code. One in src/shim/libsqlite3/bindings.rs and one in src/wrapper/libsqlite3/bindings.rs. From a quick look they seem to differ, but it's unclear for my why they differ. Also that might just be handled by a reexport of libsqlite3-sys? If the problem there is that libsqlite3-sys sets linker arguments you don't want to have it might be a solution to enable the in_gecko feature disable setting linker flags from there. Maybe it's even possible to use the sqlite3.c file from there for compilation?

All of that is no hard stopper for merging this, but something I would like to see discussed. Maybe there are hard reasons why all of this is required, in which case it would be great to see that documented somewhere.

[Spxg]

• For sqlite3.wasm, is only used in wrapper feature, and the interface is exported through wasm-bindgen. Its is not from the official product, see https://github.com/Spxg/sqlite-wasm-rs?tab=readme-ov-file#why-vendor-sqlite-wasm for details. Compiling sqlite.wasm requires the entire sqlite source code and the emscripten toolchain, which are expensive to introduce into build.rs, see https://sqlite.org/wasm/doc/trunk/building.md.
• For src/wrapper/libsqlite3/bindings.rs, in the wrapper features, those C APIs are manually wrapped instead of generated by bindgen. https://github.com/Spxg/sqlite-wasm-rs/blob/master/sqlite-wasm-rs/src/wrapper/c.rs. Directly using bindgen code will result in compilation errors (missing symbols), so there are only some basic types.(sqlite3_value, SQLITE_OK, ...).
• For src/shim/libsqlite3/bindings.rs, generated by sqlite-wasm-builder. In the shim feature, since wasm32-unknown-unknown does not have libc, emscripten is used here for compilation, otherwise we need to copy a bunch of c headers required for sqlite3 compilation, which is a bit of a hack for me. If sqlite3 is compiled at compile time, the emscripten toolchain is required, and we cannot assume that all users have it installed. (Believe me, because rust mainly supports the wasm32-unknown-unknown target, most people do not have the emscripten toolchain). Considering that wasm is cross-platform, vendor compilation products are acceptable. Here is an example that directly saves wat (WebAssembly Text Format).
• The default is the shim feature.

[Spxg]

All of that is no hard stopper for merging this, but something I would like to see discussed. Maybe there are hard reasons why all of this is required, in which case it would be great to see that documented somewhere.

I hope this answers your questions.

[weiznich]

Thanks for providing these answers.

There is a huge problem with including these binary data and link them afterwards: As a downstream consumer you cannot review what is linked into an application. That's a huge security problem and that is something I'm not comfortable with shipping in a released diesel version. I can understand that setting up the emscripten toolchain is complicated, but I personally would consider security to be more important than easy to use. I'm open for a opt-in way to use precompiled versions as long as there is documentation that explains how exactly these binaries can be reproduced and how can be ensured that a binary build according to these instructions can be used to verify that the prebuild binary is the same. The default needs to be building everything from source.

[Spxg]

I'm open for a opt-in way to use precompiled versions as long as there is documentation that explains how exactly these binaries can be reproduced and how can be ensured that a binary build according to these instructions can be used to verify that the prebuild binary is the same.

I'll think about how to do it.

[Spxg]

I'm open for a opt-in way to use precompiled versions as long as there is documentation that explains how exactly these binaries can be reproduced and how can be ensured that a binary build according to these instructions can be used to verify that the prebuild binary is the same. The default needs to be building everything from source.

I made changes to my code and can now compile locally using bundled instead of pre-compiled products (which require emcc).

In addition, the pre-compiled products have been compiled and committed through Github Actions, and the products can be traced. Spxg/sqlite-wasm-rs@4c6d908

Artifacts can be downloaded and compared. https://github.com/Spxg/sqlite-wasm-rs/actions/runs/13313741293

[Spxg]

The document also explains the compilation and pre-compilation products. If it is acceptable, I will release a new version.
https://github.com/Spxg/sqlite-wasm-rs?tab=readme-ov-file#shim-usage
https://github.com/Spxg/sqlite-wasm-rs?tab=readme-ov-file#why-provide-precompiled-library

[weiznich]

Thanks for the update ❤️. I've gone through the changes and left some comments there. Overall that's a huge step in the right direction. It already mostly addresses my concerns around the prebuild libsqlite.a libraries. Given that the shim variant is already off by default, it might be acceptable to leave the prebuild wasm binary there.

What I would like to see still changed is the default from using the prebuild binaries to using the bundled feature flags. All the other comments are mostly ideas for making things more robust.

[Spxg]

What I would like to see still changed is the default from using the prebuild binaries to using the bundled feature flags. All the other comments are mostly ideas for making things more robust.

Thanks! All comments fixed, and the artifact attestations looks great:
Spxg/sqlite-wasm-rs@4eb10f5
https://github.com/Spxg/sqlite-wasm-rs/attestations/4985266

[Spxg]

Because of breaking changes, sqlite-wasm-rs is updated to 0.3

1. Deleted wrapper feature
2. Use bundled feature to compile sqlite by default
3. Provided precompiled feature, see https://github.com/Spxg/sqlite-wasm-rs#why-provide-precompiled-library for details

[weiznich]

Thanks for the update and sorry for taking that long to review. I've been busy with other things.

That's nearly what we want here. The only thing that need to be changed is the enabled features for sqlite-wasm-rs As discussed before we shouldn't enable the precompiled feature by default.

[Spxg]

That's nearly what we want here. The only thing that need to be changed is the enabled features for sqlite-wasm-rs As discussed before we shouldn't enable the precompiled feature by default.

Do diesel's test and diesel_tests crate need to specify precompiled by default?

[weiznich]

Do diesel's test and diesel_tests crate need to specify precompiled by default?

I would prefer to use the bundled feature there instead, even if that requires setting up the emscripten tool chain on CI

[Spxg]

Do diesel's test and diesel_tests crate need to specify precompiled by default?

I would prefer to use the bundled feature there instead, even if that requires setting up the emscripten tool chain on CI

Solved

[weiznich]

Thanks

[Spxg]

It seems that the CI failed because of the compiler upgrade. Do I need to wait for this #4502 commit to be merged and then rebase?

[weiznich]

Yes, that just means that #4502 needs to be merged first. I can take care of the rebase afterwards, that shouldn't be a problem.