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

Support fetching metadata from crate #1660

Open
bkchr opened this issue Jun 27, 2024 · 11 comments
Open

Support fetching metadata from crate #1660

bkchr opened this issue Jun 27, 2024 · 11 comments

Comments

@bkchr
Copy link
Member

bkchr commented Jun 27, 2024

The subxt macro currently supports to fetch the metadata from the filesystem or from some RPC node. However, for the future zombienet rust tests, it would be nice to also specify a crate.

So, something like this:

subxt(runtime_crate = "crate_name", wasm_file_name = "name", features = ["fast-runtime"])

The crate_name would be required to be path of the same workspace. wasm_file_name should be optional and if not given, it should be guessed based on the build output. If there is more than one wasm file, the macro should complain. features is also optional and just activates some feature at compile time.

https://github.com/paritytech/polkadot-sdk/tree/85f0edae1d6c461081f42fcabd13e547e5a5b683/substrate/.maintain/build-only-wasm.sh could be helpful here.

@jsdw
Copy link
Collaborator

jsdw commented Jun 27, 2024

From the linked issue, I like the idea that you can point Subxt at the WASM runtime and have it extract and use the metadata from it. Then, I think we could have:

  • runtime_metadata_path - get metadata from path
  • runtime_metadata_insecure_url - get metadata from (possibly not secure) URL
  • runtime_path - get metadata from runtime at path

I'd prefer to avoid the subxt macro having to know or care anything about the structure of the polkadot-sdk repo if possible :)

@bkchr
Copy link
Member Author

bkchr commented Jun 28, 2024

I'd prefer to avoid the subxt macro having to know or care anything about the structure of the polkadot-sdk repo if possible :)

I mean what I proposed does not require to know the structure of the repo? Only need to know how substrate-wasm-builder works, but this is reasonable.

@pepoviola
Copy link

Hi 👋, I think we want to pass the crate_name as @bkchr mention in order to compile on the fly to get the wasm to use. I currently add a similar logic to the build script in the pr but would be great to have this implemented in subxt.

Thanks!

@jsdw
Copy link
Collaborator

jsdw commented Jul 10, 2024

Since we need to extract metadata from the runtime WASM regardless, adding runtime_path as a first step seems sensible anyway, and then we can consider actually building the runtime and extracting the wasm from it via eg runtime_crate_name.

I do still think that building the WASM makes more sense as a separate step though, as there may in the future (if not already somewhere) different ways to configure the build for some runtime (eg via crate features or env vars), and so doing it separately means that Subxt doesn't also need to bake in any additional things to handle setting this sort of stuff.

I mean what I proposed does not require to know the structure of the repo?

Ah yeah, indeed, I was thinking down the wrong line when I wrote this! I guess it would just run a cargo build on some crate path and then look for the WASM. Needing a separate wasm_file_name is a bit ugly though so I still think that having the "build wasm file" as a separate step would be cleaner overall, and could exist in some build.rs with whatever custom command etc makes most sense!

@pkhry
Copy link
Contributor

pkhry commented Aug 21, 2024

I'd also prefer to keep wasm building and extract metadata from source steps to be separate due to the need to have caching for generated wasm files.

@bkchr
Copy link
Member Author

bkchr commented Aug 21, 2024

steps to be separate due to the need to have caching for generated wasm files.

What need? Can you clarify what you mean by this.

@pkhry
Copy link
Contributor

pkhry commented Aug 21, 2024

steps to be separate due to the need to have caching for generated wasm files.

What need? Can you clarify what you mean by this.

My thought process as follows:

  • we need to build the package
  • we can keep the produced .wasm files inside the target dir.
  • macro will be triggering rebuilds of the package if it's inside the workspace and some of it deps/source have changed, even if that doesn't change the produced metadata( I am probably very wrong here)

So the line of thinking I followed is that for development it's better to leave the decision whether or not to rebuild the package to the user of the library instead of baking in any assumptions inside the macro logic/rebuilding on every new change

But I can add a commit with package building tomorrow if need be

Edit:

Yeah, baking in build process can be fine for one shot stuff or dependencies, but I'd leave it as providing "runtime_crate_name" & "wasm_file_path" to not bake in any assumption on the location of produced artifacts.

Ie

  • only "wasm_file_path" we only look for the wasm file.
  • file_path + runtime_crate_name -> we do a build and then look for the files in the provided path

@bkchr
Copy link
Member Author

bkchr commented Aug 21, 2024

  • macro will be triggering rebuilds of the package if it's inside the workspace and some of it deps/source have changed, even if that doesn't change the produced metadata( I am probably very wrong here)

Yeah that should work.

However, we still need to trigger the build and ensure that the consuming crate it build after the wasm is build.

@pepoviola
Copy link

I think we still need to allow to set the features to provide more flexibility to build the wasm.

@pkhry
Copy link
Contributor

pkhry commented Aug 23, 2024

  • macro will be triggering rebuilds of the package if it's inside the workspace and some of it deps/source have changed, even if that doesn't change the produced metadata( I am probably very wrong here)

Yeah that should work.

However, we still need to trigger the build and ensure that the consuming crate it build after the wasm is build.

Okay, i've tried implementing the build step inside the macro and unfortunately it will deadlock due to the build lock on the target folder. commented out code inside the commit

So no cargo build calls inside proc-macro. I've tried searching for a way to communicate with cargo from within proc-macro crates but it seems only buildscripts have that capability.

I think we still need to allow to set the features to provide more flexibility to build the wasm.

even if it was possible to call cargo build from within the proc macro without issues features would only work for crates defined inside the same workspace and wouldn't work for crates defined as deps inside the manifest.

Middle ground proposal

support runtime_crate_name by using runtime_crate_name and wasm_file_dir to look for wasm artifacts generated by the crate inside given wasm_file_dir.

@jsdw
Copy link
Collaborator

jsdw commented Aug 23, 2024

I'm still of the opinion that supporting runtime_path in the Subxt macro and being able to point to a build runtime WASM file is sufficient here. Then, in CI, one can run a step to build whatever runtime(s) and place the WASM file(s) wherever Subxt is being pointed to. I don't really think that Subxt should be trying to invoke a potentially costly (and potentially deadlocky if in the same project) cargo build command itself.

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

No branches or pull requests

4 participants