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

chore(deps): Update Rust toolchain to 1.83.0 #22068

Merged
merged 16 commits into from
Jan 3, 2025

Conversation

jszwedko
Copy link
Member

Summary

Updates the Rust toolchain to 1.83.0 (latest)

Change Type

  • Bug fix
  • New feature
  • Non-functional (chore, refactoring, docs)
  • Performance

Is this a breaking change?

  • Yes
  • No

How did you test this PR?

make check-clippy

Does this PR include user facing changes?

  • Yes. Please add a changelog fragment based on our guidelines.
  • No. A maintainer will apply the "no-changelog" label to this PR.

Checklist

  • Please read our Vector contributor resources.
  • If this PR introduces changes Vector dependencies (modifies Cargo.lock), please
    run dd-rust-license-tool write to regenerate the license inventory and commit the changes (if any). More details here.

References

Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
Signed-off-by: Jesse Szwedko <[email protected]>
@jszwedko jszwedko added the no-changelog Changes in this PR do not need user-facing explanations in the release changelog label Dec 20, 2024
@jszwedko jszwedko requested a review from a team as a code owner December 20, 2024 22:36
@github-actions github-actions bot added domain: sources Anything related to the Vector's sources domain: sinks Anything related to the Vector's sinks domain: core Anything related to core crates i.e. vector-core, core-common, etc domain: vdev Anything related to the vdev tooling labels Dec 20, 2024
Copy link
Member

@pront pront left a comment

Choose a reason for hiding this comment

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

Thank you! This was a large one.

@@ -264,7 +264,6 @@ pub enum DeserializerConfig {
///
/// Going forward, Vector will use that [Go SDK][implementation] as the reference implementation, which means
/// the codec may continue to relax the enforcement of specification.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: should this be ///?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I don't think so. We want this to be a doc comment so that is extracted and rendered in the docs.

@@ -94,7 +94,7 @@ where

struct Visitor<'a>(MutexGuard<'a, String>);

impl<'a> field::Visit for Visitor<'a> {
impl field::Visit for Visitor<'_> {
Copy link
Member

Choose a reason for hiding this comment

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

✨ nice readability improvement

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I forgot to leave some comments about the clippy changes, but this was one of them about it being unnecessary to specify lifetimes in some places.

@@ -64,7 +64,7 @@ pub(crate) const fn align16(amount: usize) -> usize {
"`amount` must be less than `MAX_ALIGNABLE_AMOUNT`"
);

((amount + SERIALIZER_ALIGNMENT - 1) / SERIALIZER_ALIGNMENT) * SERIALIZER_ALIGNMENT
amount.div_ceil(SERIALIZER_ALIGNMENT) * SERIALIZER_ALIGNMENT
Copy link
Member

Choose a reason for hiding this comment

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

Note (no action need): interesting change, should be covered by existing tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm, yeah, I didn't even notice this one since clippy --fix did it.

@@ -239,7 +242,7 @@ impl GlobalOptions {
}
}

fn conflicts<T: PartialEq>(this: &Option<T>, that: &Option<T>) -> bool {
fn conflicts<T: PartialEq>(this: Option<&T>, that: Option<&T>) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

✨ nice

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. This is the clippy lint that required the most changes, but was mechanical: rather than a reference to an option use an option with a reference.

@@ -233,13 +233,16 @@ impl DatadogMetricsConfig {
}

fn build_client(&self, proxy: &ProxyConfig) -> crate::Result<HttpClient> {
let default_tls_config;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the following removes the need for this local variable:

    let tls_settings = MaybeTlsSettings::from_config(
        Some(
            self.local_dd_common
                .tls
                .as_ref()
                .unwrap_or_else(|| TlsEnableableConfig::enabled()),
        ),
        false,
    )?;

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried this first, actually, but the issue is that TlsEnableableConfig::enabled() returns an owned type rather than a reference which doesn't match the type of self.local_dd_common.tls.as_ref().

@@ -188,13 +188,16 @@ impl DatadogTracesConfig {
}

pub fn build_client(&self, proxy: &ProxyConfig) -> crate::Result<HttpClient> {
let default_tls_config;
Copy link
Member

Choose a reason for hiding this comment

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

Same for this one

Copy link
Member Author

@jszwedko jszwedko left a comment

Choose a reason for hiding this comment

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

Thanks for the review! I responded inline.

@@ -264,7 +264,6 @@ pub enum DeserializerConfig {
///
/// Going forward, Vector will use that [Go SDK][implementation] as the reference implementation, which means
/// the codec may continue to relax the enforcement of specification.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I don't think so. We want this to be a doc comment so that is extracted and rendered in the docs.

@@ -94,7 +94,7 @@ where

struct Visitor<'a>(MutexGuard<'a, String>);

impl<'a> field::Visit for Visitor<'a> {
impl field::Visit for Visitor<'_> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I forgot to leave some comments about the clippy changes, but this was one of them about it being unnecessary to specify lifetimes in some places.

@@ -64,7 +64,7 @@ pub(crate) const fn align16(amount: usize) -> usize {
"`amount` must be less than `MAX_ALIGNABLE_AMOUNT`"
);

((amount + SERIALIZER_ALIGNMENT - 1) / SERIALIZER_ALIGNMENT) * SERIALIZER_ALIGNMENT
amount.div_ceil(SERIALIZER_ALIGNMENT) * SERIALIZER_ALIGNMENT
Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm, yeah, I didn't even notice this one since clippy --fix did it.

@@ -686,20 +700,6 @@ async fn reader_throws_error_when_record_is_undecodable_via_metadata() {
static GET_METADATA_VALUE: AtomicU32 = AtomicU32::new(0);
static CAN_DECODE_VALUE: AtomicU32 = AtomicU32::new(0);

impl AsMetadata for u32 {
Copy link
Member Author

Choose a reason for hiding this comment

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

Clippy complained that the impl would be pulled up so better to define it outside.

@@ -239,7 +242,7 @@ impl GlobalOptions {
}
}

fn conflicts<T: PartialEq>(this: &Option<T>, that: &Option<T>) -> bool {
fn conflicts<T: PartialEq>(this: Option<&T>, that: Option<&T>) -> bool {
Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. This is the clippy lint that required the most changes, but was mechanical: rather than a reference to an option use an option with a reference.

@@ -233,13 +233,16 @@ impl DatadogMetricsConfig {
}

fn build_client(&self, proxy: &ProxyConfig) -> crate::Result<HttpClient> {
let default_tls_config;
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried this first, actually, but the issue is that TlsEnableableConfig::enabled() returns an owned type rather than a reference which doesn't match the type of self.local_dd_common.tls.as_ref().

@jszwedko jszwedko enabled auto-merge January 2, 2025 16:52
Signed-off-by: Jesse Szwedko <[email protected]>
@datadog-vectordotdev
Copy link

datadog-vectordotdev bot commented Jan 2, 2025

Datadog Report

Branch report: jszwedko/upgrade-rust-1.83
Commit report: af6a2e7
Test service: vector

✅ 0 Failed, 560 Passed, 0 Skipped, 4m 4.12s Total Time

@jszwedko jszwedko added this pull request to the merge queue Jan 2, 2025
Merged via the queue into master with commit ca084cc Jan 3, 2025
42 checks passed
@jszwedko jszwedko deleted the jszwedko/upgrade-rust-1.83 branch January 3, 2025 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: core Anything related to core crates i.e. vector-core, core-common, etc domain: sinks Anything related to the Vector's sinks domain: sources Anything related to the Vector's sources domain: vdev Anything related to the vdev tooling no-changelog Changes in this PR do not need user-facing explanations in the release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants