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

[BUG] - features are not additive #33

Closed
OvermindDL1 opened this issue Feb 23, 2023 · 3 comments · Fixed by #34
Closed

[BUG] - features are not additive #33

OvermindDL1 opened this issue Feb 23, 2023 · 3 comments · Fixed by #34
Assignees
Labels
bug Something isn't working

Comments

@OvermindDL1
Copy link

Description

Features are not additive, for example, if a dependency uses the 'async' functionality then it breaks the sync functionality

Steps to reproduce

Enable the async feature and try using the sync API

Expected behaviour

Features should be purely additive, they should not break API's otherwise.

Environment

  • OS: [e.g. GNU/Linux Debian 10] Debian 11
  • Architecture [Arm, x86_64, ...] x86_64
  • Rust version 1.67.1
  • library version 4.7.0
  • Protocol used FTP
  • Remote server version and name Not a clue, can't get it to compile because different deps are using it in different ways

Additional information

Features should only ever be additive. One library that's being used uses it with the sync API, another is using it via the async, yet the async runner here is tokio and on initial testing it shows there's some issues with things because it appears to be hardcoded to async-std (which is not well used in the ecosystem at this time)?

@OvermindDL1 OvermindDL1 added the bug Something isn't working label Feb 23, 2023
@veeso
Copy link
Owner

veeso commented Feb 23, 2023

Yes, I know, but there's no way to do that otherwise, so I don't care.

That's just a trip I mean. Why one should ever need both clients?
I know the cargo reference says that, but it's also true that the language doesn't allow me to do otherwise. I mean, I could rename the stream to asyncstream, but idk.

@OvermindDL1
Copy link
Author

OvermindDL1 commented Feb 23, 2023

Generally they are alternate types (or tag types) for the different types of interfaces yeah. Most commonly the community and rust authors would tend to design something like this like FtpStream<IO> or so, where the different implementations of IO (a sync, a tokio, and an async-std generally, if anyone bothers with async-std nowadays) expose the relevantly useful API's for their own use. Then usually some pub type SyncFTPStream = FtpStream<SyncFTPIO> and TokioFTPStream and AsyncStdFTPStream as well, though not always.

That's just a trip I mean. Why one should ever need both clients?

Oh I know, the program here is split among a lot of libraries that are combined together into a single tower server. One is using the sync API because other stuff they are doing is forced sync on them so why not, and the other is using the async api, but it apparently is causing yet other issues because it's using the mostly-dead async-std instead of tokio for some reason? Trying to convince the async one to switch to the sync since can't seem to get suppaftp to use tokio and just put it behind a spawn_blocking or so like the other person but finding free time to do that is always fun.


But anyway, features must always be additive (sync feature, tokio feature, async-std features sound best for here? perhaps with default having sync but that can still be opted out of), everything about cargo assumes that and it will break things if it's not except for the most trivial of programs.

@veeso
Copy link
Owner

veeso commented Feb 24, 2023

Okay then, I'll manage to fix all these things in the next version then. Thanks for reporting this info.

@veeso veeso linked a pull request Feb 24, 2023 that will close this issue
4 tasks
@veeso veeso closed this as completed in #34 Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants