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

Adopt quicksink #4705

Closed
kayabaNerve opened this issue Oct 23, 2023 · 3 comments · Fixed by #4711
Closed

Adopt quicksink #4705

kayabaNerve opened this issue Oct 23, 2023 · 3 comments · Fixed by #4711

Comments

@kayabaNerve
Copy link
Contributor

Description

libp2p should inline quicksink into the rust-libp2p monorepo. libp2p is a notable consumer of quicksink with overlapping developers from the current organization responsible for it.

Motivation

Despite libp2p's active usage of quicksink, it hasn't been maintained since 2022 and has dated dependencies. While I would simply make a PR to quicksink to bump said dependencies, I can't as the repository is archived.

I did attempt to remove the reliance on quicksink as discussed a year and a half ago in #2653 (comment). Unfortunately, offering the Sink API around something without the Sink API requires effectively the entire quicksink library of code. Accordingly, there's no benefit to removing it vs adopting it.

The alternative would be to use future's unfold, dropping the ability to flush and deferring the close operation to Drop. While I can't claim experience with this level, that seemed unwise.

Requirements

Potentially the consent of the prior organization to accept the quicksink crate unless libp2p wants to inline it as a file within libp2p-websocket (the one consumer)/rename it to quicksink2.

Open questions

No response

Are you planning to do it yourself in a pull request ?

Maybe

@thomaseizinger
Copy link
Contributor

libp2p should inline quicksink into the rust-libp2p monorepo.

Is it used in several places? I am happy to have the code inlined into the usage site. It would be great if we don't have to add a crate to our workspace.

@kayabaNerve
Copy link
Contributor Author

kayabaNerve commented Oct 23, 2023

It's a single use to add Sink to soketto::Connection (meaning an alternative may be adding Sink directly to soketto::Connection?).

I'd guess the easiest option will be to the single-file crate into a single-file module, as inlining it seems to be redoing all of it. If you can write a tailored solution due to better understanding the problem space, I'd love to see it :)

@thomaseizinger
Copy link
Contributor

Inline the crate into a module is fine, then we have control over the dependencies :)

@mergify mergify bot closed this as completed in #4711 Oct 24, 2023
mergify bot pushed a commit that referenced this issue Oct 24, 2023
Adds the quicksink crate, which is offered under the MIT or Apache 2 license, as a module. This enables maintenance since quicksink itself has been archived for a year in a half.

Resolves #4705.

Pull-Request: #4711.
jxs pushed a commit to jxs/rust-libp2p that referenced this issue Jul 11, 2024
Adds the quicksink crate, which is offered under the MIT or Apache 2 license, as a module. This enables maintenance since quicksink itself has been archived for a year in a half.

Resolves libp2p#4705.

Pull-Request: libp2p#4711.
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