Skip to content
This repository has been archived by the owner on Dec 11, 2024. It is now read-only.

Fix Cyclic Dependency #42

Merged
merged 17 commits into from
May 30, 2024
Merged

Fix Cyclic Dependency #42

merged 17 commits into from
May 30, 2024

Conversation

afsalthaj
Copy link
Contributor

@afsalthaj afsalthaj commented May 28, 2024

Fixes #40

https://www.notion.so/golemcloud/StubGen-Cyclic-Dependency-06394d8825604222a9aaeae5905ab84b

It unblocked golem-timeline case, and seem to be working

  • More testing (details in above doc)

    • 1.Direct Cyclic Dependency Case 👍 - i.e, x -> x
    • 2.Normal Dependency Case - i.e, x -> y
    • 3. Mix of 1 and 2. Add x -> x on top of a workspace that's already having x -> y
      Note: Makefile.toml overwrites some steps when initialising again with new target. So when testing make sure all steps are there. Regardless x -> y and x -> x works.
    • The normal scenario such as x -> y while y -> z, all works as before
    • Also this: A workspace with y -> z was added with x -> y to later bring in x -> x dependency and it worked. Clone this branch if you want to test yourself
    • The testing can extend as long as we want. I believe this is a better version already.
  • Gather input from team based on docs

  • If all good, complete the remaining code (you can see that it's incomplete, and just a prototype with only record type)

  • Code cleanup

Tests performed in my personal test project

https://github.com/afsalthaj/testwasmrpc/tree/only_caller

   Compiling golem-wasm-rpc v0.0.0 (/Users/afsalthaj/github/wasm-rpc/wasm-rpc)
   Compiling caller v0.0.1 (/Users/afsalthaj/github/testwasmrpc/caller)
   Compiling caller-stub v0.0.1 (/Users/afsalthaj/github/testwasmrpc/caller-stub)
    Finished dev [unoptimized + debuginfo] target(s) in 3.18s
    Creating component /Users/afsalthaj/github/testwasmrpc/target/wasm32-wasi/debug/caller.wasm
    Creating component /Users/afsalthaj/github/testwasmrpc/target/wasm32-wasi/debug/caller_stub.wasm

@@ -33,13 +44,61 @@ pub fn generate_stub_wit(def: &StubDefinition) -> anyhow::Result<()> {
.interfaces
.iter()
.flat_map(|i| i.imports.iter())
.collect::<IndexSet<_>>();
.collect::<Vec<_>>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh oh! I Let me fix this part

Copy link
Contributor

Choose a reason for hiding this comment

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

will you change it back to IndexSet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was1 more field I added and TypeDef didn't have Hash or something. May be that's why I changed it. I will definitely make it Set. But I will get back to this in a couple of hours.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will change it

@afsalthaj
Copy link
Contributor Author

afsalthaj commented May 29, 2024

Fixed the following today (based on some test results):

  • Avoid cargo getting updated with cyclic dependents. There was 1 more that I had to
  • Import statement of original stub in the original WIT had to be discarded when getting copied to stub during regeneration.
  • Make sure we properly select the original world that was selected for stub generation. Every error message is so hard to understand
  • Adding support for more types

@afsalthaj afsalthaj marked this pull request as ready for review May 29, 2024 13:52
@afsalthaj
Copy link
Contributor Author

afsalthaj commented May 29, 2024

Found a small mistake: for normal dependency after 5bd0d9d. Will fix it straight up

[cargo-make] INFO - Execute Command: "/Users/afsalthaj/github/golem/target/debug/golem-cli" "stubgen" "add-stub-dependency" "--stub-wit-root" "callee-stub/wit" "--dest-wit-root" "caller/wit" "--overwrite" "--update-cargo-toml"
[/Users/afsalthaj/github/wasm-rpc/wasm-rpc-stubgen/src/lib.rs:291:5] world_name.clone() = "callee"
[/Users/afsalthaj/github/wasm-rpc/wasm-rpc-stubgen/src/stub.rs:56:9] "here?" = "here?"
Error: no world named `callee` in package

wasm-rpc-stubgen/src/wit.rs Outdated Show resolved Hide resolved
wasm-rpc-stubgen/src/wit.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@vigoo vigoo left a comment

Choose a reason for hiding this comment

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

Looks good, I asked for a rename, and there was a IndexedSet->Vec change that may affect things (introducing duplicates).

I'm also not very happy about that regex but we can keep for now.

Once these are fixed good to merge

@afsalthaj
Copy link
Contributor Author

Looks good, I asked for a rename, and there was a IndexedSet->Vec change that may affect things (introducing duplicates).

Yes, it was noted by me. Will try to fix it.

@afsalthaj afsalthaj changed the title Fix Cyclic Dependency (WIP, Tests in Progress) Fix Cyclic Dependency May 30, 2024
@vigoo vigoo merged commit 7f393fb into main May 30, 2024
2 checks passed
@vigoo vigoo deleted the cyclic_dep_try_2 branch May 30, 2024 13:21
@afsalthaj afsalthaj mentioned this pull request Jun 9, 2024
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Circular dependency case by inlining the definitions in the stub generator
2 participants