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

Static OpenSSL option for MacOS and Windows #284

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rustysec
Copy link

#278 take 2!

provides feature to opt in to static openssl linkage.

@alexcrichton
Copy link
Owner

Thanks!

Could a test on CI be added for this?

Also, I'm sort of losing track of all the features the curl-sys and curl crate has, would you mind writing up some documentation in either the README.md or in the manifest iteslf?

@rustysec
Copy link
Author

definitely, should be able to work on it later today or tomorrow.

Copy link
Owner

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Thanks! I think with an update to CI to exercise this it should be good to go

if let Some(path) = env::var_os("DEP_OPENSSL_INCLUDE") {
cfg.include(path);
}
println!("cargo:rustc-link-lib=ssl");
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be covered by openssl-sys right?

@rustysec
Copy link
Author

well i just discovered this pr actually isn't building right anyway. seems like without target specific features i probably won't be able to make this work without breaking changes. it's 100% possible i'm missing something obvious, however.

@alexcrichton
Copy link
Owner

Could that be fixed by perhaps just making openssl-sys always an optional dependency?

@rustysec
Copy link
Author

it does fix my build, but it also ends up compiling openssl for every target because ssl is on by default and openssl is a dependency of that. there doesn't seem to be a great way to handle all the conditions.

@alexcrichton
Copy link
Owner

Oh right :(

Hm I'm not sure how best to configure this in that case :(

@rustysec
Copy link
Author

i experimented locally with moving the current crate into a subfolder (curl-base) and then being able to use platform specific dependencies to turn the features on and off. there's just a one line lib.rs that re-exports the current crate. seems like i can get the same default behavior that way. it's a lot of re-organizing, not sure how you feel about that.

anyway, this whole effort may not be worth it once mesalink will build properly on windows.

@alexcrichton
Copy link
Owner

Hm yeah that may be a bit overkill for this, and if mesalink works that'd be great if that could be used!

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.

2 participants