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

Make symlink behavior configurable #141

Closed
mat-sz opened this issue Oct 5, 2024 · 10 comments · Fixed by electron-userland/electron-builder#8576
Closed

Make symlink behavior configurable #141

mat-sz opened this issue Oct 5, 2024 · 10 comments · Fixed by electron-userland/electron-builder#8576

Comments

@mat-sz
Copy link

mat-sz commented Oct 5, 2024

Symlink resolution behavior introduced in 189519a breaks yarn workspaces.

Yarn tends to symlink things around when it comes to workspaces, so using any version after v5.0.0-alpha.3 results in an error similar to this (from electron-builder):

  ⨯ Path does not end with the package name  failedTask=build stackTrace=Error: Path does not end with the package name
    at getRealSource (/metastable/node_modules/app-builder-lib/src/util/appFileCopier.ts:194:13)
    at computeNodeModuleFileSets (/metastable/node_modules/app-builder-lib/src/util/appFileCopier.ts:225:24)
    at /metastable/node_modules/app-builder-lib/src/platformPackager.ts:401:34
    at async Promise.all (index 0)
    at AsyncTaskManager.awaitTasks (/metastable/node_modules/builder-util/src/asyncTaskManager.ts:65:25)
    at LinuxPackager.doPack (/metastable/node_modules/app-builder-lib/src/platformPackager.ts:293:5)
    at LinuxPackager.pack (/metastable/node_modules/app-builder-lib/src/platformPackager.ts:138:5)
    at Packager.doBuild (/metastable/node_modules/app-builder-lib/src/packager.ts:459:9)
    at executeFinally (/metastable/node_modules/builder-util/src/promise.ts:12:14)
    at Packager.build (/metastable/node_modules/app-builder-lib/src/packager.ts:393:31)
    at executeFinally (/metastable/node_modules/builder-util/src/promise.ts:12:14)

Reverting that change and rebuilding app-builder resolves those issues.

I can provide a minimal example if needed.

@mmaietta
Copy link
Collaborator

mmaietta commented Oct 5, 2024

@beyondkmp can you please take a look at this when you have a chance?

@beyondkmp
Copy link
Contributor

This has been fixed in electron-userland/electron-builder#8560

@mat-sz
Copy link
Author

mat-sz commented Oct 6, 2024

@beyondkmp Thanks, I'll check it out once released and report back. Seems like it's a different problem and in my case it would break with a different directory structure (due to resolved symlinks), though.

@beyondkmp
Copy link
Contributor

@mat-sz v25.1.8 has been released. You can help test it now if you have a chance.

@mat-sz
Copy link
Author

mat-sz commented Oct 7, 2024

@beyondkmp Different issue now, seems also related to symlinks:

 ⨯ /metastable/packages/metastable/eslint.config.js must be under /metastable/packages/client/  failedTask=build stackTrace=Error: /metastable/packages/metastable/eslint.config.js must be under /metastable/packages/client/
    at getRelativePath (/metastable/node_modules/app-builder-lib/src/util/filter.ts:32:13)
    at AsarPackager.unpackPattern (/metastable/node_modules/app-builder-lib/src/util/filter.ts:57:22)
    at AsarPackager.createPackageFromFiles (/metastable/node_modules/app-builder-lib/src/asar/asarUtil.ts:129:82)
    at processTicksAndRejections (node:internal/process/task_queues:105:5)
    at AsarPackager.pack (/metastable/node_modules/app-builder-lib/src/asar/asarUtil.ts:49:41)
    at /metastable/node_modules/app-builder-lib/src/platformPackager.ts:439:11
    at async Promise.all (index 0)
    at AsyncTaskManager.awaitTasks (/metastable/node_modules/builder-util/src/asyncTaskManager.ts:65:25)
    at LinuxPackager.doPack (/metastable/node_modules/app-builder-lib/src/platformPackager.ts:293:5)
    at LinuxPackager.pack (/metastable/node_modules/app-builder-lib/src/platformPackager.ts:138:5)
    at Packager.doBuild (/metastable/node_modules/app-builder-lib/src/packager.ts:459:9)
    at executeFinally (/metastable/node_modules/builder-util/src/promise.ts:12:14)
    at Packager.build (/metastable/node_modules/app-builder-lib/src/packager.ts:393:31)
    at executeFinally (/metastable/node_modules/builder-util/src/promise.ts:12:14)

