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

feat(quic): deprecate QUIC draft-29 version support #5786

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

tesol2y090
Copy link

@tesol2y090 tesol2y090 commented Dec 31, 2024

Description

Deprecate support_draft_version field from QUIC protocol.

resolves #3395

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@tesol2y090 tesol2y090 marked this pull request as ready for review December 31, 2024 14:04
Copy link
Member

@dariusc93 dariusc93 left a comment

Choose a reason for hiding this comment

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

LGTM. Left a small comment :)

@dariusc93 dariusc93 changed the title feat(quic): deprecate QUIC draft-29 version support chore(quic): deprecate QUIC draft-29 version support Jan 1, 2025
@dariusc93 dariusc93 self-requested a review January 1, 2025 04:27
Copy link
Member

@dariusc93 dariusc93 left a comment

Choose a reason for hiding this comment

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

Sorry, looks like you would need to add #[allow(deprecated)] to support_draft_29 too. Could you do that too? :)

@tesol2y090 tesol2y090 requested a review from dariusc93 January 1, 2025 04:46
@elenaf9
Copy link
Contributor

elenaf9 commented Jan 28, 2025

Friendly ping @tesol2y090. The CI still fails because #[allow(deprecated)] hasn't been added to all usages of support_draft_29 yet.

Also: could we use #[expect(..)] instead of #[allow(..)]? It does the same, but additionally warns us once it is not needed anymore.

@elenaf9 elenaf9 closed this Jan 28, 2025
@elenaf9 elenaf9 reopened this Jan 28, 2025
@tesol2y090 tesol2y090 requested a review from elenaf9 February 16, 2025 17:50
@tesol2y090 tesol2y090 requested a review from elenaf9 February 20, 2025 08:58
@@ -59,6 +59,7 @@ pub struct Config {
/// If support for draft-29 is enabled servers support draft-29 and version 1 on all
/// QUIC listening addresses.
/// As client the version is chosen based on the remote's address.
#[deprecated(note = "QUIC draft versions are no longer supported")]
pub support_draft_29: bool,
Copy link
Member

@jxs jxs Feb 20, 2025

Choose a reason for hiding this comment

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

we should probably add an allow on our usage of this field, in transports/quic/src/transport.rs:91 .
To allow CI to pass

Comment on lines +5 to +7
- Deprecate `Config::support_draft_29`.
See [PR 5786](https://github.com/libp2p/rust-libp2p/pull/5786).

Copy link
Contributor

Choose a reason for hiding this comment

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

libp2p-quic-v0.12.0 is already released. You need to bump the version of it to 0.12.1 and move this to a new section above.

Copy link
Contributor

@elenaf9 elenaf9 Feb 21, 2025

Choose a reason for hiding this comment

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

I mean the version of the libp2p-quic crate itself also needs to be bumped :)

@elenaf9 elenaf9 changed the title chore(quic): deprecate QUIC draft-29 version support feat(quic): deprecate QUIC draft-29 version support Feb 21, 2025
@tesol2y090 tesol2y090 requested a review from elenaf9 February 21, 2025 16:40
@tesol2y090 tesol2y090 requested a review from dariusc93 February 22, 2025 08:56
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.

quic: don't support draft versions
4 participants