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

fix: Require tokio/rt feature for sync scenarios #192

Merged
merged 4 commits into from
May 22, 2023
Merged

fix: Require tokio/rt feature for sync scenarios #192

merged 4 commits into from
May 22, 2023

Conversation

flosse
Copy link
Member

@flosse flosse commented May 21, 2023

This should fix #191 but I'd feel better if there is a test/configuration where the CI detects such bugs.
@uklotzde do you have an idea how to provoke the issue?

@flosse flosse marked this pull request as ready for review May 21, 2023 21:39
@flosse flosse requested a review from uklotzde May 21, 2023 21:39
@uklotzde
Copy link
Member

Just fixing the issue without a regression test that triggers the dependency/feature error is brittle.

@uklotzde
Copy link
Member

cargo check --no-default-features --features rtu-sync fails.

Ideally, all or at least selected feature combinations should be checked by the CI builds. Using all possible combinations does not make much sense, but checking single features in isolation like demonstrated here would help.

@uklotzde
Copy link
Member

Another argument to avoid overusing features for crate extensions. Using separate sync and server crates would be more appropriate.

The sync stuff doesn't fit in here at all. Offering a sync-over-async bridge layer is pointless for an async-focused crate IMHO.

@flosse
Copy link
Member Author

flosse commented May 21, 2023

The sync stuff doesn't fit in here at all. Offering a sync-over-async bridge layer is pointless for an async-focused crate IMHO.

I agree!

@flosse flosse marked this pull request as draft May 22, 2023 00:14
@uklotzde
Copy link
Member

After force pushing GitHub noticed the new workflow action.

@uklotzde uklotzde marked this pull request as ready for review May 22, 2023 08:02
@uklotzde uklotzde merged commit 824143a into main May 22, 2023
24 checks passed
@uklotzde uklotzde deleted the fix-191 branch July 25, 2023 22:57
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

Successfully merging this pull request may close these issues.

Tokio module 'runtime' is private
2 participants