@beyondkmp
Copy link
Contributor

@mat-sz can you share a minimum reproducible repo for this by chance?

@mat-sz
Copy link
Author

mat-sz commented Oct 8, 2024

@beyondkmp

https://github.com/mat-sz/app-builder-workspace-bug-reproduction

Few notes:

  1. Further testing revealed that this issue is caused by putting a dependency of a workspace dependency in asarUnpack. In my real project I'm using this for sharp (native dependency). This happens with any dependency though, here: @example/client -> @example/common -> is-odd. The same setup wasn't a problem in the older version I mentioned.
  2. I'm testing this on Linux.

beyondkmp added a commit to electron-userland/electron-builder that referenced this issue Oct 10, 2024
fix develar/app-builder#141

reproduced error info
```
  ● yarn several workspaces

    /tmp/et-db411cd21f3bbd882f3a5a30c58db6ff/t-kIKFAh/test-project-2/packages/foo/package.json must be under /tmp/et-db411cd21f3bbd882f3a5a30c58db6ff/t-kIKFAh/test-project-2/packages/test-app/

      30 |     const index = file.indexOf(NODE_MODULES_PATTERN)
      31 |     if (index < 0) {
    > 32 |       throw new Error(`${file} must be under ${srcWithEndSlash}`)
         |             ^
      33 |     } else {
      34 |       return file.substring(index + 1 /* leading slash */)
      35 |     }

      at getRelativePath (../packages/app-builder-lib/src/util/filter.ts:32:13)
      at AsarPackager.unpackPattern (../packages/app-builder-lib/src/util/filter.ts:57:20)
      at AsarPackager.createPackageFromFiles (../packages/app-builder-lib/src/asar/asarUtil.ts:129:82)
      at AsarPackager.pack (../packages/app-builder-lib/src/asar/asarUtil.ts:49:41)
      at ../packages/app-builder-lib/src/platformPackager.ts:439:11
          at async Promise.all (index 0)
      at AsyncTaskManager.awaitTasks (../packages/builder-util/src/asyncTaskManager.ts:65:25)
      at LinuxPackager.doPack (../packages/app-builder-lib/src/platformPackager.ts:293:5)
      at LinuxPackager.pack (../packages/app-builder-lib/src/platformPackager.ts:138:5)
      at Packager.doBuild (../packages/app-builder-lib/src/packager.ts:459:9)
      at executeFinally (../packages/builder-util/src/promise.ts:12:14)
      at Packager.build (../packages/app-builder-lib/src/packager.ts:393:31)
      at packAndCheck (src/helpers/packTester.ts:188:41)
      at src/helpers/packTester.ts:132:36
      at executeFinally (../packages/builder-util/src/promise.ts:12:14)
```

Currently, the returned node_modules paths are no longer symlinks, but
resolved paths. For example, packages in pnpm workspace/yarn workspace
are not under node_modules.

All previous getRealSource/getRelativePath methods were based on
node_modules in the path, which was relatively tricky.


https://github.com/electron-userland/electron-builder/blob/a25d04d5a8e58b447f0462673a4362414da9ed27/packages/app-builder-lib/src/util/appFileCopier.ts#L191-L203


https://github.com/electron-userland/electron-builder/blob/a25d04d5a8e58b447f0462673a4362414da9ed27/packages/app-builder-lib/src/util/filter.ts#L28-L36




Solution: Directly include relativeNodeModulesPath in the file stats.
This is simple and easy to understand.

---------

Co-authored-by: Mike Maietta <[email protected]>
@mmaietta
Copy link
Collaborator

@mat-sz fix has landed in electron-builder v26.0.0-alpha.1. Please give it a shot when you have a chance

@mat-sz
Copy link
Author

mat-sz commented Oct 11, 2024

@mmaietta Thank you! Works perfectly fine on Linux.

I'll try it out on Windows and macOS tomorrow and let you know if everything is also fine there.

@mat-sz
Copy link
Author

mat-sz commented Oct 17, 2024

@mmaietta Apologies for the delayed response - everything works perfectly fine across all 3 platforms. Thank you once again.

@mat-sz mat-sz closed this as completed Oct 17, 2024
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 a pull request may close this issue.

3 participants