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

bpo-43466: Use pkg-config with --static in the PY_UNSUPPORTED_OPENSSL_BUILD path in setup.py #25475

Closed
wants to merge 1 commit into from

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Apr 19, 2021

I tested in a bunch of old distributions (including RHEL6) and we need to run pkg-config with "--static" to obtain the actual expanded list of static libs.

https://bugs.python.org/issue43466

Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

How does compilation fail on these platforms and what is the value of pkg-config --static --libs-only-l openssl on them?

@@ -2447,8 +2447,13 @@ def split_var(name, sep):
# features like DSO engines or external OSSL providers don't work.
# Only tested on GCC and clang on X86_64.
if os.environ.get("PY_UNSUPPORTED_OPENSSL_BUILD") == "static":
import subprocess, shutil
pkgconfig = os.getenv("PKG_CONFIG", shutil.which("pkg-config"))
Copy link
Member

Choose a reason for hiding this comment

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

This will give incorrect results on systems without pkg-config. ax_check_openssl.m4 works around missing pkg-config.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's fine, we don't support all platforms for this hack. We can fall back to the previous behaviour if there is no pkg-config

import subprocess, shutil
pkgconfig = os.getenv("PKG_CONFIG", shutil.which("pkg-config"))
libs_out = subprocess.check_output(
[pkgconfig, "--static", "--libs-only-l", "openssl"]).decode()
Copy link
Member

Choose a reason for hiding this comment

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

You are not taking --with-openssl= configure option into account. It will only look up openssl.pc in standard pkg-config paths.

Copy link
Member Author

Choose a reason for hiding this comment

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

Where is that logic in the configure script? I can try to adapt it

Copy link
Member

Choose a reason for hiding this comment

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

It's in an external autoconf-archive script called ax_check_openssl.m4, https://www.gnu.org/software/autoconf-archive/ax_check_openssl.html . You cannot modify it.

@pablogsal
Copy link
Member Author

How does compilation fail on these platforms and what is the value of pkg-config --static --libs-only-l openssl on them?

Because the libraries they you need to link against to link OpenSSL statically is different than the ones you need to link against dynamically. In particular, in the second case you pick many dependencies transitively but in the first case you don't.

The error you get in this platforms is a linker error for a missing dependency on libz.

pkg-config --static --libs-only-l gives you the complete set of libraries you need to link against to link a library statically while pkg-config --libs-only-l just gives your the first level of the dependency tree.

@tiran
Copy link
Member

tiran commented Apr 20, 2021

Because the libraries they you need to link against to link OpenSSL statically is different than the ones you need to link against dynamically. In particular, in the second case you pick many dependencies transitively but in the first case you don't.

The error you get in this platforms is a linker error for a missing dependency on libz.

pkg-config --static --libs-only-l gives you the complete set of libraries you need to link against to link a library statically while pkg-config --libs-only-l just gives your the first level of the dependency tree.

That sounds like old OpenSSL with TLS compression support. Meh!

I would be totally fine to just hard-code the extra shared library:

# workaround for OpenSSL with compression
openssl_extension_kwargs["libraries"].append("z")

@pablogsal
Copy link
Member Author

I would be totally fine to just hard-code the extra shared library:

Ok, let's do that. I honestly dislike having to deal with pkg-config directly.

@tiran
Copy link
Member

tiran commented Apr 20, 2021

I would be totally fine to just hard-code the extra shared library:

Ok, let's do that. I honestly dislike having to deal with pkg-config directly.

me, too! Does the hack solve the problem for you?

@pablogsal
Copy link
Member Author

me, too! Does the hack solve the problem for you?

I would be totally fine to just hard-code the extra shared library:

Ok, let's do that. I honestly dislike having to deal with pkg-config directly.

me, too! Does the hack solve the problem for you?

Confirmed, this patch works in all systems I checked:

diff --git a/setup.py b/setup.py
index 253053da7f..d3da969807 100644
--- a/setup.py
+++ b/setup.py
@@ -2453,11 +2453,11 @@ def split_var(name, sep):
         # Only tested on GCC and clang on X86_64.
         if os.environ.get("PY_UNSUPPORTED_OPENSSL_BUILD") == "static":
             extra_linker_args = []
-            for lib in openssl_extension_kwargs["libraries"]:
+            for lib in openssl_extension_kwargs["libraries"] + ["z"]:

@pablogsal
Copy link
Member Author

I will update the PR soon

@tiran
Copy link
Member

tiran commented Apr 20, 2021

Does it also work when you link to libz dynamically? libz.a may not be available.

            # don't link OpenSSL shared libraries
            # include libz for OpenSSL build flavors with compression support
            openssl_extension_kwargs["libraries"] = ["z"]

@tiran
Copy link
Member

tiran commented Apr 22, 2021

@pablogsal ping?

@pablogsal
Copy link
Member Author

Does it also work when you link to libz dynamically? libz.a may not be available.

            # don't link OpenSSL shared libraries
            # include libz for OpenSSL build flavors with compression support
            openssl_extension_kwargs["libraries"] = ["z"]

Sorry for the late response, I have been swamped trying to update the buildbot server. I think we can dynamically link against libz as there is no risk with those symbols. Would that work for you?

@tiran
Copy link
Member

tiran commented Apr 25, 2021

+1

I have opened #25587 to address the issue and reduce your work load.

@pablogsal
Copy link
Member Author

+1

I have opened #25587 to address the issue and reduce your work load.

Thanks a lot, @tiran, you rock 🎸 !

@pablogsal pablogsal closed this Apr 26, 2021
@pablogsal pablogsal deleted the bpo-43466 branch May 19, 2021 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants