bpo-43466: Use pkg-config with --static in the PY_UNSUPPORTED_OPENSSL_BUILD path in setup.py by pablogsal · Pull Request #25475 · python/cpython

pablogsal · 2021-04-19T21:01:06Z

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

…_BUILD path in setup.py

tiran

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

tiran · 2021-04-20T06:39:25Z

setup.py

        # 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"))


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

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

tiran · 2021-04-20T06:40:48Z

setup.py

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


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

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

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 · 2021-04-20T09:41:37Z

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 · 2021-04-20T09:52:07Z

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 · 2021-04-20T11:09:36Z

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 · 2021-04-20T11:11:39Z

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 · 2021-04-20T11:13:56Z

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 · 2021-04-20T11:15:04Z

I will update the PR soon

tiran · 2021-04-20T11:28:25Z

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 · 2021-04-22T17:42:07Z

@pablogsal ping?

pablogsal · 2021-04-22T17:44:48Z

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 · 2021-04-25T10:00:10Z

+1

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

pablogsal · 2021-04-26T11:14:02Z

+1

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

Thanks a lot, @tiran, you rock 🎸 !

bpo-43466: Use pkg-config with --static in the PY_UNSUPPORTED_OPENSSL…

03bf7ce

…_BUILD path in setup.py

pablogsal requested a review from tiran April 19, 2021 21:01

the-knights-who-say-ni added the CLA signed label Apr 19, 2021

bedevere-bot added the awaiting core review label Apr 19, 2021

pablogsal added the skip news label Apr 19, 2021

tiran reviewed Apr 20, 2021

View reviewed changes

pablogsal closed this Apr 26, 2021

pablogsal deleted the bpo-43466 branch May 19, 2021 18:57

Uh oh!

Comments

Conversation

pablogsal commented Apr 19, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiran left a comment

Choose a reason for hiding this comment

Uh oh!

tiran Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

pablogsal Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

tiran Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

pablogsal Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

tiran Apr 20, 2021

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Apr 20, 2021

Uh oh!

tiran commented Apr 20, 2021

Uh oh!

pablogsal commented Apr 20, 2021

Uh oh!

tiran commented Apr 20, 2021

Uh oh!

pablogsal commented Apr 20, 2021

Uh oh!

pablogsal commented Apr 20, 2021

Uh oh!

tiran commented Apr 20, 2021

Uh oh!

tiran commented Apr 22, 2021

Uh oh!

pablogsal commented Apr 22, 2021

Uh oh!

tiran commented Apr 25, 2021

Uh oh!

pablogsal commented Apr 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pablogsal commented Apr 19, 2021 •

edited by bedevere-bot

Loading