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

Possiblity to disable dependency on futures? #73

Closed
Urhengulas opened this issue Jul 25, 2022 · 9 comments · Fixed by #74
Closed

Possiblity to disable dependency on futures? #73

Urhengulas opened this issue Jul 25, 2022 · 9 comments · Fixed by #74

Comments

@Urhengulas
Copy link

Hello,

Thank you for your library! We are using it in https://github.com/knurling-rs/probe-run and it comes in very handy 😁

I am just upgrading from 0.5.1 to 0.8.0 and am seeing that this adds the additional dependency futures, which in turn pulls in "futures-channel", "futures-core", "futures-executor", "futures-io", "futures-sink", "futures-task" and "futures-util". Which is a lot 😵‍💫

Since we are not using any async functions, is there the possibility to make this dependency optional by hiding it behind a (default) feature?

👋🏾

bors bot added a commit to knurling-rs/probe-run that referenced this issue Jul 27, 2022
334: Simplify snapshot tests r=justahero a=Urhengulas

This PR simplifies the probe-run snapshot tests by using the `rstest` macro. ~~Additionally we update the dependencies used in the tests.~~

# How to test the tests

These tests are ignored by default, since they require the hardware (`nrf52840dk`) to be present. You can run them locally by connecting your computer to the nrf52 and executing `cargo test -- --ignored`.

# Reviewer notes

Please don't get intimidated by the number of files and lines changed. Most of the files are just renamed to work with `rstest` and the new name of the test file. The many deleted lines are because the stack overflow test got simplified in the past, but the output wasn't adapted yet. You should mainly focus on `tests/snapshot.rs`.

# Remarks

Note that the `panic_verbose` test will fail in most cases. It's because this test output contains timing related numbers, which will slightly change from run to run. This also happened before this PR, therefore isn't a regression, but should be fixed in the future.

~~Note that updating `serial_test` pulls in many `futures-*` libraries, which seems unnecessary since we are not using any async functions. I've asked if that can be avoided: palfrey/serial_test#73

---

Edit(1): Drop dependency update from this PR, in order to unblock it.
Edit(2): Add reviewer notes.
Edit(3): Add section on how to tests the tests and structure the PR description.

Co-authored-by: Urhengulas <johann.hemmann@code.berlin>
Co-authored-by: Johann Hemmann <johann.hemmann@code.berlin>
@palfrey
Copy link
Owner

palfrey commented Jul 31, 2022

Previously, I would have said "no", because I seemed to recall trying to do this when implementing the async support to begin with and it didn't work. But I've just poked at things again and it looks doable. Should have a PR for this at some point...

@Urhengulas
Copy link
Author

Previously, I would have said "no", because I seemed to recall trying to do this when implementing the async support to begin with and it didn't work. But I've just poked at things again and it looks doable. Should have a PR for this at some point...

Thank you for taking this on! 🙇🏾‍♂️

@palfrey
Copy link
Owner

palfrey commented Aug 1, 2022

#74 is the start of things, and it probably just needs some minor more fixes to resolve out some cases of some particular combinations of features not working well together TBH.

@palfrey
Copy link
Owner

palfrey commented Aug 6, 2022

The optional-async branch from #74 LGTM, but if you could test this out with your use case please before I merge that?

@palfrey palfrey linked a pull request Aug 6, 2022 that will close this issue
@Urhengulas
Copy link
Author

The optional-async branch from #74 LGTM, but if you could test this out with your use case please before I merge that?

I will test it! Thank you!

@Urhengulas
Copy link
Author

I just tested it and now serial_test does not pull in all the futures-* dependencies anymore! Thank you!

I guess you could publish that as a patch release, since the async feature is enabled by default, right?

@palfrey
Copy link
Owner

palfrey commented Aug 10, 2022

I'm planning a 0.9.0 release soon with some other stuff and this will get folded into that.

@Urhengulas
Copy link
Author

I'm planning a 0.9.0 release soon with some other stuff and this will get folded into that.

Nice. Thank you very much!

@palfrey
Copy link
Owner

palfrey commented Aug 10, 2022

Released as part of 0.9.0

bors bot added a commit to knurling-rs/probe-run that referenced this issue Aug 12, 2022
345: Update dev-dependency `serial_test` r=Urhengulas a=Urhengulas

Blocked on palfrey/serial_test#73 getting released in `serial_test 0.9`.

Co-authored-by: Urhengulas <johann.hemmann@code.berlin>
bors bot added a commit to knurling-rs/probe-run that referenced this issue Aug 12, 2022
345: Update dev-dependency `serial_test` r=Urhengulas a=Urhengulas

Blocked on palfrey/serial_test#73 getting released in `serial_test 0.9`.

Co-authored-by: Urhengulas <johann.hemmann@code.berlin>
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 a pull request may close this issue.

2 participants