fix: Skip lipo if native module is already universal. Add native module fixtures for lipo tests

[mmaietta]

Leveraged TDD from it.todo to discover issue of build failing due to universal file not being added to knownMergedMachOFiles

• If same file in both asars, error message: Detected file "Contents/Resources/app/node-mac-permissions.node" that's the same in both x64 and arm64 builds and not covered by the x64ArchFiles rule: "undefined"
• If different sha, build fails due to lipo attempting to merge universal files

Fixes #130

Functionality added:

• Detect if MachO file is already universal in both apps, if so, skip lipo step
• Extracts common logic isUniversalMachO(buffer: Buffer) to leverage MachO magic header validation in multiple locations

Testability:

• Resolves 3 it.todo test cases and adds some more 💪
  • identical app dirs with universal macho files (e.g., do not shim, just copy x64 dir)
  • different app dirs with universal macho files (e.g. shim but don't lipo)
  • identical app dirs with different macho files (e.g. do not shim, but still lipo)
  • different app dirs with different macho files (e.g. shim and lipo)
  • works for lipo binary resources

Fixtures added:

• hello-world.c is generated into macho files at test runtime (~1 second added in global test setup) and lipo'd together for a universal fixture.

[mmaietta]

Quick update:

Well this is interesting. Now I'm receiving this in the debug logs:

2025-02-28T23:57:53.887Z electron-universal hashing /Users/mikemaietta/Development/universal/test/fixtures/apps/LipoX64.app/Contents/Resources/app.asar.unpacked/node-mac-permissions.node
2025-02-28T23:57:53.887Z electron-universal hashing /Users/mikemaietta/Development/universal/test/fixtures/apps/LipoArm64.app/Contents/Resources/app.asar.unpacked/node-mac-permissions.node
2025-02-28T23:57:53.888Z electron-universal joining two MachO files with lipo {
  first: '/private/var/folders/y3/r1xf5xzn5pddms2gslqh14sh0000gn/T/electron-universal-GVeAlH/Tmp.app/Contents/Resources/app.asar.unpacked/node-mac-permissions.node',
  second: '/Users/mikemaietta/Development/universal/test/fixtures/apps/LipoArm64.app/Contents/Resources/app.asar.unpacked/node-mac-permissions.node'
}
2025-02-28T23:57:53.903Z electron-universal merging x64 and arm64 asars
2025-02-28T23:57:53.903Z electron-universal merging /var/folders/y3/r1xf5xzn5pddms2gslqh14sh0000gn/T/electron-universal-GVeAlH/Tmp.app/Contents/Resources/app.asar and /Users/mikemaietta/Development/universal/test/fixtures/apps/LipoArm64.app/Contents/Resources/app.asar

    Can't reconcile two non-macho files node-mac-permissions.node

It's almost like asar.extractFile isn't able to return a Buffer that has the macho prefix, even though the lipo was succesful. Debugging this further still, but wanted to provide an update on why the tests are failing now.

The Buffer for x64 is basically twice the size of the arm64, so something else is happening that is unexpected.

Moving this back into Draft state

[UPDATE]

Debugging it was straightforward, but I could use some of your guidance @erikian . The x64 .node is indeed a universal macho file during mergeAsars, but the arm64 .node is still arm64, due to this code:

universal/src/index.ts

Lines 190 to 196 in 2b67c90

	await spawn('lipo', [
	first,
	second,
	'-create',
	'-output',
	await fs.realpath(path.resolve(tmpApp, machOFile.relativePath)),
	]);

It outputs the universal .node to the x64 tmpApp copy, which then is attempted to be merged. The reason this worked before was due to app.asar.unpacked files were all detected as AppFileType.APP_CODE instead of being processed as AppFileType.MACHO enum., which was discussed in wg-ecosystem. This change to endsWith(".asar") from includes("app.asar") was introduced in 2b67c90#diff-980e8022478ab1ab100eaffa7a0fd231015a2ccffdf2636887bc4eea04f0c2ccL48-R48

It looks like the arm64 app may need to be copied as well to a tmp directory, which then also has the lipo'd universal binary also present (same process we already take with the x64 version cp -R logic), otherwise the mergeAsars will be comparing a universal .node with the already existing x64 .node unmerged version. What are your thoughts?

[erikian]

@mmaietta all tests pass for me after changing this...

universal/src/asar-utils.ts

Lines 155 to 157 in 7e3167b

	if (isUniversalMachO(x64Content) && isUniversalMachO(arm64Content)) {
	continue;
	}

...into this:

if (isUniversalMachO(x64Content)) {
  continue;
}

From what I could gather, since we're already joining the single-arch Mach-O files into one universal file in...

universal/src/index.ts

Lines 190 to 196 in 2b67c90

	await spawn('lipo', [
	first,
	second,
	'-create',
	'-output',
	await fs.realpath(path.resolve(tmpApp, machOFile.relativePath)),
	]);

...as you mentioned, when x64Content (which comes from the .asar that we're generating in...

universal/src/index.ts

Lines 255 to 258 in 7e3167b

	await asar.createPackage(
	entryAsar,
	path.resolve(tmpApp, 'Contents', 'Resources', 'app.asar'),
	);

..., not from any user-provided .asar) is a universal Mach-O file in mergeASARs, we should skip that file, otherwise we'd try to join the already-universal "x64" Mach-O file and the arm64 version into another universal file in...

universal/src/asar-utils.ts

Lines 197 to 200 in 7e3167b

	const destination = await fs.realpath(path.resolve(x64Dir, binding));
	d(`merging binding: ${binding}`);
	execFileSync(LIPO, [source, destination, '-create', '-output', destination]);

Notice that we never touch the arm64 version in the last snippet — we only use it as one of the inputs for generating a new universal file —, so it's safe to remove the isUniversalMachO(arm64Content)) check because we don't really care about the arm64 version when our "x64" version is already universal.

The naming is a bit confusing throughout the project because we start out by copying the x64 app into a temp folder that will eventually become our final universal bundle, so in a lot of places where we use x64Foo we should probably use universalFoo to make it clear that we're not dealing with the user-provided x64 input app, but with our work-in-progress universal output.

Hope that helps!

[mmaietta]

That's amazing, thank you for the advice and investigation! That makes perfect sense.

The naming is a bit confusing throughout the project because we start out by copying the x64 app into a temp folder that will eventually become our final universal bundle, so in a lot of places where we use x64Foo we should probably use universalFoo to make it clear that we're not dealing with the user-provided x64 input app, but with our work-in-progress universal output.

Would you like me to add some const renaming as part of this PR?
I think I could do that in a subsequent PR while also extracting common logic to individual files. (I had previously started on this in #121 but was waiting on getting the snapshot tests integration in first)

[mmaietta]

@erikian would you mind taking a look at this PR again? I think it may resolve this issue as well: #130

[erikian]

Would you mind adding some logic for installing / cloning a specific version of node-mac-permissions and generating the native artifacts locally before running the tests for better reproducibility?

[mmaietta]

As in add to the README.txt instructions to do so or to add additional logic to the jest setup script to clone and build locally?

[mmaietta]

Nvm, ignore the comment above. @MarshallOfSound recommended I just create a basic HelloWorld.s file as a fixture, compile into a macho file during runtime, and then lipo at runtime as well into the fixture output dir. Total time should only add a few seconds to the test duration. 🙂

[mmaietta]

Updated unit tests to runtime-generate a macho file off a very simple hello-world.c. Note, this did require adjusting the snapshot logic to remove blocks, hash, and integrity fields, specifically when it contains a runtime-generated asset as the macho file in this case is always a unique hash per-build machine.

[erikian]

Thanks Mike, and sorry for the delay reviewing this PR — I'll take another look in the next couple of days!

[BlackHole1]

LGTM