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

mmaietta · 2025-02-19T22:02:44Z

Technically this is a "fix" but I don't see any issue reported for it currently. 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

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:

Arch-specific modules generated from node-mac-permissions (https://github.com/codebytere/node-mac-permissions) using electron/rebuild
Universal module generated with lipo

lipo ./test/fixtures/native/node-mac-permissions.x64.node ./test/fixtures/native/node-mac-permissions.arm64.node -create -output ./test/fixtures/native/node-mac-permissions.universal.node

…e-mac-permissions` fixture from https://github.com/codebytere/node-mac-permissions and resolves 3 `it.todo` test cases

…ve-module-tests # Conflicts: # test/index.spec.ts

src/index.ts

…g into asar to leverage `unpack: "**/*.node"` properly.

erikian · 2025-02-22T14:52:30Z

test/util.ts

+    if (!fs.existsSync(resourcesApp)) {
+      await fs.mkdir(resourcesApp);
+    }
+    const { testPath } = await createTestApp(


note: considering that we have the templateApp and generateNativeApp functions whose job is to create a new .app bundle, createTestApp feels like a bit of a misnomer, it took me a while to realize that we're not creating an entirely new app here just to copy Resources/app or Resources/app.asar out of it.

Maybe we could rename it to prepareResourcesFolder / populateResourcesFolder / copyFilesToResourcesFolder or something along those lines to make its purpose clear?

Great callout. Will push a change! Note though, it's also used for no-asar mode, which is technically a Test App, right? So I'm not sure if prepare/populate correctly describes intent, since testPath is technically a completely contained no-asar app/resource

Maybe createStagingAppDir?

it's also used for no-asar mode, which is technically a Test App, right?

I guess I'm confused because it's not entirely clear whether "app" refers to the final .app bundle / distributable test artifact or to the contents of Resources/app, both could reasonably be called a "test app" IMO.

testPath is technically a completely contained no-asar app/resource

It is, but again it's not immediately clear from the variable name whether we're dealing with entire distributable artifacts or just with the contents of Resources/app. The fact that the object returned by createTestApp also contains an appPath property makes things even blurrier (that could be just me needing more coffee though).

Maybe createStagingAppDir?

I think that would work. If we add a comment to that function explaining that its job is to copy files to Resources/app it would also help a great deal.

jest.config.js

test/util.ts

test/index.spec.ts

erikian · 2025-02-22T16:04:00Z

test/index.spec.ts

+          {
+            'i-aint-got-no-rhythm.bin': 'boomshakalaka',
+          },


question: this file is being copied to Resources/app and not just Resources/arm64, I think we don't need the Resources/app folder at all in this case since we have the arch-specific app folders?

Yeah, that was a mistake in the fixture generation I just discovered. Pushed this change:
https://github.com/electron/universal/pull/126/files#diff-8951d3f508f54ec05c5df49f04f821a2aec5c75bce858639461f388e498a398cR202-R212

Summary:

Move native module into the testPath generated by createTestApp

If createAsar = true, then it's packaged into asar with unpack: "**/*.node"

if createAsar = false, just copy app dir to Resources dir

Just a sanity check to make sure we're talking about the same issue: for this test, I'm getting Resources/app, Resources/app-arm64, Resources/app-x64 and Resources/app.asar:

i-aint-got-no-rhythm.bin is a arm64-only file, and it's correctly included in Resources/app-arm64. What I don't understand is 1) why do we also have the arch-agnostic Resources/app folder in the bundle; and 2) why does it contain the arm64-only i-aint-got-no-rhythm.bin file? I think Resources/app either shouldn't be there or it should also include the x64-only hello-world.bin file, no?

Hmmm, I can definitely take a deeper look at this. Maybe something is wrong with the way the test fixture is being generated. I agree that Resources/app shouldn't be there IIRC, but I don't recall making any changes for that logic. My guess is that other "shim" tests may also exhibit the same behavior? I'll double check

Btw, many thanks for the in-depth review and quick replies! Really do appreciate it

Wait, I think this is expected behavior for No_Asar mode:

universal/src/index.ts

Lines 199 to 204 in d90d573

/**

* If we don't have an ASAR we need to check if the two "app" folders are identical, if

* they are then we can just leave one there and call it a day. If the app folders for x64

* and arm64 are different though we need to rename each folder and create a new fake "app"

* entrypoint to dynamically load the correct app folder

*/

The part I don't understand is why there's the i-aint-got-no-rythmn.bin is in that folder though. I'll keep investigating

Confirming that this is already existing behavior. Looks like it's been this way for years, albeit I'm not sure I understand what the logic flow is looking to achieve. Maybe .bin snapshots are expected to always be loaded from app dir?

Smells like a bug, but something I need more context on first to think through the flow

…function

mmaietta added 3 commits February 19, 2025 09:30

fix: when native modules are already universal, don't lipo. adds `nod…

01257e7

…e-mac-permissions` fixture from https://github.com/codebytere/node-mac-permissions and resolves 3 `it.todo` test cases

add test different app dirs with different macho files (shim and lipo)

5f15d75

add additional test

49e4ec2

mmaietta changed the title ~~fix: Skip lipo if native module is already universal. Add native module tests for lipo tests~~ fix: Skip lipo if native module is already universal. Add native module fixtures for lipo tests Feb 20, 2025

mmaietta marked this pull request as ready for review February 21, 2025 01:20

mmaietta requested a review from a team as a code owner February 21, 2025 01:20

Merge commit 'd90d573ccf69a5b14b91aa818c8b97e0e6840399' into add-nati…

977e346

…ve-module-tests # Conflicts: # test/index.spec.ts

erikian reviewed Feb 22, 2025

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

mmaietta added 3 commits February 21, 2025 21:25

PR feedback

e1871f3

gotta close fd

462527e

use stream to read first 4 bytes. copy native fixture before packin…

66f2c33

…g into asar to leverage `unpack: "**/*.node"` properly.

erikian reviewed Feb 22, 2025

View reviewed changes

mmaietta and others added 3 commits February 22, 2025 08:32

convert params to object

61abfd3

rename createTestApp to createStagingAppDir and add jsdoc to the …

67a8437

…function

Merge branch 'main' into add-native-module-tests

87412e6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mmaietta commented Feb 19, 2025 •

edited

Loading

erikian Feb 22, 2025

mmaietta Feb 22, 2025 •

edited

Loading

erikian Feb 22, 2025

erikian Feb 22, 2025

mmaietta Feb 22, 2025

erikian Feb 22, 2025

mmaietta Feb 22, 2025 •

edited

Loading

mmaietta Feb 23, 2025

mmaietta Feb 23, 2025 •

edited

Loading

	/**
	* If we don't have an ASAR we need to check if the two "app" folders are identical, if
	* they are then we can just leave one there and call it a day. If the app folders for x64
	* and arm64 are different though we need to rename each folder and create a new fake "app"
	* entrypoint to dynamically load the correct app folder
	*/

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

Are you sure you want to change the base?

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

Conversation

mmaietta commented Feb 19, 2025 • edited Loading

erikian Feb 22, 2025

Choose a reason for hiding this comment

mmaietta Feb 22, 2025 • edited Loading

Choose a reason for hiding this comment

erikian Feb 22, 2025

Choose a reason for hiding this comment

erikian Feb 22, 2025

Choose a reason for hiding this comment

mmaietta Feb 22, 2025

Choose a reason for hiding this comment

erikian Feb 22, 2025

Choose a reason for hiding this comment

mmaietta Feb 22, 2025 • edited Loading

Choose a reason for hiding this comment

mmaietta Feb 23, 2025

Choose a reason for hiding this comment

mmaietta Feb 23, 2025 • edited Loading

Choose a reason for hiding this comment

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

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

mmaietta commented Feb 19, 2025 •

edited

Loading

mmaietta Feb 22, 2025 •

edited

Loading

mmaietta Feb 22, 2025 •

edited

Loading

mmaietta Feb 23, 2025 •

edited

Loading