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

perf: parallelize signing #341

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SuperDisk
Copy link

When you're signing an application that includes a lot of native code, e.g. dylibs and executables, the time it takes to sign it all can balloon up to hours-- this is the case for myself on a project that includes GStreamer (which is massive).

This commit does signing in parallel. It's slightly nuanced-- the Apple code signing tool can't operate on a higher directory level directory while signing is going on below, so we need to handle it at the leaf nodes first and then continue upwards. It's really weird and unintuitive, but oh well; this PR handles those cases properly.

For me this knocks signing time down from hours to about a minute.

Additionally, this PR fixes a bug in the existing code, where stat is incorrectly used instead of lstat, meaning that the later check in the code for .isSymbolicLink() would never return true.

@SuperDisk SuperDisk requested a review from a team as a code owner January 30, 2025 17:57
package.json Outdated Show resolved Hide resolved
yarn.lock Outdated
@@ -4,7 +4,7 @@

"@electron/get@^2.0.2":
version "2.0.2"
resolved "https://registry.yarnpkg.com/@electron/get/-/get-2.0.2.tgz#ae2a967b22075e9c25aaf00d5941cd79c21efd7e"
resolved "https://registry.npmjs.org/@electron/get/-/get-2.0.2.tgz"
Copy link
Member

Choose a reason for hiding this comment

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

Note that a maintainer is going to have to revert the lockfile changes as well and re-apply them as per our dependency policy.

I'm not sure why all the URLs are being switched to npmjs.org here.

Copy link
Author

Choose a reason for hiding this comment

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

Sure-- sorry, I just quickly slapped this PR up since it had just been sitting on my machine and I just wanted it to see the light of day. I had hacked up things a bit to install with npm and this is PR is just the logic extracted from that work. I can undo the whitespace changes and the syntax issue to make it work with Yarn, but I think someone on your side will need to re-lock things as you said.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you! Will give the business logic a review once that's ready. 🙇‍♂️

Copy link
Author

Choose a reason for hiding this comment

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

Done! Let me know if there's anything else that needs changed. I updated p-limit to the latest-- I had specifically chosen that old version because I was running into weird module type issues, but I was building osx-sign in an unorthodox manner so it might be alright to use the latest.

src/util.ts Outdated Show resolved Hide resolved
@erickzhao erickzhao changed the title Parallelize signing perf: parallelize signing Jan 30, 2025
@SuperDisk SuperDisk force-pushed the parallelize-signing branch from 46c4e9e to 3bacf54 Compare January 30, 2025 21:14
@SuperDisk SuperDisk requested a review from erickzhao January 30, 2025 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants