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

notify cargo to regenerate inclusion files when WASM changes #1262

Merged
merged 1 commit into from
Mar 19, 2023

Conversation

vedhavyas
Copy link
Member

This was not the case earlier since the inclusion file that contains WASM was not updated even when WASM was updated because the cargo was not watching the change to re-run the build script that actually updates the inclusion file.

Code contributor checklist:

This was not the case earlier since the inclusion file that contains WASM was not updated even when WASM was updated beacuse the cargo was not watching the change to re-run the build script that actually updates the inclusion file.
Copy link
Contributor

@liuchengxu liuchengxu left a comment

Choose a reason for hiding this comment

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

Could you share a bit more the story behind this? I'm curious what kind of issue leads you to this patch.

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Are you sure this is necessary? The only reason for that file to change is for code to change and dependencies to update. When dependencies change it will already trigger recompilation. What rerun-if-changed is typically used for is to reduce amount of recompilation, let's say, when code changes, but you know you don't need to recompile.

So I'm not yet convinced it is doing anything good. I'm also curious what issue you had exactly and why you think this fixes it.

This is only helpful if you modify WASM file manually after the fact, but we don't do that.

@vedhavyas
Copy link
Member Author

We hardcode the length of WASM in the bundle inclusion. Right now build.rs is not run since there is not notifier to watch for WASM changes once it has run first time or if the crate itself changes.

I found that, if I change something in the pallets, do a cargo clippy and then revert the change, and do a cargo clippy again, the build script is not triggered since it is neither the first run nor change in its crate. So bundle inclusion file is not updated. So there will be a length mismatch.

here is a sample error

     Compiling core-payments-domain-runtime v0.1.0 (/subspace/domains/runtime/core-payments)
     Compiling system-domain-runtime v0.1.0 (/subspace/domains/runtime/system)
  error[E0308]: mismatched types
   --> /subspace/target/debug/wbuild/system-domain-runtime/target/wasm32-unknown-unknown/release/build/system-domain-runtime-48451db30dbd7a03/out/core_payments_wasm_bundle.rs:7:61
    |
  7 | ... = *include_bytes!("/subspace/target/debug/wbuild/core-payments-domain-runtime/core_payments_domain_runtime.compact.wasm...
    |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected an array with a fixed size of 891945 elements, found one with 891944 elements

  For more information about this error, try `rustc --explain E0308`.
  error: could not compile `system-domain-runtime` due to previous error

With this patch, build will re-run the inclusion file when the WASM blobs changes.

@vedhavyas
Copy link
Member Author

I'm actually curious how @nazar-pc or @liuchengxu did not see this earlier 🤔
Makes me wonder how your workflow is 😄

@nazar-pc
Copy link
Member

It might depend on file system you're using. Cargo uses mtime to see if files changed. As https://doc.rust-lang.org/cargo/reference/build-scripts.html#rerun-if-changed says it is not additive, rerun-if-changed=build.rs will make it ONLY recompile when build.rs itself changes. So I'm worried that now you need to create more rules with rerun-if-changed to make up for reduced scope.

What were the exact steps you were doing to reproduce this? I'd like to experiment myself.

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

We actually typically combine this with Substrate's wasm building tooling, which issues some of these instructions to Cargo, which might be the reason why it requires more annotation like added here.

So I'm generally not opposed to having this overall, but I don't feel like I fully understood what happens in your specific case, I have not seen this before even though I recompile things with minor and major changes (something like subspace-core-primitives triggers a lot of recompilation) every day.

@vedhavyas
Copy link
Member Author

I would love to recreate this in your machine. This is what I pretty much did. I'm using clippy here but I think it happens with all the cargo commands.

  1. run clippy to success
  2. change something in the pallets under domains.
  3. then run clippy, sometimes I see the issue here
  4. if above was successful, then if I revert the change and run clippy again
  5. it fails here with that error if not already in previous step.

This is what I think is happening.

taken from rust lang

By default, Cargo will re-run the build script if any of the files in the package changes. Typically it is best to use the rerun-if commands, described in the [change detection](https://doc.rust-lang.org/cargo/reference/build-scripts.html#change-detection) section below, to narrow the focus of what triggers a build script to run again.

I think build script is not run after it was run first time if there no changes to the package. This is confusing, does package mean crate here or workspace. From the observations so far, it looks like its crate. So since the change was not in the crate, maybe build is not re-run and hence, bundle inclusion file is not regenrated with updated WASM. Would love to hear your results

@nazar-pc
Copy link
Member

Repeated a few times (I was running cargo clippy --all-targets, I use it all the time to make sure everything compiles, including tests, benches, etc.). Worked every time. I was adding a new public storage item in pallet-domain-registry.

I'm on Linux of course, might behave differently on other OSs.

@vedhavyas
Copy link
Member Author

alright, only difference I see is the OS. I'm gonna get this in. If you see any issues, we can revert

@vedhavyas vedhavyas merged commit aa21336 into main Mar 19, 2023
@vedhavyas vedhavyas deleted the runtime_wasm_change_trigger branch March 19, 2023 16:09
i1i1 added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Mar 20, 2023
i1i1 added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Mar 20, 2023
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.

3 participants