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

coordinating wasm-bindgen versions #146

Closed
ashleygwilliams opened this issue Jun 5, 2018 · 14 comments · Fixed by #244
Closed

coordinating wasm-bindgen versions #146

ashleygwilliams opened this issue Jun 5, 2018 · 14 comments · Fixed by #244
Labels
PR attached there's a PR open for this issue question Further information is requested
Milestone

Comments

@ashleygwilliams
Copy link
Member

currently we are force installing wasm-bindgen, but this is frustrating because it takes a long time and often doesn't need to happen. i'd love a solution for this. let's use this issue to brainstorm. (related to #51 )

strawman proposal:

  • read cargo.toml to find wasm-bindgen lib version
  • check currently install wasm-bindgen version
    • if not installed, install version listed in cargo.toml
    • if installed, check version, if version mismatch install otherwise skip

@alexcrichton can you confirm that the lib and cli versions are paired up and that this strategy would be effective?

@ashleygwilliams ashleygwilliams added help wanted Extra attention is needed question Further information is requested labels Jun 5, 2018
@mgattozzi
Copy link
Contributor

One issue I forsee is if the project uses 0.2.3 but the current one is 0.2.8 for instance. What if they want to specifically use 0.2.3? Where should we install it? One approach I like from stack a Haskell package manager is that it sandboxes build tools and the compiler used for the project.

Would it be possible to compile a version of wasm-bindgen-cli in say the target directory and use the one there? That way it's not checked in by accident and we can have it be project dependent for wasm-bindgen-cli.

@ashleygwilliams
Copy link
Member Author

oh i like this idea. npm does the same thing with dev dependencies. perhaps we should take that tack and read it from devdependencies?

@mgattozzi
Copy link
Contributor

Isn't wasm-bindgen-cli a binary not a lib? Can we even specify it in dev-dependencies?

@data-pup
Copy link
Member

I rebased the PR I had open at #81, to prepare for taking care of this. I had a few questions regarding issue:

@ashleygwilliams
Copy link
Member Author

i don't think it's blocked by #162, but i think i'll probably land #162, #127, #151, #150 for 0.4.0 and this might be something we put off until 0.5.0- again, not blocked, but worth mentioning because #151 introduces a p large rewrite so i can only imagine what the rebase will look like :/

@ashleygwilliams
Copy link
Member Author

and re:strategy... i am still not entirely sure, but the devdep one is the best idea i have so far! alex is on vacay for a month so i'm not sure- perhaps @fitzgen has some thoughts?

@ashleygwilliams ashleygwilliams added this to the 0.5.0 milestone Jun 11, 2018
@fitzgen
Copy link
Member

fitzgen commented Jun 11, 2018

The thing with dev-dependencies is that is a cargo-centric thing, but all of our post-build steps aren't really integrated into cargo.

What I did in the template was to ensure that a project-local wasm-bindgen CLI was always installed by using cargo install --root like this: https://github.com/rustwasm/rust_wasm_template/blob/master/ci/script.sh#L155-L174

@data-pup
Copy link
Member

Aiming for 0.5.0 makes sense to me! Thanks for the heads up on what's landing soon @ashleygwilliams, and thank you for that link @fitzgen, that's really helpful to know about 😄

@FreeMasen
Copy link

FreeMasen commented Jun 13, 2018

If still available, I would like to take this on?

The way I see it we should validate the wasm-bindgen version used when compiling a crate by checking the Cargo.lock file for the version used. We can then check against .crates.toml file which inventories the binaries installed by cargo. This file would exist either in the crate root (if --root is used) or would be located in CARGO_HOME. If the Cargo.lock version doesn't match the .crates.toml version then force install otherwise skip the install step.

I have done something similar here

@data-pup
Copy link
Member

@FreeMasen I was hoping to take care of this issue, if you don't mind. Some of this issue is carrying over from #51, which I opened a PR for back in April at #81. That issue was blocked while #115 was handled, but I can start working on this issue this week since we have a good strategy to solve this now.

If you feel strongly about it and would -really- like to get it done feel free, but there's also some other open issues tagged as 'Help Wanted' here.

@FreeMasen
Copy link

@data-pup Ok, thanks for letting me know.

If it is any help, I broke out the local package parts of cargo-upstall into a lib.rs in the same crate. I also added the ability to check both $CARGO_HOME/.crates.toml as well as $PWD/.crates.toml

@ashleygwilliams ashleygwilliams added PR attached there's a PR open for this issue and removed help wanted Extra attention is needed labels Jun 18, 2018
data-pup added a commit to data-pup/wasm-pack that referenced this issue Jun 19, 2018
data-pup added a commit to data-pup/wasm-pack that referenced this issue Jun 19, 2018
data-pup added a commit to data-pup/wasm-pack that referenced this issue Jun 19, 2018
data-pup added a commit to data-pup/wasm-pack that referenced this issue Jun 20, 2018
@alexcrichton
Copy link
Contributor

Currently when publishing wasm-bindgen I try to publish a new version of all the associated crates. All the wasm-bindgen crates also have = dependencies on all internal crates, so if you install wasm-bindgen-cli 0.2.5 you'll only get the 0.2.5 version of things and won't accidentally pick up the 0.2.6 versions if they're newer. In that sense if the wasm-bindgen library dependency is at 0.2.5 then, yes, it should be safe to install the 0.2.5 version of wasm-bindgen-cli and go from there.

That being said wasm-bindgen actually has support already for automatically detecting when it's digesting data it doesn't understand. This relies on manually updating a "schema version" every so often if necessary, however. This doesn't seem to be working out super well in practice because that check was botched originally I think and only recently got updated/fixed.

Despite that though it'd still be best if this were all automagically managed for you. One option as well is that wasm-pack could run cargo update -p wasm-bindgen --precise WASM_BINDGEN_VERSION with whatever wasm-bindgen is installed locally. That way you're guaranteed it'll always work and no need to rebuild?

@mgattozzi
Copy link
Contributor

I still think a sandboxed build tool rather than a globally namespaced one is the best way to go, because what about other projects using the same thing? what if they use a different version?

@alexcrichton
Copy link
Contributor

Currently at least there's not much reason to stick to an older wasm-bindgen version so in that sense it'd be fine to aggressively move it forward, but in the future it may be worthwhile to have per-project versions for sure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR attached there's a PR open for this issue question Further information is requested
Projects
None yet
6 participants