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

Handle versions of wasm-bindgen in lockfiles #270

Closed
mgattozzi opened this issue Aug 25, 2018 · 13 comments
Closed

Handle versions of wasm-bindgen in lockfiles #270

mgattozzi opened this issue Aug 25, 2018 · 13 comments
Labels
feature request PR attached there's a PR open for this issue to-do stuff that needs to happen, so plz do it k thx
Milestone

Comments

@mgattozzi
Copy link
Contributor

💡 Feature description

We have a basic implementation of keeping up to date with wasm-bindgen. Ideally though with the local install we'd tie the version to whatever is in the lock file to avoid issues with things like Cargo.toml using wasm-bindgen = 0.2 as what version of wasm-bindgen-cli does this mean? Having a lock file look up would solve this issue.

@fitzgen
Copy link
Member

fitzgen commented Aug 27, 2018

Finding the lock file can be interesting if the crate is in a workspace. We can leverage https://crates.io/crates/cargo_metadata to help.

@mgattozzi
Copy link
Contributor Author

So would the metadata output the current version in the lockfile? Or what's in Cargo.toml?

@fitzgen
Copy link
Member

fitzgen commented Aug 28, 2018

It gives us the location of the workspace root, which will have the Cargo.lock in it: https://docs.rs/cargo_metadata/0.6.0/cargo_metadata/struct.Metadata.html#structfield.workspace_root

I don't think we want to use cargo_metadata::Dependency; I think it is the Cargo.toml's dependency, not the Cargo.lock's.

@data-pup
Copy link
Member

Digging into this now 😸

https://github.com/oli-obk/cargo_metadata/blob/master/src/lib.rs#L156

It does look like the Cargo.toml dependency is what is given, rather than the lockfile. This crate should still be useful for finding the lock file though :)

@fitzgen
Copy link
Member

fitzgen commented Aug 30, 2018

Yeah, I think we only want to use it in order to get the workspace root.

@data-pup
Copy link
Member

Sounds good to me :o) I'll follow up if I have other questions!

@ashleygwilliams ashleygwilliams added help wanted Extra attention is needed to-do stuff that needs to happen, so plz do it k thx feature request labels Sep 6, 2018
@data-pup
Copy link
Member

data-pup commented Sep 6, 2018

I think I have a lot of the work on this done 😸 I should be able to open a PR shortly.

As an aside, is there anywhere where the fields in Cargo.lock are officially specified? The closest I could find was this, in the Cargo book.

@fitzgen
Copy link
Member

fitzgen commented Sep 6, 2018

I think @ashleygwilliams or @alexcrichton would know the answer to that question.

@alexcrichton
Copy link
Contributor

Ah they're not necessarily officially specified but when using TOML+Serde you can deserialize into a structure like:

#[derive(Deserialize)]
struct Lockfile {
    packages: Vec<Package>,
}

#[derive(Deserialize)]
struct Package {
    name: String,
    version: String,
    source: Option<String>,
}

and that should be enough to match on wasm-bindgen!

@data-pup
Copy link
Member

data-pup commented Sep 6, 2018

Yay! Thanks for the help @alexcrichton, that lines up with what I had done so far. My struct looked like this:

#[derive(Deserialize)]
struct Package {
    name: String,
    version: String,
    source: Option<String>,
    dependencies: Option<Vec<String>>
}

Should I keep the dependencies field out of this, following your example?

@alexcrichton
Copy link
Contributor

Ah yeah it won't hurt anything to keep dependencies there, but it should be fine to leave out in that it'll still deserialize successfully

@data-pup
Copy link
Member

data-pup commented Sep 6, 2018

Awesome, thank you!

@ashleygwilliams
Copy link
Member

closed by the last release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request PR attached there's a PR open for this issue to-do stuff that needs to happen, so plz do it k thx
Projects
None yet
5 participants