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

(WIP) Use Cargo.lock to identify wasm-bindgen version. #302

Closed
wants to merge 1 commit into from

Conversation

data-pup
Copy link
Member

@data-pup data-pup commented Sep 12, 2018

Fixes #270. This aims to find the wasm-bindgen version using the lockfile.

Note: This is still a WIP, and should probably land after #271. While working on this, I ran into some of the same problems that were solved by that PR, so I tried to remove any duplicate work. I can rebase this once that lands, but review/feedback is welcome in the meantime :)


Make sure these boxes are checked! 📦✅

  • You have the latest version of rustfmt installed and have your
    cloned directory set to nightly
$ rustup override set nightly
$ rustup component add rustfmt-preview --toolchain nightly
  • You ran rustfmt on the code base before submitting
  • You reference which issue is being closed in the PR text

✨✨ 😄 Thanks so much for contributing to wasm-pack! 😄 ✨✨

@data-pup
Copy link
Member Author

data-pup commented Sep 12, 2018

Unrelated, but of note: I've had a lot of trouble running the test bench on my machine. Admittedly, this might be a resources problem (< 4Gb RAM), but it might be worth thinking about in the future for other contributors :)

cargo test -- --test-threads=1 has been helpful for me, could be worth adding to the contributor help docs

@data-pup data-pup force-pushed the lockfile-bindgen-version branch from 8247c51 to 5813428 Compare September 12, 2018 21:34
@ashleygwilliams ashleygwilliams added the work in progress do not merge! label Sep 13, 2018
@ashleygwilliams
Copy link
Member

thanks so much for this @data-pup - #271 should be landing momentarily so hopefully that unblocks you! also thanks for noting the test stuff- i've also run into that. let me get the moar docs Pr landed and then i would happily accept a PR for that!

@fitzgen
Copy link
Member

fitzgen commented Sep 14, 2018

Unrelated, but of note: I've had a lot of trouble running the test bench on my machine. Admittedly, this might be a resources problem (< 4Gb RAM), but it might be worth thinking about in the future for other contributors :)

When running the wasm-pack tests, I've had issues that I think boil down to doing lots of I/O on encrypted disks on linux but I'm not totally sure. I have low CPU usage, low mem usage, and yet my system is super janky. Not totally sure what's up.

If you think you might have similar problems, I'd love to hear more. Or just generally what appears to be the bottleneck for you: CPU, memory, etc and how you determined that (eg looking at an activity monitor while running tests or using perf, etc).

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

This is looking really good @data-pup :)

More cases we can handle now and should have tests for:

  • running wasm-pack build in each crate within an umbrella workspace:

    umbrella/
      - Cargo.toml                        # just have `[workspace] members = [..]`
      - foo/
        - Cargo.toml
        - src/
          - lib.rs
      - bar/
        - Cargo.toml
        - src/
          - lib.rs
    
  • a dependency uses wasm-bindgen, but the root crate does not:

    // child/src/lib.rs
    extern crate wasm_bindgen;
    use wasm_bindgen::prelude::*;
    
    #[wasm_bindgen]
    pub fn hello() -> u32 { 42 }
    
    // parent/src/lib.rs
    extern crate child;
    pub use child::*;

toml::from_str(&lockfile).map_err(Error::from)
}

/// Given the path to the crate that we are buliding, return a `PathBuf`
Copy link
Member

Choose a reason for hiding this comment

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

"buliding" -> "building"

@data-pup
Copy link
Member Author

Awesome, thanks so much for the help/review everyone! I'll spend some more time on this PR this week, and should hopefully have it finished up by Friday at the latest. 😸

@fitzgen
Copy link
Member

fitzgen commented Sep 18, 2018

BTW, make sure that you delete any target directories from inside the test fixtures! Those get copied during the tests, which would slow thigns down a lot. Not sure if this is the issue you are running into wrt test performance, but @ashleygwilliams just ran into it, so worth checking!

@fitzgen
Copy link
Member

fitzgen commented Sep 20, 2018

Rebased and updated in #324.

Sorry to snatch this PR away from you @data-pup! I don't like to do this. Just really want to get a new release of wasm-pack out (see all the recent issues filed that have solutions on master) and this was the last thing blocking us :( Thanks for understanding!

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

Successfully merging this pull request may close these issues.

Handle versions of wasm-bindgen in lockfiles
3 participants