electron-builder icon indicating copy to clipboard operation
electron-builder copied to clipboard

fix(linux): native modules overwritten with wrong arch

Open megahertz opened this issue 1 year ago • 9 comments

Fixes: #7608

Description When building an app for multiple Linux architectures, native modules are hardlinked to the modules in appDir/node_modules/**/*.node if CI env is set.

When the builder processes the first arch, it works fine. The second arch overwrites appDir/node_modules/**/*.node. Since these files are hardlinked to linux-*unpacked/resources/app.asar.unpacked/node_modules/**/*.node it overwrites native modules for all archs, so now the first arch distributive is broken.

FileCopier in builder-util/out/fs copies files by default, but once it detects a CI environment, it switches to hadlinking mode instead.

Example package.json which reproduces the issue
{
  "name": "builder-arm64-linux-issue",
  "version": "1.0.0",
  "main": "index.js",
  "scripts": {
    "build": "env CI=1 electron-builder --linux --x64 --arm64 && find dist/linux-unpacked -name realm.node -exec file {} \\;"
  },
  "build": {
    "linux": {
      "target": {
        "arch": ["x64", "arm64"],
        "target": "dir"
      }
    }
  },
  "devDependencies": {
    "electron": "^29.1.0",
    "electron-builder": "^24.13.3"
  },
  "dependencies": {
    "realm": "^12.6.2"
  }
}

megahertz avatar Mar 07 '24 13:03 megahertz

🦋 Changeset detected

Latest commit: 702a291e7f45fa2cd5e104a54bd587410ca83db5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages
Name Type
app-builder-lib Patch
dmg-builder Patch
electron-builder-squirrel-windows Patch
electron-builder Patch
electron-forge-maker-appimage Patch
electron-forge-maker-nsis-web Patch
electron-forge-maker-nsis Patch
electron-forge-maker-snap Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Mar 07 '24 13:03 changeset-bot[bot]

Deploy Preview for car-park-attendant-cleat-11576 ready!

Name Link
Latest commit 702a291e7f45fa2cd5e104a54bd587410ca83db5
Latest deploy log https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/65e9c4955c328900082c2449
Deploy Preview https://deploy-preview-8107--car-park-attendant-cleat-11576.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Mar 07 '24 13:03 netlify[bot]

I'm wary of this change as I'm assuming the owner/author of this project had a reason for not having hard links always disabled. For your local project, why not use env var USE_HARD_LINKS=false?

mmaietta avatar Mar 08 '24 06:03 mmaietta

I'm wary of this change as I'm assuming the owner/author of this project had a reason for not having hard links always disabled. For your local project, why not use env var USE_HARD_LINKS=false?

It's faster to use hardlinks instead of copying. But this change affects only *.node files inside app.asar.unpacked, so it can't impact performance too much. Anyway, it would be nice to ask @develar whether there are some pitfalls or hadrlinks are used only for speed reasons.

Sure, I use this env as a workaround. But it took pretty much time for me to find the reason of this issue, so it would be nice to include this fix by default.

megahertz avatar Mar 08 '24 06:03 megahertz

Maybe it makes sense to disable hardlinking on the Linux build only since there is no such issue on macOS build?

megahertz avatar Mar 08 '24 07:03 megahertz

I really need @develar to chime in before moving forward with this PR. I do understand the need to resolve this issue, but I don't have the historical context to understand the reasoning for it to be disabled by default.

mmaietta avatar Mar 08 '24 16:03 mmaietta

@develar ?

thebalaa avatar Jun 26 '24 04:06 thebalaa

@thebalaa can you still reproduce this on next v25.0.1? I'm trying to get a local project set up with the package.json you specify but I think I'm doing something wrong. Would you be willing to put together a sample minimum reproducible repo and I can take a deeper look if the issue still persists?

mmaietta avatar Jul 23 '24 23:07 mmaietta

How does anyone contact @develar? I don't think he's been actively checking out this repo since 2022. @mmaietta Also, one more thing, I think this repo is community maintained as of 2024 and the current maintainers have the full rights to make new changes since stuff in the software world changes and we have to adapt according to that.

Even though there might be a very good reason behind whatever was implemented in this repo during the time when it was actively owned and maintained by @develar, it does not necessary mean imo that it should always stay like this. AFAIK, @develar did not also want autoupdates on targets other than AppImage, which @mmaietta implemented and was merged anyways.

There are a lot of things in this repo that are have a very unclear and vague "why" to them. That is why imo this repo deserves to move forward, and of course, if issues are discovered, you can always undo a change (before a release).

xyloflake avatar Aug 05 '24 12:08 xyloflake

Can you try this on latest electron-builder? Should be 25.0.5 at the time of writing this. If a minimum reproducible repo could be provided, I'd be happy to investigate further as well

mmaietta avatar Sep 19 '24 02:09 mmaietta

@mmaietta I will try it next week.

megahertz avatar Sep 20 '24 12:09 megahertz

It looks like it's fixed in 25.0.5 or earlier

megahertz avatar Sep 20 '24 12:09 megahertz