Closed jmcnamara closed 1 year ago
On Fri, 12 May 2023 at 10:21, 苯苯 @.***> wrote:
@jmcnamara I am a novice in rust, can I try to do this work?
Sure. Some volunteer is better than no volunteer. :-)
@jmcnamara Any plans? For example
I think we or maybe need some planning ?
Any plans?
I think the best approach would be to create a separate GitHub project and get some examples working with Javascript.
Then, once it it working, and we get some user testing/feedback, I will add a chapter in the rust_xlsxwriter
User Guide with links/code from the Wasm project.
ok
Hey, was working on a rust lib with python and wasm bindings and noticed this issue. In my case, I couldn't get rust_xlswriter
to work with wasm because of time not being implement on that platform (yet). I believe this is due to your usage of the chrono
crate. It looks like they have a wasmbind
feature flag that should make it work in WebAssembly (for reference).
Could the feature flag be added for chrono?
Could the feature flag be added for chrono?
It could. I could hide all the date/time types behind IntoDateExcelTime/IntoExcelDate/IntoExcelTime
traits and implement it for the chrono types (under a feature flag) and some other tuple/string types with a fallback to native/internal date/time handling. I already have functions like that in the C version of the library.
This would give the user some access to date/time handling (which is important in Excel) but also chrono if they wanted it. This would also remove, or at least make an option of, a dependency which would be good.
Note, this part of the request is similar to #6.
Could the feature flag be added for chrono?
@Ashci42 I have made chrono
an "optional on" feature in rust_xlsxwriter
v0.41.0 and it will be "optional off" in future version (and current main). I've added the ExcelDateTime
struct as a native alternative.
I don't think it is currently possible to run rust_xlsxwriter
with Wasm.
Calling Workbook::new()
calls DocProperties::new()
, which itself calls ExcelDateTime::utc_now()
. This method calls std::time::SystemTime::now()
, which panics on runtime in Wasm.
Perhaps we could make DocsProperties.creation_time
an OptionWorkbook
would also need to be able to be created with a user-defined DocProperties
, instead of just calling DocProperties::new()
.
I also think that packager::assemble_file will probably panic, as it calls std::thread::(scope::)spawn
, which I don't think is implemented in Wasm.
I don't think it is currently possible to run
rust_xlsxwriter
with Wasm.
Thanks for taking a look at this.
Calling
Workbook::new()
callsDocProperties::new()
, which itself callsExcelDateTime::utc_now()
. This method callsstd::time::SystemTime::now()
, which panics on runtime in Wasm.
Excel has metadata with a creation and modification data (which are the same for a new file). It is possible to specify a datetime so that ExcelDateTime::utc_now()
isn't called like this:
use rust_xlsxwriter::{DocProperties, ExcelDateTime, Workbook, XlsxError};
fn main() -> Result<(), XlsxError> {
let mut workbook = Workbook::new();
// Create a file creation date for the file.
let date = ExcelDateTime::from_ymd(2023, 1, 1)?;
// Add it to the document metadata.
let properties = DocProperties::new().set_creation_datetime(&date);
workbook.set_properties(&properties);
let worksheet = workbook.add_worksheet();
worksheet.write_string(0, 0, "Hello")?;
workbook.save("properties.xlsx")?;
Ok(())
}
See: https://rustxlsxwriter.github.io/workbook/checksum.html#checksum-of-a-saved-file
However, I don't know if it is still incompatible with Wasm if the ExcelDateTime::utc_now()
function is still in the compile path even if not in the execution path.
I also think that packager::assemble_file will probably panic, as it calls
std::thread::(scope::)spawn
, which I don't think is implemented in Wasm.
That was only added in recently in v0.44.0. You could test with version v0.43.0 instead. If required I could put it behind a wasm
feature flag.
It is possible to specify a datetime so that
ExcelDateTime::utc_now()
isn't called like this:
Actually I'm wrong. It is called at the creation of DocProperties
. However, I could make that an Option<>
or also hide it behind a feature flag. It might matter to some folks if the creation time isn't actually the creation time.
I have forked the repo (here) changing those two things (making DocsProperties.creation_time
an Option and putting std::thread::spawn
under a cfg attribute) and everything seems to work so far. I'm afraid that I don't have a simple example to show that it actually works, though.
It might matter to some folks if the creation time isn't actually the creation time.
Yeah, that makes sense. Although maybe a DocProperties::new_lazy
method could be added so that DocProperties::new
does what it does now (plus wrapping DocsProperties.creation_time
in Some
), and therefore we have both methods without an additional feature flag
I have forked the repo (here) changing those two things (making
DocsProperties.creation_time
an Option and puttingstd::thread::spawn
under a cfg attribute) and everything seems to work so far.
Thanks for that.
For the ExcelDateTime::utc_now()
issue I think a more complete solution would be to fall back to a JS function on wasm targets like Chrono does. I could implement a wasmbind
feature flag like Chrono and pass it to Chrono when that is in use. (I now see that this is what @Ashci42 was asking for above but I missed the point.) I'll also use it to make ExcelDateTime
JS compliant and to turn off the back end threading.
I'm afraid that I don't have a simple example to show that it actually works, though.
That is still the main help I need here. It doesn't have to be full tutorial, like I asked for above. Even a hello world example with some instructions would be useful to debug and test issues like this.
@Clipi-12 (or @Ashci42) I have pushed a (hopefully) Wasm compatible version to the wasm
branch if you can test it.
I don't know if it also needs some target_arch
configs. You can let me know.
I need to add support for the wasm
flag into the chrono
flag as well. I'll add that a bit later.
I'll take a look at this.
In the meanwhile, it could be important to think about Wasm in non-javascript environments. I'm not an expert on this, but I know Wasm was "designed to be capable of being executed without a Javascript VM present".
Also, if we look at chrono's Utc::now()
implementation they don't call js_sys
unless they absolutely need to (i.e. when wasm32-unknown-unknown is used). In the case that wasi or emscriptem are used, chrono doesn't use js_sys or anything javascript-related, (presumably) so that the Wasm code doesn't need a javascript environment.
Also, if we look at chrono's
Utc::now()
implementation they don't calljs_sys
unless they absolutely need to
Yes, you are probably right. I had a look at the time.rs
crate and they do the same thing. I'll update the branch. -- Done.
I have made a minimal example of a rust_xlsxwriter program that compiles to WASM: https://github.com/Clipi-12/rust_xlsx_wasm_example It can be used to compile and test in a browser environment, in Nodejs, in Deno, and in WASI.
It is a bit big, but there's not much I can do, since most files are just there to replace std::fs with browser-specific/nodejs-specific/deno-specific code that does the same thing.
I also found a bug in the current branch. You are taking the result of Date.now() as if it meant seconds since UNIX_EPOCH, while it actually represents the number of milliseconds since UNIX_EPOCH (so you just have to divide it by 1,000 and it would be fixed)
That is really great. Thanks!
I was able to get the browser example working with minimal fuss. That and the other environments make it perfect for testing so that is just what I needed.
I also found a bug in the current branch
I fixed that on the wasm
branch and force pushed it.
It can be used to compile and test in a browser environment, in Nodejs, in Deno, and in WASI.
I was able to get all of the targets (Browser, Node.js, Deno and WASI) working after installing a few dependencies. So that is really useful for testing.
It is a bit big, but there's not much I can do, since most files are just there to replace std::fs with browser-specific/nodejs-specific/deno-specific code that does the same thing.
It doesn't seem too bad.
I have a few minor suggestions. I'll make them directly on your repo a bit later.
Thanks once more.
@Clipi-12 I added support in rust_xlsxwriter
for wrapping wasm_bindgen::JsValue
into XlsxError
. That should simplify the code and make handling errors in the wasm app(s) easier since it already wraps IO::Error
.
I've pushed it to a different branch (wasm2
) since it doesn't work for wasmtime which gives a generic and not very clear error:
$ wasmtime --dir=. pkg/wasi/rust_xlsx_wasm_example.wasm
Error: failed to run main module `pkg/wasi/rust_xlsx_wasm_example.wasm`
Caused by:
0: failed to instantiate "pkg/wasi/rust_xlsx_wasm_example.wasm"
1: unknown import: `__wbindgen_placeholder__::__wbindgen_throw` has not been defined
EDIT: never mind, I understand what you meant with the last commit. It is indeed very useful! I'll probably remove some code in the example thanks to that
@jmcnamara I modified a private copy of the wasm2 branch and got rid of the error by simply avoiding to import wasm-bindgen in wasi. Honestly I think the best option is to rename the wasm
feature to javascript
, since wasi already does all the thing we are trying to do with javascript, but it does them natively.
Well, everything but std::thread:spawn
, which doesn't really need the feature-flag check anyways, since it can use #[cfg(target_arch = "wasm32")]
Also, an additional benefit is that if we rename the feature flag to javascript
we don't have to do the long check #[cfg(all(target_arch = "wasm32", not(any(target_os = "emscripten", target_os = "wasi"))))]
since we are guaranteed that js_sys is going to work
never mind, I understand what you meant with the last commit. It is indeed very useful! I'll probably remove some code in the example thanks to that
That is good. I meant to attach the updated example but somehow failed to do it, but it looks like you figured it out anyway. :-)
I'll have a look at the changes a bit later.
Apologies for slow responses, I have some other things going on at the moment.
I got a chance to look at this again and your recent changes look good. I've cleaned things up on my side and merged the changes onto main. I'll push the changes to crates.io in the next few days if everything is okay on your side and mine.
Honestly I think the best option is to rename the
wasm
feature tojavascript
,
I'm going to stick with wasm
for now since I think that makes most sense in terms of what what users will be looking for.
Well, everything but
std::thread:spawn
, which doesn't really need the feature-flag check anyways, since it can use#[cfg(target_arch = "wasm32")]
Good point. I made that change.
Also, an additional benefit is that if we rename the feature flag to
javascript
we don't have to do the long check
I don't really get this. Could you explain a bit more. Or submit a PR against main with the suggested changes.
I don't really get this. Could you explain a bit more. Or submit a PR against main with the suggested changes.
I was about to submit a PR when I noticed that it actually doesn't matter. I thought that we could ignore the target_arch
and target_os
as long as the wasm
feature flag was set because of the panicking we faced when running in non-javascript targets. I thought that we could assume we were in a js environment because otherwise it would still panic elsewhere.
However, the panic no longer occurs, and even if it did, it's honestly more safe to just do the check, in case it would ever get solved (which it already is).
Thank for that @Clipi-12
I've pushed the changes to crates.io in v0.47.0.
I've mentioned your repo in the release notes: https://rustxlsxwriter.github.io/changelog.html#0470---2023-09-02
Thanks once more for the help. There will probably be some improvements to be added over time (in both repos) but this is a good start.
I'm going to close this feature request since I think it completes the "Help needed" request.
Help wanted
It would be great if someone could create a Wasm example/tutorial using
rust_xlsxwriter
.Ideally I could add this as a section in or link from the Working with the rust_xlsxwriter library user guide.
An example like one in the SheetJS tutorial would be good but don't copy it exactly.
It would be good to use the
write_row_matrix()
orwrite_column_matrix()
methods that are currently on main (#16) and which will be in v0.39.0.It would also be nice to use whatever is the current Rust/Wasm best practices.