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

Use a hash as the SCHEMA_VERSION #2546

Merged
merged 1 commit into from
May 7, 2021

Conversation

chinedufn
Copy link
Contributor

Closes #2544

@chinedufn chinedufn force-pushed the schema-version-hash branch 6 times, most recently from 214d9e2 to 7cf11e0 Compare May 4, 2021 10:07
It is not recommended to install `wasm-bindgen-cli` as its version must match
_exactly_ the version of `wasm-bindgen` that is specified in the project's
cargo.lock file. Using `wasm-pack` for building simplifies the build process
It is not recommended to install `wasm-bindgen-cli` as its internal schema
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be fair, this is much less of an issue now so I think this warning can be lifted especially since the schema hasn't changed in over 6 months.

But I kept it intact for now in case you still want this here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually yeah would you be ok rewording these docs indicating that using the CLI directly is recommended nowadays? Given the current state of things I think it'd be fine to either remove references to wasm-pack or mention it only in a subsection below perhaps too

Copy link
Contributor Author

@chinedufn chinedufn May 4, 2021

Choose a reason for hiding this comment

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

Cool, I'll remove all wasm-pack recommendations for build related things.

While we're at it, I think we can replace the wasm-pack test recommendation and just include an example of how to download the webdriver yourself.

I'll go ahead and do that, but if you happen to see this before I dive in and you think it's a bad idea just let me know.

I'll also include a an example of setting the runner with an environment variable instead of needing to manage the config file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that we're no longer recommending wasm-pack I don't think this quick little hash solution is good enough anymore. A little too brittle.

For example, if rustfmt got a new rule and we ran cargo fmt and the schema file got touched we'd have a new version for no good reason.

I'm going to change the approach to something along the lines of:

  1. Generate a SCHEMA_HASH in the build script.
  2. Set SCHEMA_VERSION by hand to the current Cargo.toml version
  3. Add a test case to the bottom of shared/src/lib.rs that asserts that the current hash is equal to the last human approved hash.
  4. Whenever the hash changes tests will fail. Then a human can either decide to keep the SCHEMA_VERSION the same, or bump it to the latest Cargo.toml version if the schema has indeed changed. This would be documented above the test.

For my own needs just bumping versions every now and then would've been fine, even if sometimes unnecessary.

I'm only going this far because a lot of people use wasm-bindgen so I don't want the unnecessary churn across the entire ecosystem.

The most robust would probably be to pull the shared_api! macro out into a crate, have the shared crate re-export it and have the shared/build.rs depend on that shared_api! macro directly along with the encode_api! and decode_api! macros and run them all, hash the results and then use that as our source of truth hash.... But I'll hold off on that since the approach suggested above seems like it will meet our needs in practice just fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

For changes to testing, sounds good! For human verification the hash changes, also sounds good!

I'm fine for restructuring things too, but it's probably not too necessary to go too overboard. Honestly things don't change enough nowadays that manually changing the schema version/hash is probably fine.

@@ -1457,15 +1457,16 @@ fn extract_programs<'a>(
"

it looks like the Rust project used to create this wasm file was linked against
a different version of wasm-bindgen than this binary:
version of wasm-bindgen that uses a different bindgen format than this binary:

rust wasm file: {}
this binary: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW This was intended to be a human readable error message where the versions here were human readable and could give an indicator of what to do. With the versions being hashes now, though, this message isn't quite as appropriate as it was before. Perhaps these indicators could be less emphasized? Or could the version still be serialized somewhere and it's only used for these purposes?

Mostly I'm just thinking that hashes shouldn't be too prominently displayed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, I forgot that the versions are now hashes when I was editing this.

I'll expose the Cargo.toml version in the schema so that be used here and reword the message to more or less suggest that the easiest solution is to make the CLI and wasm-bindgen versions match.


rust wasm file: {}
this binary: {}
rust wasm file schema version: {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose this could be more descriptive and make it clear why the schema version is different from the actual version of the crate/binary that you're using, but in practice since the schema seems to be fairly stable at this point I don't think we need to come up with the perfect language here.

@chinedufn
Copy link
Contributor Author

@alexcrichton alright I edited some sections in the guide to focus on wasm-bindgen-cli, but I didn't get all of them. Notably the testing section still revolves around wasm-pack.

I think that these remaining sections can just be refreshed over time.

Otherwise, I think we're good to go here, so just let me know if there's anything wrong or missing and I'll get it fixed.

@chinedufn chinedufn force-pushed the schema-version-hash branch 2 times, most recently from 7150527 to e71871e Compare May 6, 2021 16:35
@chinedufn
Copy link
Contributor Author

Hmm the failing UI tests don't seem related to my changes but I see that tests are passing on the master branch.

Here's an example failure:

EXPECTED:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error[E0277]: the trait bound `std::result::Result<wasm_bindgen::JsValue, wasm_bindgen::JsValue>: FromWasmAbi` is not satisfied
 --> $DIR/missing-catch.rs:6:9
  |
6 |     pub fn foo() -> Result<JsValue, JsValue>;
  |            ^^^ the trait `FromWasmAbi` is not implemented for `std::result::Result<wasm_bindgen::JsValue, wasm_bindgen::JsValue>`
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈

ACTUAL OUTPUT:
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
error[E0277]: the trait bound `Result<wasm_bindgen::JsValue, wasm_bindgen::JsValue>: FromWasmAbi` is not satisfied
 --> $DIR/missing-catch.rs:6:9
  |
6 |     pub fn foo() -> Result<JsValue, JsValue>;
  |            ^^^ the trait `FromWasmAbi` is not implemented for `Result<wasm_bindgen::JsValue, wasm_bindgen::JsValue>`
┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈
note: If the actual output is the correct output you can bless it by rerunning
      your test with the environment variable TRYBUILD=overwrite

Looks like the actual output isn't using the fully qualified path for std::result::Result.

@alexcrichton
Copy link
Contributor

For the UI test failures are you using the same compiler version? A release just happened and I think it's using stable rustc, which often tweaks error messages slightly. I can also fix this up after merging.

@alexcrichton
Copy link
Contributor

Thanks!

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.

Relax same schema version requirement between wasm-bindgen-cli and wasm-bindgen
2 participants