Adding missing rpath to Python on Apple Silicon #1467

colincornaby · 2023-08-16T05:27:31Z

On Apple Silicon, Homebrew has moved the location its installed its libraries. That means if you are installing a version of Cairo from Homebrew, Python/python3-cairosvg cannot find Cairo, and vcpkg will fail to build python3-cairosvg.

Homebrew's version of Python passes an additional rpath to the configuration phase of the Python build to fix this:
https://github.com/carlocab/homebrew-core/blob/d83d8d4665518494ef7fc3e5a64ba937e69cb06c/Formula/python%403.9.rb#L150

This patch to our Python port also introduces the same patch. I'm still pretty new with vcpkg so I possibly haven't defined this correctly from a maintainable vcpkg perspective. But this fix does block Apple Silicon builds out of the box. I've had to do some manual patching each vcpkg update to work around it.

dpogue · 2023-08-16T05:36:46Z

Two things I'm wondering here:

This should have no ill-effects if the Homebrew directory does not exist?
This does not somehow introduce a potential security hole whereby the game Python interpreter can load modules from the homebrew path? (I assume not, but good to make sure)

I seem to recall that on Windows, we have to build Python as a dynamic library so that we get a working interpreter at build-time that can load the cairo module. I know you're trying to make sure Python is built as a static library on macOS, but do we run into the same issue there when trying to use it at build time?
(grumble grumble back to #1423 and the inability of CMake to find a host Python interpreter and a target Python library)

colincornaby · 2023-08-16T06:18:42Z

It should not cause any failure if the Homebrew path does not exist.

To answer 2 and your follow on question…. It gets weird.

The rpath argument is embedded into the binary and parsed by the dynamic linker. But we build a static version of Python, so this rpath does absolutely nothing for the static library built for Plasma. In since Python is a static library I’m not even sure dynamic loader commands could be embedded at all. What it does effect is the Python executable that is built by vcpkg - which is what the Python Cairo package uses to test it’s integration.

dpogue · 2023-08-16T06:20:20Z

Interesting... okay. So to confirm, on macOS we're able to build both a working Python interpreter executable and a Python static library from vcpkg at the same time?

colincornaby · 2023-08-16T06:22:18Z

That seems to be what is occurring. In the vcpkg output there is also an interpreter that the Cairo module is executing tests against. On Apple Silicon the Cairo module fails during it’s test phase.

dgelessus · 2023-08-16T06:49:28Z

Hmm, I don't understand why anything should be pulled in from Homebrew when using vcpkg to build the dependencies... Shouldn't Cairo also come from vcpkg in that case?

Hoikas · 2023-08-17T00:42:17Z

PSF seems to think this is incorrect https://bugs.python.org/issue46381

colincornaby · 2023-08-17T00:46:20Z

Reading the full thread, it seems ok because we use LDFLAGS_NODIST which is the intended mechanism.

python/cpython#90539 (comment)

Hoikas · 2023-08-17T00:53:46Z

Ok, in that case, it would probably be better to, instead of patching the Python build, pass through the configure option CONFIGURE_LDFLAGS_NODIST from the triplet. That way, we could (potentially) de-vendor the port someday. So, in the triplet, you'll want to do something like:

if(PORT STREQUAL python3)
    set(VCPKG_MAKE_CONFIGURE_OPTIONS "CONFIGURE_LDFLAGS_NODIST=-rpath /opt/homebrew/lib")
endif()

and toss the patch.

colincornaby · 2023-08-17T00:54:57Z

Oooooo thanks. That’s the sort of insight I was looking for. I’ll edit the commit history to reflect only this suggestion.

colincornaby · 2023-08-17T01:50:06Z

That chunk is applying during the Python build - but the output isn't correct. I'm tinkering with it to see if I can figure out the issue.

colincornaby · 2023-08-17T06:00:26Z

Ok - there is an issue on the vcpkg side. I can submit a PR - but something is going to get broken possibly.

VCPKG_MAKE_CONFIGURE_OPTIONS is indeed a documented option:
https://learn.microsoft.com/en-us/vcpkg/users/triplets#vcpkg_make_configure_options

This change seems to have intentionally or unintentionally renamed that option to VCPKG_CONFIGURE_MAKE_OPTIONS - without updating the docs:
microsoft/vcpkg@5283cdb#diff-b92b07d5c0c681687d71463c6a368fae98e9ffedb241e2aadbde3aac688899a9L519

(Line 519, scripts/cmake/vcpkg_configure_make.cmake)

So that's why it wasn't working. I can use VCPKG_CONFIGURE_MAKE_OPTIONS for now. But something is not right.

colincornaby · 2023-08-17T06:16:14Z

I've gone ahead and implemented the change through a history rewrite - accounting for the issue noted in the previous comment.

dgelessus · 2023-08-17T11:03:39Z

Why do we need to add Homebrew paths to the linker flags at all though? If a library is missing, the proper fix would be to add it as a dependency in our vcpkg.json, not pull it in from a different package manager - right?

dpogue · 2023-08-17T13:10:16Z

If a library is missing, the proper fix would be to add it as a dependency in our vcpkg.json, not pull it in from a different package manager - right?

For the case of cairo, since it's an optional dependency used only on the host system to generate resource.dat at build time, we've been pulling it from package managers on macOS and Linux. On Windows we do pull it in with vcpkg though, so that's an option if it makes our lives easier:

Plasma/vcpkg.json

Lines 55 to 59 in d863078

    
           { 
        
             "name": "cairo", 
        
             "platform": "windows", 
        
             "default-features": false 
        
           },

colincornaby · 2023-08-17T15:40:16Z

I'm not completely sure that would work on macOS - but I could try. When vcpkg builds the library it won't add it to a search path, taking us right back where we started. The Python interpreter won't be able to use Cairo because it won't be in a known path.

dgelessus · 2023-08-17T17:21:35Z

For the case of cairo, [...], we've been pulling it from package managers on macOS and Linux.

On Linux it's not a problem in practice, because often you already have Cairo installed system-wide as a dependency of the usual GUI environments. On macOS you usually need to install it manually, and when you do, it will be in a location that isn't searched by default by many things, which is suboptimal.

since it's an optional dependency used only on the host system to generate resource.dat at build time

Ah okay, that makes some sense. But is there any disadvantage to using vcpkg for such dependencies as well (especially if we already do that on Windows)? There's even already some code in VcpkgToolchain.cmake that skips installing Cairo if you turn off PLASMA_BUILD_RESOURCE_DAT, so you can always do that if the extra dependency is an issue.

When vcpkg builds the library it won't add it to a search path, taking us right back where we started. The Python interpreter won't be able to use Cairo because it won't be in a known path.

That would make things more difficult... but in that case, I'd argue that the right fix/workaround is to put vcpkg's directories onto the Python search path, rather than depending on Homebrew.

(Side note: I personally use MacPorts and don't even have Homebrew installed, so I get a bit peeved when things have special defaults that only work for Homebrew 🙂 But regardless of that, I think we shouldn't depend on more than one package manager, unless there's a very good reason for it.)

Hoikas · 2023-08-17T22:04:58Z

I'm not completely sure that would work on macOS - but I could try. When vcpkg builds the library it won't add it to a search path, taking us right back where we started. The Python interpreter won't be able to use Cairo because it won't be in a known path.

If we went this route, we would need to copy the cairo DLL into the Python site-packages in the install step in our python3-cairosvg port.

colincornaby · 2023-08-18T02:40:45Z

I think I probably could just point the rpath at the vcpkg dir instead of the Homebrew dir.

Overall what would be required is:

Building Cairo as a dylib instead of as a static library (easy)
Building Cairo for the host architecture instead of the destination architecture (easy)
Getting the path to Cairo lib folder within the triplet (not sure how to do this)

Hoikas · 2023-08-18T03:10:48Z

Building Cairo for the host architecture instead of the destination architecture (easy)

python3-cairosvg should probably be marked "host": true

colincornaby · 2023-08-19T04:17:42Z

Alright - let's try this approach. :)

This pulls Cairo from vcpkg. It also sets up the rpath to look for Cairo within the vcpkg installed directory. Cairo is no longer required from Homebrew.

I didn't remove the Homebrew install from Github actions yet. If this builds successfully in Github Actions I'll go ahead and do so.

Hoikas · 2023-08-19T17:39:18Z

Scripts/Triplets/universal-macos-plasma.cmake

@@ -13,3 +13,7 @@ if(PORT STREQUAL cairo)
    set(VCPKG_BUILD_TYPE release)
 endif()

+if(PORT STREQUAL python3)
+    set(VCPKG_CONFIGURE_MAKE_OPTIONS "LDFLAGS_NODIST=-rpath ${CURRENT_INSTALLED_DIR}/lib")


The Python documentation says that this should be CONFIGURE_LDFLAGS_NODIST

Hoikas · 2023-08-19T17:39:21Z

Scripts/Triplets/x64-macos-plasma.cmake

@@ -12,3 +12,7 @@ if(PORT STREQUAL cairo)
    set(VCPKG_LIBRARY_LINKAGE dynamic)
    set(VCPKG_BUILD_TYPE release)
 endif()
+
+if(PORT STREQUAL python3)
+    set(VCPKG_CONFIGURE_MAKE_OPTIONS "LDFLAGS_NODIST=-rpath ${CURRENT_INSTALLED_DIR}/lib")


The Python documentation says that this should be CONFIGURE_LDFLAGS_NODIST

CONFIGURE_LDFLAGS_NODIST does not seem to work. Build fails when I switch to that argument.

What is the error?

It's the same error this is trying to fix in the Python Cairo module. It's as if no configure arguments are applied at all.

Hoikas · 2023-08-19T17:40:19Z

vcpkg.json

-            "platform": "windows",
-            "default-features": false
+            "default-features": false,
+            "host": true


This should probably be removed from here and put into the manifest for python3-cairosvg, then host: true should cascade properly.

Hoikas · 2023-08-27T01:27:30Z

Scripts/Ports/python3-cairosvg/vcpkg.json

-      "platform": "windows",
-      "default-features": false
+      "default-features": false,
+      "host": true


python3-cairosvg in the project's manifest should probably be "host": true so that it will cascade properly. What you seem to be asking for here, in my mind, if you're on x64 macOS cross-compiling for ARM, x64 cairo and ARM python3.

I think there is a bit of breakage going on in vcpkg that I've talked about with @dpogue that may be at play here. But - just to clarify - is Cairo only used for Resource.dat generation? I think it's only needed by the host.

What's actually needed in a cross compile (that might be broken right now):

Client architecture Python

Host architecture Python for build processes

Host architecture Cairo

Host architecture python3-cairo

(The above assumes that my understanding of Cairo as a build related tool is correct and not part of the client.)

This is correct. We want for cairo to cascade to host arch by marking python3-cairosvg as host.

I may be confused because I feel like this is the opposite of this comment here:
#1467 (comment)

I think I'm confused on whats being asked for.

In the root vcpkg.json:

{ "name": "python3-cairosvg", "host": true }

In python3-cairosvg's json, cairo is a regular dependency.

I think I understand what you're asking and I edited the last commit. Let me know if thats what you were thinking. I removed Cairo from the root manifest and am allowing python3-cairosvg to pull it through.

I think you do need to remove the "platform": "windows" line in python3-cairosvg's config file though, otherwise it's going to skip installing cairo on macOS: https://github.com/H-uru/Plasma/blob/master/Scripts/Ports/python3-cairosvg/vcpkg.json#L9

colincornaby · 2023-09-03T04:30:14Z

I've updated the commit to remove the Windows gate for the Cairo dependency.

colincornaby requested review from dpogue and Hoikas August 16, 2023 05:27

dpogue approved these changes Aug 16, 2023

View reviewed changes

colincornaby force-pushed the python-apple-silicon-rpath-fix branch from f07fe38 to b937364 Compare August 17, 2023 06:11

Making Cairo a vcpkg dependency and setting rpath

82558e7

colincornaby force-pushed the python-apple-silicon-rpath-fix branch from b937364 to 82558e7 Compare August 19, 2023 04:14

colincornaby changed the title ~~Adding missing Homebrew rpath to Python on Apple Silicon~~ Adding missing rpath to Python on Apple Silicon Aug 19, 2023

Removing Cairo install step via Homebrew

da88f3c

Hoikas reviewed Aug 19, 2023

View reviewed changes

Hoikas reviewed Aug 27, 2023

View reviewed changes

colincornaby force-pushed the python-apple-silicon-rpath-fix branch from 2ad9ab0 to 80b2b68 Compare August 31, 2023 05:01

Treating Cairo as dependency of python3-cairosvg

5fbd30a

colincornaby force-pushed the python-apple-silicon-rpath-fix branch from 80b2b68 to 5fbd30a Compare September 3, 2023 04:25

Hoikas approved these changes Sep 6, 2023

View reviewed changes

Hoikas merged commit 4285442 into H-uru:master Sep 6, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding missing rpath to Python on Apple Silicon #1467

Adding missing rpath to Python on Apple Silicon #1467

colincornaby commented Aug 16, 2023

dpogue commented Aug 16, 2023

colincornaby commented Aug 16, 2023

dpogue commented Aug 16, 2023

colincornaby commented Aug 16, 2023

dgelessus commented Aug 16, 2023

Hoikas commented Aug 17, 2023 •

edited

Loading

colincornaby commented Aug 17, 2023

Hoikas commented Aug 17, 2023

colincornaby commented Aug 17, 2023

colincornaby commented Aug 17, 2023

colincornaby commented Aug 17, 2023 •

edited

Loading

colincornaby commented Aug 17, 2023

dgelessus commented Aug 17, 2023

dpogue commented Aug 17, 2023

colincornaby commented Aug 17, 2023

dgelessus commented Aug 17, 2023

Hoikas commented Aug 17, 2023

colincornaby commented Aug 18, 2023 •

edited

Loading

Hoikas commented Aug 18, 2023

colincornaby commented Aug 19, 2023

Hoikas Aug 19, 2023

Hoikas Aug 19, 2023

colincornaby Aug 19, 2023

Hoikas Aug 19, 2023

colincornaby Aug 19, 2023

Hoikas Aug 19, 2023

Hoikas Aug 27, 2023

colincornaby Aug 28, 2023

Hoikas Aug 28, 2023

colincornaby Aug 28, 2023

Hoikas Aug 28, 2023 •

edited

Loading

colincornaby Aug 31, 2023

dpogue Aug 31, 2023

colincornaby commented Sep 3, 2023

Adding missing rpath to Python on Apple Silicon #1467

Adding missing rpath to Python on Apple Silicon #1467

Conversation

colincornaby commented Aug 16, 2023

dpogue commented Aug 16, 2023

colincornaby commented Aug 16, 2023

dpogue commented Aug 16, 2023

colincornaby commented Aug 16, 2023

dgelessus commented Aug 16, 2023

Hoikas commented Aug 17, 2023 • edited Loading

colincornaby commented Aug 17, 2023

Hoikas commented Aug 17, 2023

colincornaby commented Aug 17, 2023

colincornaby commented Aug 17, 2023

colincornaby commented Aug 17, 2023 • edited Loading

colincornaby commented Aug 17, 2023

dgelessus commented Aug 17, 2023

dpogue commented Aug 17, 2023

colincornaby commented Aug 17, 2023

dgelessus commented Aug 17, 2023

Hoikas commented Aug 17, 2023

colincornaby commented Aug 18, 2023 • edited Loading

Hoikas commented Aug 18, 2023

colincornaby commented Aug 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hoikas Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colincornaby commented Sep 3, 2023

Hoikas commented Aug 17, 2023 •

edited

Loading

colincornaby commented Aug 17, 2023 •

edited

Loading

colincornaby commented Aug 18, 2023 •

edited

Loading

Hoikas Aug 28, 2023 •

edited

Loading