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

Default to async-std #129

Merged
merged 3 commits into from May 17, 2024
Merged

Default to async-std #129

merged 3 commits into from May 17, 2024

Conversation

sveitser
Copy link
Contributor

@sveitser sveitser commented May 6, 2024

There's no issue, based on discussion on zulip it should not be required for consumers of this crate to always set RUSTFLAGS.

Instead of checking for presence of async-std, tokio or flume specified with --cfg the crate now checks for presence of tokio or flume and defaults to async-std if neither of those are set.

This PR:

  • Change cfg directives to use async-std channels and executor if
    nothing is specified via rust flags.
  • Add new empty RUSTFLAGS matrix variant to CI jobs.
  • Add a justfile for local testing.
  • Update README.
  • Fix and simplify flake.nix.

This PR does not:

  • Fix all the outdated comments, for example about cargo features.

Key places to review:

The cfg directives.

How to test this PR:

For example with just test-all or the other added just recipes.

Things tested

Manually tested that setting setting channels to tokio without using the tokio executor still fails. E. .g. env RUSTFLAGS="--cfg async_channel_impl=\"tokio\"" cargo check

- Change cfg directives to use async-std channels and executor if
  nothing is specified via rust flags.
- Add new empty RUSTFLAGS matrix variant to CI jobs.
- Add a justfile for local testing.
- Update README.
@sveitser sveitser changed the title Ma/async std default Default to async-std May 6, 2024
@@ -41,7 +41,7 @@ opentelemetry-jaeger = { version = "0.19.0", features = [
], optional = true }
opentelemetry-aws = { version = "0.8.0", features = ["trace"], optional = true }

[target.'cfg(all(async_executor_impl = "tokio"))'.dependencies]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why these were all and if that was intentional.

Copy link
Contributor

@rob-maron rob-maron left a comment

Choose a reason for hiding this comment

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

Not sure on the Nix stuff, but it looks good to me.
One thing to look for when integrating upstream is we will have to change the flags there as well. Some repositories have cfg_attrs that map to tokio::main and async_std::main

@sveitser
Copy link
Contributor Author

I just made the more or less simplest nix flake that has everything this project needs. I tested it on NixOS and OSX. I didn't say anything because I thought it runs in the CI anyway and will be tested there but that workflow doesn't trigger on PRs.

Triggered it manually now: https://github.com/EspressoSystems/async-compatibility-layer/actions/runs/9028630166/job/24809455559

It does still fail (like on main) so I will fix that.

- Use `env` command to set rustflags, otherwise nix develops tries to
  run RUSTFLAGS=... as a command.
- Replace no longer necessary cancel-workflow-action with concurrency
  group.
@sveitser
Copy link
Contributor Author

@sveitser
Copy link
Contributor Author

Not sure on the Nix stuff, but it looks good to me. One thing to look for when integrating upstream is we will have to change the flags there as well. Some repositories have cfg_attrs that map to tokio::main and async_std::main

I think we should use async_main from this crate instead.

main as async_main,

Although this doesn't seem to play nicely with rust-analyzer. If I write async_main it suggests me to import async_std::main 😒

@sveitser
Copy link
Contributor Author

For tokio::main hotshot sometimes passes flavor = "multi_thread" and sometimes uses the default but multi_thread is the default according to https://docs.rs/tokio-macros/latest/tokio_macros/attr.main.html#multi-threaded-runtime, so I think this can be switched to async_main.

For tokio::test hotshot always use flavor multi_thread but sometimes customizes worker threads, setting it to 10 or 2.

@sveitser
Copy link
Contributor Author

@jbearer Do you have any thoughts on this matter? It seems that currently this crate does not provide all the features needed for compat between async-std and tokio in hotshot because in hotshot sometimes arguments are passed to tokio::test. Should we add functionality to async_test so that the user can pass arguments to tokio::test?

@sveitser sveitser merged commit 27473ac into main May 17, 2024
12 checks passed
@sveitser sveitser deleted the ma/async-std-default branch May 17, 2024 14:23
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.

None yet

2 participants