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

Fixed RUSTLS on Windows Compilation #456

Closed
wants to merge 1 commit into from
Closed

Conversation

mcrypwd
Copy link

@mcrypwd mcrypwd commented Nov 5, 2024

Migrated my project to windows and had issues compiling the underlying pingora lib's modifed the compilation of the impl UniqueID for TlsStream and it allowewd me to compile. I am still familiarizing myself with the code base so I am unsure if this is the best way to do this but it worked for me.

Copy link
Contributor

@fredr fredr left a comment

Choose a reason for hiding this comment

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

Added some nits to conform the cfg statements of the rest of the project (I'm not part of cloudflare so can't approve, but I made some related changes recently)

@@ -216,9 +216,15 @@ impl<T> UniqueID for TlsStream<T>
where
T: UniqueID,
{
#[cfg(not(target_os = "windows"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[cfg(not(target_os = "windows"))]
#[cfg(unix)]

To keep it consistent with the other cfg statements

fn id(&self) -> i32 {
self.tls.stream.as_ref().unwrap().get_ref().0.id()
}

#[cfg(target_os = "windows")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[cfg(target_os = "windows")]
#[cfg(windows)]

@johnhurt johnhurt self-assigned this Nov 15, 2024
@GREsau
Copy link

GREsau commented Nov 23, 2024

I don't think this change is necessary now that #471 / 1756948 is merged

@mcrypwd
Copy link
Author

mcrypwd commented Nov 25, 2024

Addressed with #471

@mcrypwd mcrypwd closed this Nov 25, 2024
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.

4 participants