Correct signing order for macOS bundles #1910
Conversation
Why?
That doesn't look like an error. What's the actual problem? Based on the log in #1891, Briefcase has once again captured the stderr but decided not to show it to the user or even to log it (#1907, #1918): |
The macOS docs on signing order say that you need to sign code "inside out - That is, if component A depends on component B, sign B before you sign A." I'm not sure how it identifies this and fails on signing the binary, in this case, but it definitely is failing.
It might not look like an error, but that is the full output of a manual invocation of the code signing process. If you attempt to sign But, if you sign the app that uses the library first, it succeeds: |
| # documentation for groupby() says that a new break is created every time a new | ||
| # group is found in the input data; sorting the input in reverse order ensures | ||
| # that only one group is found per folder, and that the deepest folder is found | ||
| # first. |
There was a problem hiding this comment.
But it doesn't guarantee that subfolders and files in the same folder are put into separate groups. For example, if there was no Helpers directory, the group separation would be:
.../Qt/lib/QtWebEngineCore.framework/Versions/A/QtWebEngineProcess.app/Contents/MacOS/QtWebEngineProcess
---
.../Qt/lib/QtWebEngineCore.framework/Versions/A/QtWebEngineProcess.app
.../Qt/lib/QtWebEngineCore.framework/Versions/A/QtWebEngineCore
Because the last two paths have the same parent, they would be signed in parallel.
Possible solution: make the key for both sorting and grouping be (path.parent, path.is_dir()), and sort with reverse=True. Those two elements map directly onto the two requirements stated in the comment.
If the grouping was factored out into a utility function, it could be covered by a test.
There was a problem hiding this comment.
Good catch - I hadn't considered that although the sort order will put the directory first, if they're grouped together parallelism won't guarantee their signed in that order.
I've modified the grouping as you've described (well - close - the sorting already puts directories before files, so we just need a different grouping key, not a full reverse sort order).
There was a problem hiding this comment.
WIth reverse=True, I think the sorting and grouping key can both be (path.parent, path.is_dir()), and all the complexity of sorted_depth_first can be removed.
There was a problem hiding this comment.
The ordering within the group shouldn't matter, but to make the build a bit more reproducible (and pass the tests), it would also be a good idea to sort within each group by the full paths.
There was a problem hiding this comment.
The key can't be exactly the same, because the sorting process needs to sort the actual files, whereas the grouping needs to sort on the containers of the files. So - we need to have a different key; the three-element key gives the sort order that matches a normal directory listings, so in any debug log it's going to be marginally less confusing. The 3-element key also matches the "complexity" of the actual search - sorting by "depth, then by directory vs file, then by filename" is literally what the algorithm is sorting on, rather than relying on a subtle edge effect of reverse interacting with the boolean value of is_dir().
There was a problem hiding this comment.
OK, so we can sort the whole list bykey=(path.parent, path.is_dir()), reverse=True, then group by the same key, then sort each group by path. That's still much easier to understand than the current version of sorted_depth_first, which creates keys which are lists of tuples.
There was a problem hiding this comment.
Unless the file sorting contains the actual path in the key, the ordering of files/folders inside any given group will be at least partially unpredictable. This won't matter to the signing order, but it makes testing a little harder because we can't completely guarantee the order of the output.
It also seems a little silly to me have a sort method that doesn't fully sort. Plus, we're already sorting, so requiring a second pass of sorting (in the test harness, and potentially in the usage of the final groups) seems a little silly.
However, I can't argue that the simple tuple is much easier to understand than the list of tuples, even if the resulting order isn't quite a "lexically pleasing" as the more complex option; and we can fix the filename sorting by adding the actual path to the initial sort key (using the simpler 2 part key for the grouping key).
|
The test failure on Ubuntu is a mystery to me... flatpak packaging on Ubuntu failing after these changes makes no sense at all... |
The run for PySide6 on Linux was taking longer than 30 minutes....at which point, the run times out. For some reason, the Docker builds were taking significantly longer... [edit] Fedora build: Arch build: |
|
I've just worked out what is going on. Since this issue was revealed by signing But - PySide6 is a 137MB download zipped... and it will be installed on Ubuntu as well. So, the binaries on Linux just doubled in size. 🤦 We could fix this by increasing the timeout... but since we only need the signing validation on macOS, I think I might modify the bootstrap so that -addons is only a requirement on macOS. |
Fixes #1891.
Corrects the order in which files are signed in a macOS app bundle.
This manifests if you sign an PySide app that includes PySide-Addons. This package contains a number of frameworks; these frameworks contain embedded applications. These applications must be signed before the library content of the framework, or the signing of the framework library fails with the error:
The previous approach to signing used reverse lexical sorting; this was enough to guarantee that content in a folder would be signed before the folder; but it wouldn't guarantee that a file in a folder would be signed before subfolders in the same folder.
Using the PySide6 example, reverse lexical sorting would result in a sort order of:
Because "QtWebEngineCore" sorts alphabetically after "Helpers", and the sort order is reversed, QtWebEngineCore is signed before the QtWebEngineProcess.app, which causes the error.
The new sorting approach results in the signing order:
so QtWebEngineCore is signed after the app that uses it.
PR Checklist: