Skip to content

[Feature Request] Move ephemeral server behind feature flag #559

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

Closed
h7kanna opened this issue May 25, 2023 · 7 comments · Fixed by #561
Closed

[Feature Request] Move ephemeral server behind feature flag #559

h7kanna opened this issue May 25, 2023 · 7 comments · Fixed by #561
Labels
enhancement New feature or request

Comments

@h7kanna
Copy link
Contributor

h7kanna commented May 25, 2023

Is your feature request related to a problem? Please describe.

As ephemeral_server is only used for testing, Can it be moved into a separate crate to reduce deps in the core and optimize compilation time for user code for using client and worker?

Describe the solution you'd like

Additional context

I am trying to optimize compilation time for worker code, and zip dependency of ephemeral_server is big contributor

@h7kanna h7kanna added the enhancement New feature or request label May 25, 2023
@h7kanna h7kanna changed the title [Feature Request] Move ephemeral server into a separate crate [Feature Request] Move ephemeral server into a separate crate or behind feature flag May 25, 2023
@cretz
Copy link
Member

cretz commented May 25, 2023

As ephemeral_server is only used for testing

It's not only used for testing. It's used by all official users of the core crate today in non-testing code. All SDKs that use core use this.

Having said that, if we think our official Rust SDK is going to put this server behind a feature flag anyways, I don't mind. Would like it to be a default on feature though. Would also support a different dependency/approach to unzip files if the dependency is that burdensome.

@h7kanna
Copy link
Contributor Author

h7kanna commented May 25, 2023

Oh, I am not aware that it is used in non-test code in other SDKs. I only looked at its usage in tests.
Ya, it being behind the default feature should work.

@h7kanna h7kanna changed the title [Feature Request] Move ephemeral server into a separate crate or behind feature flag [Feature Request] Move ephemeral server behind feature flag May 25, 2023
@h7kanna
Copy link
Contributor Author

h7kanna commented May 25, 2023

@cretz , I created PR to add the feature and made it default.

@cretz
Copy link
Member

cretz commented May 25, 2023

Oh, I am not aware that it is used in non-test code in other SDKs

https://typescript.temporal.io/api/classes/testing.TestWorkflowEnvironment, https://python.temporal.io/temporalio.testing.WorkflowEnvironment.html, and https://dotnet.temporal.io/api/Temporalio.Testing.WorkflowEnvironment.html are all powered by it.

Thanks for the PR! Will let @Sushisource review.

@h7kanna
Copy link
Contributor Author

h7kanna commented May 25, 2023

Seems like the change to add as 'default' does not work because of the missing cargo feature in stable Rust 😔

rust-lang/cargo#8366
rust-lang/cargo#5015

cargo tree --package=temporal-sdk-core --no-default-features --edges normal -Zpackage-features
error: the `-Z` flag is only accepted on the nightly channel of Cargo, but this is the `stable` channel
See https://doc.rust-lang.org/book/appendix-07-nightly-rust.html for more information about Rust release channels.

@h7kanna
Copy link
Contributor Author

h7kanna commented May 26, 2023

@cretz @Sushisource

Unfortunately, due to 'feature unification in cargo workspace', we cannot add a feature and add it in default and also can ignore the feature with 'no-default-feature' or 'default-feature = false' in a user crate.

I also created another PR #561 without default, But as mentioned above this will need changes to existing SDK code 😔.

Anyways, I am just putting it here if you want to consider this in the future as it is contributing significant time to compilation in Rust-based worker code.

Screenshot 2023-05-25 at 6 50 58 PM

@cretz
Copy link
Member

cretz commented May 26, 2023

This is quite a bit to just avoid the zip dependency. I wonder if there's another way we can tackle that dependency need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants