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

xtask/Add rustwasm task #1008

Closed
wants to merge 3 commits into from
Closed

xtask/Add rustwasm task #1008

wants to merge 3 commits into from

Conversation

Luni-4
Copy link
Collaborator

@Luni-4 Luni-4 commented Nov 28, 2023

Pull Request Template

Checklist

  • Confirmed that run-checks all script has been executed.
  • Made sure the book is up to date with changes in this PR.

Related Issues/PRs

Provide links to relevant issues and dependent PRs.

Changes

This PR fixes #851

Testing

Describe how these changes have been tested.

@Luni-4
Copy link
Collaborator Author

Luni-4 commented Nov 28, 2023

@antimora

I need your help because I think I'm doing something wrong. I get the following error running cargo xtask dist

[2023-11-28T10:06:50Z INFO  xtask::runwasm] Generating package...
[2023-11-28T10:06:50Z INFO  xtask::runchecks] rustup target add wasm32-unknown-unknown


info: component 'rust-std' for target 'wasm32-unknown-unknown' is up to date
[2023-11-28T10:06:50Z INFO  xtask::runwasm] Building Non-SIMD version of wasm for web...
Error: failed to parse manifest: /home/burn/Cargo.toml
Caused by: failed to parse manifest: /home/burn/Cargo.toml
Caused by: TOML parse error at line 1, column 1
  |
1 | [workspace]
  | ^
missing field `package`

Error: cargo command failed

Is it related to xtask-wasm?

@Luni-4 Luni-4 requested a review from antimora November 28, 2023 12:34
@AlexErrant
Copy link
Contributor

Burn is setup as a workspace, and you're using wasm-pack, which led me to this possibly relevant issue.

@Luni-4
Copy link
Collaborator Author

Luni-4 commented Nov 29, 2023

@AlexErrant

I've used wasm-pack because of this script https://github.com/Tracel-AI/burn/blob/main/examples/image-classification-web/build-for-web.sh
I do not know which command I should use to build the package otherwise

env::set_var("RUSTFLAGS", rustflags);

// Build wasm-pack command
let mut command = Command::new("wasm-pack");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let mut command = Command::new("wasm-pack");
let mut command = Command::new("wasm-pack");
command.current_dir("./examples/image-classification-web");

I think adding current_dir is important, since that in turn sets which Cargo.toml to use. If you add

    let exit = build_command
        .spawn()
        .unwrap()
        .wait()
        .unwrap();
    log::info!("build's spawn's exit: {}", exit);

to dist below you'll observe that it runs successfully with exit code 0. Unfortunately, this doesn't solve the larger issue with arg.base.build_command(build_command). If you don't set current_dir, you'll observe that build_command.spawn() exits with the same error output as arg.base.build_command(build_command). This makes me think that arg.base.build_command(build_command) isn't respecting command.current_dir.

Grepping xtask-wasm for current_dir yields only one hit, so I think it's an upstream problem.

@antimora
Copy link
Collaborator

I've reviewed the work in progress and believe it will require additional time to be properly fixed.

I'm not sure if you have considered integrating xtask directly under example/image-classifier-web, which, in my opinion, seems to be the most appropriate approach. For the task at hand, I suggest relying on xtask-wasm. The xtask under each example would essentially serve as a driver, a thin wrapper, instead of incorporating the logic into the root's xtask. This approach would ensure each example remains self-contained, primarily leveraging the logic from xtask-wasm.

@antimora antimora marked this pull request as draft November 30, 2023 20:19
@Luni-4
Copy link
Collaborator Author

Luni-4 commented Nov 30, 2023

@antimora

My initial idea was to create a script which could be used for all our examples, but more generally, for wasm applications. That would also provide a general-purpose mechanism, that is why I've discarded the integration of xtask-wasm in each example. That could also lead to a bunch of duplicated code and difficulties in maintaining these examples if new code is added.

@antimora
Copy link
Collaborator

I'll close it for now since the approach will be different a little bit. I'll try to reuse code from it.

@antimora antimora closed this Jan 29, 2024
@Luni-4
Copy link
Collaborator Author

Luni-4 commented Jan 29, 2024

Yes, sorry for giving up this PR, busy times. You can reuse my code without any problem!

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.

Cross-Platform Integration using xtask-wasm
3 participants