Codesigning vapor on Apple Silicon #3639
Conversation
| set (CMAKE_INSTALL_RPATH "@executable_path/../Frameworks") | ||
| set (CMAKE_INSTALL_RPATH "@loader_path") | ||
| elseif ("${CMAKE_OSX_ARCHITECTURES}" STREQUAL "arm64") | ||
| set (CMAKE_INSTALL_RPATH "@executable_path/../Frameworks;@executable_path/../Resources") |
There was a problem hiding this comment.
RPATH is for finding libraries/frameworks which for an AppBundle should exist inside the Frameworks directory. Is there a reason we also need it to link to the Resources directory?
From the macOS docs:
Frameworks: Contains any private shared libraries and frameworks used by the executable
Resources: data files that live outside your application’s executable file. Resources typically consist of things like images, icons, sounds, nib files, strings files, configuration files, and data files
There was a problem hiding this comment.
Yes, it's a codesigning requirement. Frameworks cannot contain non-binary files like .py modules, so Python needs to reside in Resources.
There was a problem hiding this comment.
Just a followup with a reference. This is also how Blender distributes Python.
Store Python, Perl, shell, and other script files and other non-Mach-O executables in your app's Contents/Resources directory. While it's possible to sign such executables and store them in Contents/MacOS, this is not recommended.
There was a problem hiding this comment.
I see the issue and I am fine keeping it this way.
To respond to your followup:
Store Python, Perl, shell, and other script files and other non-Mach-O executables in your app's Contents/Resources directory. While it's possible to sign such executables and store them in Contents/MacOS, this is not recommended.
Blender's macOS app does not follow the specification at all.
There was a problem hiding this comment.
Ok, thanks, but for the record:
Blender's macOS app does not follow the specification at all.
Blender puts all of its libraries in the Resources directory. Could you elaborate if I'm mistaken?
There was a problem hiding this comment.
Correct, which directly contradicts the specification.
| string exists = FileUtils::JoinPaths({path, PYTHON_INSTALLED_PATH}); | ||
|
|
||
| if (!FileUtils::Exists(FileUtils::JoinPaths({path, PYTHON_INSTALLED_PATH}))) path = string(PYTHON_DIR); | ||
| string path = string(PYTHON_DIR); // Try our third-party-library directory first |
There was a problem hiding this comment.
We want to use the installed python dir first, and only have the developer libs as a fallback.
There was a problem hiding this comment.
This is a fairly obscure part of our codebase. I'd recommend revisiting it after 3.9.3 but we can postpone the release if it's that important.
There was a problem hiding this comment.
I'm not sure what you mean by obscure, this determines the version of python that is used by Vapor so it is rather critical. With this change it will be impossible for a developer to test an installed version of Vapor without first removing their third party library installation.
There was a problem hiding this comment.
I understand and totally see you point. IMO, I don't think it's critical to fix now, but it's critical to fix soon because installers are not often tested on dev computers. This is why we do platform tests on clean (non-dev) systems.
If this solution didn't work as in this PR, I would agree to modify it as you've suggested.
This file is obscure because:
- It heavily relies on macro'd functions that cannot be plugged into a debugger like lldb. GetResourcePath() can't be debugged without adding print statements to see what happens
- PYTHON_INSTALLED_PATH is also macro'd
- CMake code injections are furthermore specified from lib/common/CMakeConfig.cpp.in. This is as hard to debug as macro's, but the changes are coming from yet another compile time modification
- The #ifdef WIN32 macro for GetPythonPath() should be in-lined with GetPythonDir(). What's the difference between a PythonPath and a PythonDir?Apparently it's windows is dir, unix is path? Path and Dir are synonyms so this makes no sense.
There was a problem hiding this comment.
I use the installers to test on my dev machine frequently when I am unsure if an issue is related to new code or my dev environment. This invalidates such a test. It also creates the situation where if someone has a different incompatible version of the third part libs installed (older or newer) vapor may try to load that version and crash. A few months down the line we may forget about this behavior and have to figure out why vapor is crashing. Since this is only going to affect developers not end users I'm fine with not holding up the release over this. Also, read obscure as not important or well known.
Definitely temporary and noted in the PR text:
|
Fair point, my apologies. I read this but must've forgot about it by the time I was wrapping up the review after our meeting. |
|
I see what you're saying, but does this need to be done imminently?
I agree and suggest that we add a new issue and handle it for the next release. I want to emphasize that debugging this code requires a chain of events that include recompilation, codesigning, and notorization. Adding a single print statement to debug this code is inordinately time consuming. I've tested this with confidence but it's a huge time consuming pain. This process works right now, but I agree that we can also improve it with your suggestions. If we're going to do rapid releases, we should heavily consider flexibility. |
I said
however, the reason why it would be preferable to do this now is once vapor 3.9.3 is released with this behavior, that version will permanently have this issue when we use it to test for regressions. |
Makes changes to support a native arm64 build. These changes are currently macro'd in order to keep supporting our current x86 build. The macros will be removed when the x86 third party libraries have been updated to support codesigning.
After an installer is generated, scripts/codesignMacOS.sh can be run to sign all binaries, libraries, and the bundle itself. Notarization and stapling then takes place after codesigning. All of these steps require the use of our Developer Application ID certificate.
A large portion of this work included modifying our third party libraries. The new libraries can be found on our AWS bucket here: https://vaporawsbucket.s3.us-west-2.amazonaws.com/2024-Jul-AppleSilicon.tar.xz
@StasJ, OSPRay was upgraded to version 3, which deprecated a few of the old API calls. Can you take a close look at what they've been replaced with? I am seeing that the OSPRay volume renderer only renders during fast mode while moving the scene.
Fixes #3638, #3483, and #3269.
A test installer can be found here:
https://drive.google.com/file/d/1JkpBhv1WcgNvNeNuUH1HT1fU3ztARzcf/view?usp=drive_link