Make usage of openSSL optional

[stormshield-gt]

Add the possibility to disable the usage of openssl when building libpq from source.

[weiznich]

Thanks for opening this PR. Could you explain the motivation for why the usage of openssl should be disabled by default? I believe that's the wrong choice for most of our users as that disables support for SSL/TLS postgres connections in libpq.

[stormshield-gt]

Thanks for the review.

Could you explain the motivation for why the usage of openssl should be disabled by default?

It was for not adding an extra feature flag into diesel. User who wanted to add openssl would just have to add pq-src with the with-openssl feature to their crate dependencies:

[dependencies]
pq-src = { version = "0.3.0", features = ["with-openssl"] }

But I guess I was wrong as pq-src is only a dependency of diesel if the __with_asan_tests feature is enabled. So downstream users are free to add pq-src to their dependencies and select the features they like

I believe that's the wrong choice for most of our users as that disables support for SSL/TLS postgres connections in libpq.

All right, I enabled the support for openssl by default then.

[weiznich]

I'm sorry but that won't work as cargo unifies features across your dependency graph. So if you depend on pq-sys with the bundled feature enabled that will automatically enable the default features for pq-src as well (as we have no default-features = false there).

Thinking a bit more about that and given that we want to have openssl by default I think the best solution here is to have a disable-openssl feature on pq-src that can be explicitly enabled and that disables using openssl. The documentation for that feature should be explicit about the fact that this will disable support for SSL/TLS based postgres connections, so this feature likely will break things with non-local postgres installations.

[stormshield-gt]

I believe the feature unification problem can also happen with the disable-openssl feature.

• if a dependency activates the disable-openssl feature, someone could lose the support for TLS when compiling pq-src
• if a dependency activates the with-openssl feature or use default-feature or use the bundled feature of pq-sys, someone could become to compile openssl

In my opinion, it's better to accidentally gain support for TLS than to lose it.
Anyway, I don't feel strongly about this and would be happy to add a disable-openssl feature.

[weiznich]

You are correct that the same unification problem still exists with the disable-openssl approach. That written: I would expect that this is a option that is only set by a top level application crate that want's to control the whole dependency chain. I do not expect that libraries will enable this feature.

In my opinion, it's better to accidentally gain support for TLS than to lose it.

Well that really depends on your motivation. I just know that having an enabled by default feature would be essentially impossible to disable for any user and that an opt-in feature (with-openssl) would require that the vast majority of users explicitly enable that feature. Therefore an explicit opt out seems in my opinion like the best solution.

[stormshield-gt]

I just know that having an enabled by default feature would be essentially impossible to disable for any user

Right now as diesel don't use pq-src directly on regular dependency this is OK, but I agree if a crate that depends on pq-src/pq-sys and is not that cautious, that would be a problem. Basically, a PR would be need to be made to not do so, or to re-export a feature to be able to disable openssl.

Therefore an explicit opt out seems in my opinion like the best solution.

After reading your comment, I also think this might be the best solution, but I don't see how I would implement this, because there is AFAIK no way to disable a dependency based on a feature flag.

So if the disable-openssl feature is activated, I can't disable the dependency on openssl-sys.

I see several solutions

• accept the dependency on openssl-sys even with the disable-openssl feature is activated
• let the current implementation as it works with diesel, but might require other PR to other projects based on the needs

[weiznich]

Ah, yes I totally missed that. This makes things even worse :(

At that point: Mind explaining why you want to build libpq without openssl support at all? Maybe the underlying problem can/should be addressed in a different way?

[stormshield-gt]

At that point: Mind explaining why you want to build libpq without openssl support at all? Maybe the underlying problem can/should be addressed in a different way?

Basically, it's to optimize compile time. We only use diesel in a safe environment that doesn't need HTTPS and so to compile the whole openssl library. On average it adds 1m30 on a compilation from scratch.

Since #60 we are not obligated to compile openssl, but sadly a lot of package managers only provide the version 1 of openssl and not the 3, so it's a pain to get the right version of it in every building environment. If we could just disable openssl it would be perfect, as we rather not pay for a functionality we don't use. Besides the original libpq already provides this functionality, so I guess it makes sense to just reexport it.

Regarding the two proposed solutions:

• openssl opt-out through no-default-features = false (current): after analyzing the download on crate io (here and here), diesel is the only consumer of pq-sys. So the current solution works for most of the use cases.
• openssl opt-out through a feature (disable-openssl): I've made a separate commit that implement that version so we can easily compare. As openssl-sys only contains a bunch of symbols, I think most of it will probably be discarded by the linker, so I guess it's OK.

In any case, thanks for taking the time to discuss it

[weiznich]

Thanks for clarifying. Just to double check: Disabling openssl does then not run the build script anymore, right?

[weiznich]

I think we also need to put a #[cfg(not(feature = "disable-openssl")] on top of the extern crate openssl_sys; statement here: https://github.com/sgrif/pq-sys/blob/master/pq-src/src/lib.rs#L1 to prevent linking openssl if we don't need it.

[weiznich]

This comment should be more explicit about what are the implications of enabling this feature. It should more or less literally say, that this disables the SSL support in libpq, so that you cannot connect to database requiring SSL anymore.

[stormshield-gt]

Thanks for the suggestion, I reworded the comment

[stormshield-gt]

Thanks for the review, indeed the build script is still run even with

#[cfg(not(feature = "diable-openssl")]
extern crate openssl_sys

So the only solution is to make this opt-out through default-features = false. I've added a feature bundled_without_openssl to pq-sys, mirroring the vendored feature of openssl-sys. I've also added some CI jobs to avoid regression like the one above, including compatibility with diesel.

Please let me know what you think.

[weiznich]

Thanks for the update, this looks ok now. I think that's the best solution we can currently implement.
Thanks again for your patience and the will to try out different solutions. I will try to cut a release in the next few days.

(I've pushed a minor fixup commit)

[weiznich]

Published as 0.6.2