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 basic wasm32-unknown-unknown support #148

Closed
wants to merge 19 commits into from

Conversation

huangjj27
Copy link

@huangjj27 huangjj27 commented Dec 17, 2019

Simple version of web logger support

Update 2020-01-10 23:31:15

This PR is to add basic, ugly but work support for the wasm32-unknown-unknown target, with default features disable. Tests can be used as:

wasm-pack test --headless --firefox -- --no-default-features --test web
wasm-pack test --node -- --no-default-features --test node

Issues remain

  • To separate color, style(maybe still parts of fmt) and writer
  • To make wasm support independent to other features

compile

modified example default.rs:


#[macro_use]
extern crate log;

use env_logger::Env;

#[cfg(target="wasm32-unknown-unknown")]
use wasm_bindgen::prelude::*;

fn main() {
    // The `Env` lets us tweak what the environment
    // variables to read are and what the default
    // value is if they're missing
    let env = Env::default()
        .filter_or("MY_LOG_LEVEL", "trace")
        .write_style_or("MY_LOG_STYLE", "always");

    env_logger::init_from_env(env);

    trace!("some trace log");
    debug!("some debug log");
    info!("some information log");
    warn!("some warning log");
    error!("some error log");
}

#[cfg(target="wasm32-unknown-unknown")]
#[wasm_bindgen(start)]
fn start() {
    main();
}

compile the example and generate bindgen:

cargo build --example default --no-default-features
wasm-bindgen ./target/wasm32-unknown-unknown/debug/examples/default.wasm --out-dir=./wasm --web

add index.html to ./wasm directory:

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-type" content="text/html; charset=utf-8"/>
    <title>Tour - Iced</title>
  </head>
  <body>
    <script type="module">
      import init from "./default.js";

      init('./default_bg.wasm');
    </script>
  </body>
</html>

open the index.html from a browser(I used Firefox 71). It's expected to see logs on the console(F12)

@huangjj27
Copy link
Author

While implementing this feature, I found the lost of level after formatted, then I have to re parse the level message, Otherwise I have to log all message with console.log. Any better way to get the level message so that we could seperate different levels' message?

@huangjj27 huangjj27 changed the title WIP: Add a simple implement Add a simple implement Dec 22, 2019
@huangjj27
Copy link
Author

related to #102 #149

Copy link
Collaborator

@KodrAus KodrAus 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 working on this @huangjj27!

