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

Add feature for including openssl-sys #29

Closed
wants to merge 2 commits into from

Conversation

Raniz85
Copy link

@Raniz85 Raniz85 commented Apr 17, 2020

Added a new feature (openssl-static) that includes the openssl-sys crate
to fix static builds.

Renamed src/lib.rs to src/bindgen.rs and added a new lib.rs that
re-exports everything from bindgen.rs to prevent future bindgen runs
from reverting this change.

This fixes #25

Added a new feature (openssl-static) that includes the openssl-sys crate
to fix static builds.

Renamed src/lib.rs to src/bindgen.rs and added a new lib.rs that
re-exports everything from bindgen.rs to prevent future bindgen runs
from reverting this change.

This fixes sgrif#25
@golddranks
Copy link
Contributor

I think #25 was missing a reliable reproduction; here is one: https:/golddranks/pq_link_test

I built it against your fork of pq-sys, and indeed, adding the feature openssl-static fixes the build.

Cargo.toml Outdated Show resolved Hide resolved
Now requires a version between 0.9.0 and 0.11.
@Raniz85
Copy link
Author

Raniz85 commented Sep 21, 2021

Any chance of merging this?

@weiznich
Copy link
Collaborator

@Raniz85 I got recently the rights to merge PR's in here, but I want to do a check of the current status of the crate first before merging anything. Unfortunately I haven't found the necessary time yet.

@weiznich
Copy link
Collaborator

While thinking a bit more about this PR: What's the expected use case for this? You mention fixing static builds, but this change on your own won't help you there much as you need to enable the corresponding feature anyway. Most people will likely use pq-sys through diesel that means that with this change you need to add pq-sys to your dependencies + enable the corresponding. feature. Without this feature you would need to add openssl-sys to your dependencies and be done. I must say that I don't really get what's then the advantage of having this feature here.

@Raniz85
Copy link
Author

Raniz85 commented Sep 22, 2021

Without this feature you would need to add openssl-sys to your dependencies and be done.

It's been more than a year since I worked on this so my memory is hazy, but if I remember correctly, just adding a dependency on openssl-sys isn't enough, the dependency (or rather the extern crate openssl_sys) must come from pq-sys so that we can guarantee that openssl-sys is linked before pq-sys.

If the order is reversed, compilation will fail.

Read the linked issue and the issues referenced there, they contain quite a lot of information and debugging.

@golddranks
Copy link
Contributor

I can confirm that there were certainly problems with the linking ordrer. What made them hard to deal with was that they were somewhat non-deterministic.

@Raniz85
Copy link
Author

Raniz85 commented Jan 19, 2022

@weiznich Any progress on this? Either reject it or merge it please

@weiznich
Copy link
Collaborator

@Raniz85 I had no time to look into this yet, so there is no progress here. To be clear even to decide if this should be rejected or not I need time to understand the underlying problem. Unfortunately I cannot say when I will find the required time to look into this.

@weiznich
Copy link
Collaborator

weiznich commented Dec 7, 2023

Closed in favor of #46

@weiznich weiznich closed this Dec 7, 2023
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.

Static linking needs to include openssl somehow.
3 participants