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

fix: Respect unpack minimatch for symlinks within previously unpacked directories #341

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mmaietta
Copy link
Contributor

@mmaietta mmaietta commented Nov 12, 2024

Summary:
Fixes issue with unpack configuration when considering symlinks within previously unpacked directories. This directly fixes unpacking static .framework modules on Mac, as otherwise codesigning will fail due to symlink files/directories not being reflected in the app.asar.unpacked directory.

Fix:

  • Tracks all symlinks in a list (similar as files) as to what are supposed to be unpacked.
  • Adds unpacked flag as node.unpacked=true
  • Extracts common logic for detecting if a path is unpacked
  • Added promisified symlink and readlink
  • Reuses Filesystem unit test approach for programmatically creating symlinks during tests so as to properly replicate symlinks on Win/Linux/MacOS. (committing Windows symlinks created with mklink do not get committed properly to git)

Details:
.framework has to have all symlinks reproduced in the unpacked directory for signing. The top is with electron/asar unpacked folder, the bottom is the actual framework contents.
Screenshot 2024-11-11 at 4 28 35 PM

Without this PR, codesigning will fail with QtCore.framework: bundle format unrecognized, invalid, or unsuitable

  • signing         file=dist/mac/SanitizedAppName.app platform=darwin type=distribution identity=<SanitizedIdentity> provisioningProfile=none
  ⨯ Command failed: codesign --sign <SanitizedIdentity> --force --timestamp --options runtime --entitlements ./build/entitlements.mac.plist /Users/<project-folder>/dist/mac/SanitizedAppName.app/Contents/Resources/app.asar.unpacked/node_modules/<sanitized-dep-name>/lib/Release/QtCore.framework
/Users/<project-folder>/dist/mac/SanitizedAppName.app/Contents/Resources/app.asar.unpacked/node_modules/<sanitized-dep-name>/lib/Release/QtCore.framework: bundle format unrecognized, invalid, or unsuitable

Fixes: electron-userland/electron-builder#8655 (comment)

… previously unpacked directories. This directly fixes unpacking static `.framework` modules on Mac, as otherwise codesigning will fail due to symlink files/directories not being reflected in the app.asar.unpacked directory.

Added unit test with Hello.framework, generated from tutorial https://jano.dev/apple/mach-o/2024/06/28/Hello-Static-Framework.html
Fixes: electron-userland/electron-builder#8655
@mmaietta mmaietta force-pushed the fix/static-framework-symlinks-2 branch from b98871b to 888ef4f Compare November 13, 2024 21:37
src/disk.ts Show resolved Hide resolved
test/cli-spec.js Outdated Show resolved Hide resolved
@mmaietta mmaietta marked this pull request as ready for review December 13, 2024 19:08
@mmaietta mmaietta requested a review from a team as a code owner December 13, 2024 19:08
@mmaietta mmaietta changed the title Fix: Respect unpack minimatch for symlinks within previously unpacked directories fix: Respect unpack minimatch for symlinks within previously unpacked directories Dec 14, 2024
@BlackHole1
Copy link
Member

BlackHole1 commented Dec 20, 2024

Note: This implementation is isolated to mac/linux. It seems symlinks created on Windows (mklink) do not get properly committed to git, they seem to be committed as an actual file and I couldn't figure out why

I encountered this problem before when reviewing #308. Creating symlinks on Windows and uploading them to GitHub will cause CI to fail. I also tried creating symlinks in git bash, but it still didn't work. Perhaps Windows symlinks and Unix symlinks are completely incompatible? I'm not sure.

The method I used is to create them dynamically during testing, rather than using a fixtures-like solution. See:

/**
* Directory structure:
* tmp
* ├── private
* │ └── var
* │ ├── app
* │ │ └── file.txt -> ../file.txt
* │ └── file.txt
* └── var -> private/var
*/
const tmpPath = path.join(__dirname, '..', 'tmp');
const privateVarPath = path.join(tmpPath, 'private', 'var');
const varPath = path.join(tmpPath, 'var');
fs.mkdirSync(privateVarPath, { recursive: true });
fs.symlinkSync(path.relative(tmpPath, privateVarPath), varPath);

@mmaietta
Copy link
Contributor Author

Thanks @BlackHole1 ! I'll get working right on that.

@mmaietta mmaietta marked this pull request as draft December 24, 2024 19:07
… (same approach as has been taken for filesystem UT already)
@mmaietta mmaietta force-pushed the fix/static-framework-symlinks-2 branch from 82ff252 to 5f886a8 Compare December 25, 2024 15:37
@mmaietta mmaietta marked this pull request as ready for review December 25, 2024 15:49
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.

electron builder v26.0.0-alpha.2 fails to copy native deps with sym links
3 participants