Made pg_config path configurable per-target. #18

golddranks · 2017-08-21T09:47:50Z

Makes possible to fix rust-lang/rust#43486

When cross-compiling Diesel, libpq is compiled twice: once for the target system and once for the host system. This is because Diesel's codegen crates use libpq, and they need to run on the host system since they are compiler plugins. (In my case, the host system is linux+glibc and the target system is linux+musl) These systems need to use different libraries, but the current libpq build script accesses the same pg_config regardless. This change makes it possible to set the pg_config per-target which remedies this.

golddranks · 2017-08-22T08:08:00Z

So, I pushed a commit that has a per-target version of PQ_LIB_STATIC too; for example PQ_LIB_STATIC_X86_64_UNKNOWN_LINUX_MUSL for the musl target. Since the compiler plugins are loaded dynamically, linking statically things with pq-sys built for the use of a plugin will break the build, so per-target variables are good to have.

So, there is now two sets of settings: the normal env variables and target-specific variables, which help with cross-compiling.

The current changes help me cross-compile using this Docker image: https://github.com/golddranks/rust_musl_docker , but at the moment I have to use my fork of pq-sys that understands these target-specific env variables, so I can't announce the image yet.

If you have comments or questions about the PR, I'll be happy to answer and/or improve the code.

Also, I'm not sure if I managed to explain clearly why having these config settings are needed, so don't be afraid to ask!

golddranks · 2017-08-22T08:10:00Z

One thing I didn't implement yet is specifying target-specific lib paths directly, instead of consulting pg_config. If you think the target-specific vars are a sane way to configure the build, I'll send another commit later.

sgrif · 2017-10-06T19:52:57Z

build.rs

 fn pg_config_output(command: &str) -> Option<String> {
-    Command::new("pg_config")
+


emk · 2017-10-19T19:56:17Z

Thank you for figuring this out!

This is also breaking diesel with https://hub.docker.com/r/ekidd/rust-musl-builder/, which is how we deploy our in-house Diesel based tools at Faraday. We use an Ubuntu Docker container to produce static binaries, which we then release either directly to our users, or package as Alpine Docker images.

Is there anything I could do to help test this patch or help get it ready to merge?

golddranks · 2017-10-19T23:46:54Z

Glad that I'm not the only one experiencing this! Let's make this happen!

Btw. since we both link stuff statically, might be interested in each other's solutions; here's my builder container. I put some effort into documenting it thoroughly: https://github.com/golddranks/rust_musl_docker

@golddranks

Fixes #27, or at least works around it. This deals with the issues mentioned in sgrif/pq-sys#18, and it relies heavily on ideas from @golddranks and @clux.

emk · 2017-10-24T14:54:41Z

OK, a big thank you to everyone!

I've added workarounds and documentation for this issue in rust-musl-builder. Specifically, the following lines need to be added to Cargo.toml:

[dependencies]
# This is needed to make sure that Cargo statically links against
# `libssl`. This should happen automatically, but it doesn't.
openssl-sys = "0.9"

[patch.crates-io]
# This is needed to handle cross-compilation of libpq.
pq-sys = { git = 'https://github.com/golddranks/pq-sys' }

After that, it's just a matter of setting up a cross-compilation environment, which has its own wrinkles. But happily, that's not the job of pq-sys.

I think that cross-compilation is a relatively important use case:

It allows building statically-linked Linux binaries that can be used on any distro. This is a really great Rust feature.
It allows building Alpine-based Docker containers using multi-stage builds, which makes it much easier to put containerized Rust microservices into production without dragging along a full-fledged Linux distro.

Given that I think this is an important feature, I'm willing to help get the PR ready to merge. I'd also be willing to contribute a Docker-based test case that verifies that it's possible to build and link a fully-static diesel-based application.

emk

Thank you so much for preparing this PR! It has made my life much easier.

Reading through the code, here are a couple of ideas for things which might be tweaked. And at least one case where I'm probably doing something wrong, but I don't understand the issue yet. :-/

Thank you very much for this PR!

emk · 2017-10-24T14:56:52Z

build.rs

+
+    // Link unconditionally statically
+    if env::var_os("PQ_LIB_STATIC").is_some() {
+        println!("cargo:rustc-link-lib=static=pq");


I've noticed that I need to include the following when linking diesel statically:

[dependencies] # This is needed to make sure that Cargo statically links against # `libssl`. This should happen automatically, but it doesn't. openssl-sys = "0.9"

Is there an underlying issue here with ps-sys or openssl-sys when statically linking that we should deal with? Or am I just messing up? I have a very small test program here that might help figure this out.

emk · 2017-10-24T14:58:12Z

build.rs

+
+    // Examine the per-target env vars
+    if let Ok(target) = env::var("TARGET") {
+        let pg_config_for_target = format!("PQ_LIB_STATIC_{}", target.to_ascii_uppercase().replace("-", "_"));


This requires us adding a new

println!("cargo:rerun-if-env-changed={}", pg_config_for_target);

...line somewhere in the script. Also, you probably mean pq_lib_static_for_target here. :-)

emk · 2017-10-24T15:00:38Z

build.rs

+    use std::ascii::AsciiExt;
+
+    if let Ok(target) = env::var("TARGET") {
+        let pg_config_for_target = &format!("PG_CONFIG_{}", target.to_ascii_uppercase().replace("-", "_"));


This also requires something like:

println!("cargo:rerun-if-env-changed={}", pg_config_for_target);

It might be best to wrap env::var_os in a wrapper that always generates this println!, and then refactor all of build.rs to use it.

sgrif · 2017-10-24T19:07:29Z

build.rs

@@ -63,8 +58,56 @@ fn configured_by_vcpkg() -> bool {
    false
 }

+fn static_or_not() {


This method name seems odd to me. It has side effects but implies it returns a boolean. Can we have it return a value instead? Perhaps a 3 state enum?

chrisdotcode · 2018-02-22T20:30:20Z

Hey @golddranks, @sgrif, has there been any progress on this? Being able to statically link diesel is something I definitely need.

emk · 2018-02-22T21:27:27Z

I'm currently recommending this PR of diesel for all https://hub.docker.com/r/ekidd/rust-musl-builder/ builds. Nobody has reported any issues yet. :-)

sgrif · 2018-02-23T23:28:42Z

I'm going to close this, as there's unresolved feedback that's been sitting for more than 6 months. If someone would like to revive this feature and address the open feedback, I will be happy to accept it.

golddranks · 2018-05-09T06:21:19Z

Just for the record (as there are inbound links to this thread), this was later pulled in as #19

golddranks and others added 2 commits August 21, 2017 18:41

Made pg_config path configurable per-target.

bcd9aa2

Now the static linking setting is also target-dependent.

e1d26ac

sgrif reviewed Oct 6, 2017

View reviewed changes

emk mentioned this pull request Oct 19, 2017

Rust 1.21.0 & diesel_codegen: "undefined symbol: SSL_set_ex_data" emk/rust-musl-builder#27

Closed

clux mentioned this pull request Oct 20, 2017

diesel_codegen forces dynamic linkage clux/muslrust#19

Closed

emk reviewed Oct 24, 2017

View reviewed changes

sgrif reviewed Oct 24, 2017

View reviewed changes

sgrif closed this Feb 23, 2018

weiznich mentioned this pull request Mar 1, 2018

Feature/platform depend static compilation #19

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made pg_config path configurable per-target. #18

Made pg_config path configurable per-target. #18

golddranks commented Aug 21, 2017

golddranks commented Aug 22, 2017

golddranks commented Aug 22, 2017 •

edited

Loading

sgrif Oct 6, 2017

golddranks Oct 19, 2017

emk commented Oct 19, 2017

golddranks commented Oct 19, 2017 •

edited

Loading

emk commented Oct 24, 2017 •

edited

Loading

emk left a comment

emk Oct 24, 2017

emk Oct 24, 2017

emk Oct 24, 2017

sgrif Oct 24, 2017

chrisdotcode commented Feb 22, 2018

emk commented Feb 22, 2018

sgrif commented Feb 23, 2018

golddranks commented May 9, 2018

		fn pg_config_output(command: &str) -> Option<String> {
		Command::new("pg_config")

Made pg_config path configurable per-target. #18

Made pg_config path configurable per-target. #18

Conversation

golddranks commented Aug 21, 2017

golddranks commented Aug 22, 2017

golddranks commented Aug 22, 2017 • edited Loading

sgrif Oct 6, 2017

Choose a reason for hiding this comment

golddranks Oct 19, 2017

Choose a reason for hiding this comment

emk commented Oct 19, 2017

golddranks commented Oct 19, 2017 • edited Loading

emk commented Oct 24, 2017 • edited Loading

emk left a comment

Choose a reason for hiding this comment

emk Oct 24, 2017

Choose a reason for hiding this comment

emk Oct 24, 2017

Choose a reason for hiding this comment

emk Oct 24, 2017

Choose a reason for hiding this comment

sgrif Oct 24, 2017

Choose a reason for hiding this comment

chrisdotcode commented Feb 22, 2018

emk commented Feb 22, 2018

sgrif commented Feb 23, 2018

golddranks commented May 9, 2018

golddranks commented Aug 22, 2017 •

edited

Loading

golddranks commented Oct 19, 2017 •

edited

Loading

emk commented Oct 24, 2017 •

edited

Loading