It would be great to at least build for the wasm target in CI, if not also run all the tests (looks like there's a wasm-bindgen-test crate we could use for that).

Cargo.toml Outdated Show resolved Hide resolved
examples/default.rs Outdated Show resolved Hide resolved
@KodrAus KodrAus changed the title Add a simple implement Add wasm32-unknown-unknown support Jul 1, 2020
@huangjj27
Copy link
Author

@KodrAus Thanks for your sugesstion! I have committed some codes for that.

Another point is that,how could we parse the logging levels on web as we do on native runtime? I think it is better to make use of the logging levels from the web console

@huangjj27 huangjj27 changed the title Add wasm32-unknown-unknown support WIP: Add wasm32-unknown-unknown support Jul 5, 2020
@huangjj27
Copy link
Author

huangjj27 commented Jul 5, 2020

block the merge with the logging levels issue I finally decided to “make it work first", because parsing the logging levels might need some refactor work.

@huangjj27 huangjj27 changed the title WIP: Add wasm32-unknown-unknown support Add wasm32-unknown-unknown support Jul 11, 2020
@jplatte
Copy link
Contributor

jplatte commented Oct 16, 2020

Hey @huangjj27, it's a little hard to tell what the current state of this is. Is there more work to be done?

@huangjj27
Copy link
Author

Is there more work to be done?

Hi @jplatte , I think there is:

  • When I wrote this PR, I hadn't considered how to log with the browsers' logging level, all message is at the log level now. I think that integrating with the browser can be improved.
  • the CI seems blocked, we need to pass through it

@jplatte
Copy link
Contributor

jplatte commented Oct 17, 2020

  • When I wrote this PR, I hadn't considered how to log with the browsers' logging level, all message is at the log level now. I think that integrating with the browser can be improved.

Do you plan to work on this?

  • the CI seems blocked, we need to pass through it

I'd expect CI to run properly after a rebase.

@huangjj27
Copy link
Author

Do you plan to work on this?

@jplatte Hi! I rebased and it passed all checks now. I'm glad that someone will help with this issue. I think it needs much refactoring to the current formatter?

@KodrAus
Copy link
Collaborator

KodrAus commented Nov 8, 2020

We could refactor things to pass the log::Level all the way through the writer so that web::print can pick the right console function to call.

@huangjj27
Copy link
Author

I think this will pending until refactoring for this is done, for there are information of level and some other meta data we can make use of.

@KodrAus
Copy link
Collaborator

KodrAus commented Jan 12, 2021

@huangjj27 Support for structured logging is something we could add at any time, but is absolutely desirable.

I think all we need to do here is add some CI support for WebAssembly. I've got an example from another project, and there's also the wasm-bindgen docs.

If you'd like a hand getting this up and running let me know!

@huangjj27
Copy link
Author

If you'd like a hand getting this up and running let me know!

Ok, I will take this at the weekend, or as soon as I’ get spare time

@huangjj27 huangjj27 changed the title Add wasm32-unknown-unknown support WIP: Add wasm32-unknown-unknown support Jan 14, 2021
@huangjj27
Copy link
Author

CI is added but it seems not to test. Will fix it as soon

@huangjj27
Copy link
Author

Aha! here is the document.

@huangjj27
Copy link
Author

huangjj27 commented Jan 15, 2021

encounter with this bug. To work around this, I think we have to disable the default feature when we compile to the wasm32-unknown-unknown target, whose time should be provided by the browser/node.

@huangjj27 huangjj27 marked this pull request as draft January 16, 2021 06:41
@huangjj27
Copy link
Author

huangjj27 commented Jan 16, 2021

@KodrAus I get it worked, but there is some issue: when I tried to compile with the wasm32-unknown-unknown target, it still prevented me from compiling, showing this denied warning:

error: function is never used: `print`
   --> src\fmt\writer\wasm.rs:13:31
    |
13  | pub(in crate::fmt::writer) fn print(msg: &str, t: Target) {
    |                               ^^^^^
    |
note: lint level defined here
   --> src\lib.rs:280:54
    |
280 | #![deny(missing_debug_implementations, missing_docs, warnings)]
    |                                                      ^^^^^^^^
    = note: `#[deny(dead_code)]` implied by `#[deny(warnings)]`

error: aborting due to previous error

although the wasm::print function is use in termcolor::shim_impl :

  // termcolor/shim_impl.rs: 32-48
   pub(in crate::fmt::writer) fn print(&self, buf: &Buffer) -> io::Result<()> {
        // This impl uses the `eprint` and `print` macros
        // instead of using the streams directly.
        // This is so their output can be captured by `cargo test`
        let log = String::from_utf8_lossy(&buf.0);

        #[cfg(all(target_arch = "wasm32", target_vendor = "unknown"))]
        wasm::print(&log, self.target);

        #[cfg(not(all(target_arch = "wasm32", target_vendor = "unknown")))]
        match self.target {
            Target::Stderr => eprint!("{}", log),
            Target::Stdout => print!("{}", log),
        }

        Ok(())
    }

So I have to comment out the "deny warning" parts.
I think there is something I don't know or I mess up so here is the error. If you have some advice please let me know, thanks!

@huangjj27 huangjj27 marked this pull request as ready for review January 16, 2021 12:34
@huangjj27 huangjj27 changed the title WIP: Add wasm32-unknown-unknown support Add basic wasm32-unknown-unknown support Jan 16, 2021
@huangjj27
Copy link
Author

Hi @KodrAus, basic implement and basic CI are committed. Any Advice?

@jplatte
Copy link
Contributor

jplatte commented Jan 16, 2021

I've just removed deny(warnings) on master, we really shouldn't do that. CI will still fail with rustc or clippy emitting warnings though, since pass -- -D warnings. Could it be that (without deny(warnings)) the function that termcolor::shim_impl::print is also marked unused? IIRC, rustc will complain about transitively dead code too.

@sunfishcode
Copy link

I'm curious why it's desirable to use env_logger on wasm32-unknown-unknown. The motivation here seems to be for use on the Web, but the Web embedding doesn't have environment variables, and its stdin and stderr don't print anything. The log facade is intended to make it easy to plug in different logging backends; would it make sense to write a new backend rather than porting env_logger?

@huangjj27
Copy link
Author

would it make sense to write a new backend rather than porting env_logger?

Thanks for advice! I'll abandon this PR.

@huangjj27 huangjj27 closed this Dec 2, 2022
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.

None yet

4 participants