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

clean up build warnings #244

Merged
merged 1 commit into from
Sep 15, 2021
Merged

clean up build warnings #244

merged 1 commit into from
Sep 15, 2021

Conversation

davepacheco
Copy link
Collaborator

@davepacheco davepacheco commented Sep 15, 2021

This change cleans up several warnings in the build.

This depends on #225. That one changes the toolchain, and so changes the warnings around.

@davepacheco davepacheco marked this pull request as ready for review September 15, 2021 22:48
@davepacheco davepacheco force-pushed the warning-cleanup branch 2 times, most recently from 5e987c9 to 93166a5 Compare September 15, 2021 22:52
@davepacheco davepacheco marked this pull request as draft September 15, 2021 22:55
@@ -1,6 +1,6 @@
//! Integration tests for oximeter collectors and producers.

mod common;
pub mod common;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason for this change is similar to what's described in rust-lang/rust#46379: some tests that use mod common use identity_eq function, but this one doesn't. So when Cargo builds this test, it reports a dead_code warning on identity_eq. Every other test file works around this kind of problem using pub mod common. Other solutions are possible (e.g., put identity_eq into a separate module that's only included where needed), but for now I'm doing the simple thing that's consistent with the other tests.

@davepacheco davepacheco marked this pull request as ready for review September 15, 2021 23:00
Copy link
Collaborator

@smklein smklein left a comment

Choose a reason for hiding this comment

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

... I feel bad; been ignoring these warnings when running Omicron tests for too long 😅

@bnaecker
Copy link
Collaborator

@smklein ditto! 😬

@davepacheco
Copy link
Collaborator Author

Thanks for the reviews! @jessfraz tells me to ignore the "403 Forbidden" error in the Docker build because it's a server error so I'm going to merge.

@davepacheco davepacheco merged commit 09f2d48 into main Sep 15, 2021
@davepacheco davepacheco deleted the warning-cleanup branch September 15, 2021 23:38
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.

3 participants