stellar / soroban-examples

Example Soroban Contracts
Apache License 2.0
65 stars 68 forks source link

Fixing the dependencies between some of the examples #229

Closed anupsdf closed 1 year ago

anupsdf commented 1 year ago

What

Some examples have dependencies between them which was not specified in Cargo.toml files.

Why

The first time the wasms were build from the root directory of soroban-examples, there would be errors because of the dependencies. Adding the dependencies explicitly fixes this issue.

Command run to test the fix:

cargo build --target wasm32-unknown-unknown --release
dmkozh commented 1 year ago

Nice find. 👍

I'm surprised we are able to reference these crates as dependencies. The reason I'm surprised is that the contract crates have their crate-type as only cdylib in their Cargo.toml, and don't include lib or rlib. I've seen cases where you can't import an only-cdylib. (We don't lib rlib as a crate-type because multi-crate-type packages don't support some optimizations and then we end up with large .wasm files.)

If it works for the examples 👍. We might find it stop working at some point if our examples change or if we add a new example that happens to do whatever causes it to no longer work. There's an unstable cargo feature (artifact-dependencies) for supporting dependencies to other crates artifacts which will cause them to build in order without having to import them. We can switch to that if it stabilizes.

I'd be surprised if that works as expected, as the dependency contracts and user contracts typically are built with different targets (wasm for dependencies, x86 for the tests).

The artifact dependency would be great though.

anupsdf commented 1 year ago

Thanks for the suggestions @leighmcculloch and @dmkozh ! What I noticed with my dependency change here is that running cargo build from within the contract_b directory (cross_contract example) would only create the wasm file for contract_b and not the contract_a that it depends on. I found that a bit strange and thought cargo must be making some assumptions and moving on with the build. However, if I don't have this dependency in the Cargo.toml of contract_b then cargo build would throw an error. I will do some more research into this and also look at the unstable feature artifact-dependencies.

anupsdf commented 1 year ago

Here is what I have found so far,

  1. We already know that artifact-dependency will have to wait until it goes to stable channel from nightly channel.

  2. cargo build should only be run from top level since our soroban-examples are organized as members of a virtual workspace that are meant to be managed together.

  3. Current dependencies in soroban-examples are only on the wasm files that we are importing using SDK API. Is there a way to test the x86 target for tests? Running cargo test from top level passes only when its run after cargo build with the changes from this PR. I guess that's because its only looking for wasm files in the tests and not the x86 targets.

    soroban_sdk::contractimport!(
        file = "../../target/wasm32-unknown-unknown/release/soroban_cross_contract_a_contract.wasm"
    );
  1. More on path dependencies between workspace members here.

Should we proceed with the current path dependency? Or is there another way to get the top level cargo build to work?

On a side note, I wonder if soroban_sdk::contractimport should supports importing a revision number of the wasm. With upgradable contracts I imagine users can use a revision.

@dmkozh , @leighmcculloch

leighmcculloch commented 1 year ago

I think we should pass on the path dependency, and instruct folks to run make build before doing other things. Then once artifact dependencies stabilize, use it.

I think it would also be reasonable to remove the virtual workspace and include instructions for each example for how to build it. The ones that are dependent on others would include an instruction to build those other contracts first.

anupsdf commented 1 year ago

I think we should pass on the path dependency, and instruct folks to run make build before doing other things. Then once artifact dependencies stabilize, use it.

I think it would also be reasonable to remove the virtual workspace and include instructions for each example for how to build it. The ones that are dependent on others would include an instruction to build those other contracts first.

Sounds good! That way users can go to one example at a time and build them while learning. I will create a new PR to remove the virtual workspace. Closing this PR.

leighmcculloch commented 1 year ago

I will create a new PR to remove the virtual workspace.

This might break the gitpod setup, but that should be trivial to fix. Fyi as this may not be obvious as it won't cause any tests to break. If you open a PR with the change, gitpod will drop a link in the PR and clicking it should verify if anything is broken.