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

Move prototype rust sdk to separate crate #176

Closed
Sushisource opened this issue Aug 19, 2021 · 6 comments · Fixed by #252
Closed

Move prototype rust sdk to separate crate #176

Sushisource opened this issue Aug 19, 2021 · 6 comments · Fixed by #252
Assignees
Labels
enhancement New feature or request Rust SDK Issues about or asking for Rust SDK release

Comments

@Sushisource
Copy link
Member

We need to move this to its own crate, since it'll need to be one long run and to prevent accruing bad dependencies.

@tedbyron
Copy link
Contributor

@Sushisource tried separating crates, i'm getting one error in core_tests:

error[E0277]: the trait bound `CoreSDK: temporal_sdk_core::Core` is not satisfied
  --> src\core_tests\child_workflows.rs:32:42
   |
32 |     let mut worker = TestRustWorker::new(Arc::new(core), TEST_Q.to_string(), None);
   |                                          ^^^^^^^^^^^^^^ the trait `temporal_sdk_core::Core` is not implemented for `CoreSDK`
   |
   = note: required for the cast to the object type `dyn temporal_sdk_core::Core`

Is this because CoreSDK isn't pub?

There is a cyclical dependency between the prototype sdk and the core sdk (prototype sdk a dev-dep in core, required by core_tests), but it still compiles and can run cargo integ-test just fine. IDE won't be happy about the cyclical deps though. I think this can be fixed in the future by not using path dependencies, once api is more stable.

tedbyron/sdk-core@05a1b70

@tedbyron
Copy link
Contributor

I made CoreSDK pub(crate) and still getting the same trait bound error, I guess I am not sure what else to try as I don't have much experience troubleshooting trait object issues

@Sushisource
Copy link
Member Author

@tedbyron Thanks for giving this a shot! The reason you ran into that error is because of the dependency "cycle". The problem is described here: #220

I have no problem making CoreSDK pub, but as you found that's not the root issue here.

To really resolve this, it may need to be the case that all the tests which currently use the prototype SDK need to be lifted up and out of the core crate into the integ tests. The reason is that when you run unit tests, the crate is complied in dev mode, but when the prototype-sdk crate you just separated out is compiled, it is compiled in release mode. This is why the "cycle" is permitted, because technically they are totally different crates which is why you get weird type errors where the types look the same but aren't.

And then of course, those tests use a bunch of stuff like these mock pollers and history builders etc, and that'll have to be accessible, etc etc. It's a pretty big amount of stuff to shuffle around. Probably results in splitting core into more sub crates.

If you'd like to continue on the path of trying to get it going please let me know, I'm happy to answer questions etc. It's a pretty big chunk of work though for sure.

@tedbyron
Copy link
Contributor

@Sushisource definitely interested on working on that and maybe contribute to rust SDK in the future. I need to get a better understanding of the core first and then I can comment #220 when i give it a shot.

In the meantime I have fixed a bunch of clippy lints and refactor some code but wasn't sure if you would want PR or not... I know it's annoying to review something like this that doesn't really fix anything.

There are some good changes I think that make stuff more concise -- like using more Option methods, & and * instead of explicit .deref() or .deref_mut() which require namespacing ops::Deref[Mut], and .as_deref() instead of .as_ref().expect(...).deref() -- but there are also some changes that could be opinionated if you want to look.

diff: master...tedbyron:b34678ca2af4c62586f550739a21df83fe7c519e

@Sushisource
Copy link
Member Author

Sushisource commented Nov 17, 2021

@tedbyron Are you running clippy with additional lints turned on? I am liking most of what I'm seeing. I run clippy as part of CI so I'm surprised that there are lints getting triggered.

I would be super happy to review that diff as a PR, and we can upgrade the invocation here to include whatever additional lints you're running https://github.com/temporalio/sdk-core/blob/master/.buildkite/pipeline.yml#L20

I gave it a once-over and I like almost everything I see, maybe just a couple things I'd change.

Thanks so much for contributing this is great 🎉

@Sushisource
Copy link
Member Author

@tedbyron FYI this is done now -- it should be much easier to directly use the prototype SDK now. We're still not going to be releasing it anytime soon, but it can be played with now.

@Sushisource Sushisource self-assigned this Jan 7, 2022
@Sushisource Sushisource added the Rust SDK Issues about or asking for Rust SDK release label Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Rust SDK Issues about or asking for Rust SDK release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants