-
Notifications
You must be signed in to change notification settings - Fork 250
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: relative link error #308
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This modification is related to #301. Can you try the solution that @dsanders11 commented on in #301?
This problem is quite complex, and I think I need to provide a detailed description here so that more people can understand the whole context: In previous versions, asar would ignore the “intermediate” symbolic links. For example, I have three files: A, B, and C, with the relationship being: About a month ago, @jrainlau discovered this issue and submitted a related fix: #301. In his fix, he used
At that time, we didn’t notice any issues until 3 weeks ago when @viplifes reported a problem. When processing with asar, it incorrectly assumed that the target file was outside the src directory. At that time, @dsanders11 proposed a fix but didn’t receive a response from @jrainlau until today when @ziqiangai submitted an RP and verified that the modification proposed by @dsanders11 was not feasible. Next, I will explain why the fix code submitted by @jrainlau has this issue. Before we start, there are two prerequisites to understand:
// p = /var/.../Electron.app/Contents/Resources/app/node_modules/is-inside-container/node_modules/.bin/is-docker
insertLink (p) {
// symlink = ../is-docker/cli.js
const symlink = fs.readlinkSync(p)
// /var/.../Electron.app/Contents/Resources/app/node_modules/is-inside-container/node_modules/.bin
const parentPath = path.dirname(p)
// fs.realpathSync(this.src) = /private/var/.../Electron.app/Contents/Resources/app
// path.join(parentPath, symlink) = /var/.../Electron.app/Contents/Resources/app/node_modules/is-inside-container/node_modules/is-docker/cli.js
// link = ../../../../../../../../../../../../var/.../Electron.app/Contents/Resources/app/node_modules/is-inside-container/node_modules/is-docker/cli.js
const link = path.relative(fs.realpathSync(this.src), path.join(parentPath, symlink))
} I think everything will become clear after adding comments to the above code. Due to Lines 105 to 107 in d50b03f
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. It would be better if you could add a unit test for this!
It's a bit difficult for me. |
Signed-off-by: Kevin Cui <bh@bugs.cc>
a13b90e
to
5899d16
Compare
a340f59
to
880d020
Compare
NP 😃. I have pushed the relevant unit tests. |
In previous implementations, the result obtained from `path.relative(realSrcPath, target)` is likely to be `../../../../../../../../../var/`. Although it has been tested and does not affect the program's execution, it looks very messy. To avoid this issue, modify `realSrcPath` to `this.src`. Signed-off-by: Kevin Cui <bh@bugs.cc>
🫡 Congratulations, thank you for letting me learn a lot. |
The fix can be improved (it's not complete), there are symlinks that resolve to an absolute path too (I ran into this with my Electron project), in which case, it should be ignored (they're usually executable commands like 'python', some dependencies generate these symlinks on building). The fix I propose is this: insertLink (p) {
const symlink = fs.readlinkSync(p)
// Can't really package these absolute links, they tend to be executable scripts like 'python'.
if (path.isAbsolute(symlink)) return
const parentPath = path.dirname(p)
const link = path.relative(fs.realpathSync(this.src), fs.realpathSync(path.join(parentPath, symlink)))
if (link.substr(0, 2) === '..') {
throw new Error(`${p}: file "${link}" links out of the package`)
}
const node = this.searchNodeFromPath(p)
node.link = link
return link
} |
@sethyuan Thank you for your additional information! I will conduct research in the next few days. It would be even better if you can provide a reproducible repo, as this can save me a lot of time.😃 |
@BlackHole1 My environment is macOS 14.4.1 (arm64), I have miniconda installed and I'm using an environment created with it. The following steps can reproduce the packaging issue with symlinks:
Basically, it uses electron-forge to build and package the app and better-sqlite3 will build the sqlite3 lib from code leaving a symlink (absolute path) to the 'python' used, which in this case, is the one from miniconda's environment. Here is a screen capture of the error: |
@sethyuan Thanks. If I have enough time today, I will follow up on this issue today. |
I'm also encountering a similar issue on OSX 14.2.1 (arm64). The A fix to make it work in that instance was to add this to forge.config.ts:
I suppose that would also work for But in my particular case one of my app dependencies (@istanbuljs) is using They all seem to be linked to something upstream, something related to symlink resolution. |
Signed-off-by: Kevin Cui <bh@bugs.cc>
3a584ae
to
eaf970c
Compare
I am sorry for the delay. I have been really busy lately. Thank you @sethyuan and @bstst for the feedback. I have just rethought the repair plan and realized that it does not require a fix as initially discussed by #308 (comment). The main cause of this bug is to address the issue from @ziqiangai @jrainlau @sethyuan @bstst Can you try the latest repair solution? (It has been verified on my local system already.) |
The latest solution fixes the issue on my local system 🎉 I do have another somewhat related issue, but am not sure who's to blame: My project has a dependency on the
That comes from them having a dependency on "nyc" in their "dependencies" section. If I move it to "devDependencies" it works fine. Which makes sense since it's all tests. But the actual error comes, I believe, from the fact that they're all symlinks and it tries to copy a symlink into a symlink or something like that (I didn't spend much time on that). Is the "fs-extra" package here to blame (jprichardson/node-fs-extra#1019)? |
Yes, it does work for me, thanks! |
Does not seem like this package is related to the other issue that I'm getting ("fs-extra".copy is being called directly from the @electron-forge/vite-plugin, so it looks like the issue is indeed on the "fs-extra" side, will continue with them). So it looks like this PR does indeed fix the "links out of the package" issue. Thanks for the effort everyone! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this manually:
-
filesystem-spec.js
fails if changes tolib/filesystem.js
are reverted. - With a sample Forge app that fails
yarn package
on@electron/asar@3.2.9
, this fixes the error and the packaged module works correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nits
🎉 This PR is included in version 3.2.10 🎉 The release is available on: Your semantic-release bot 📦🚀 |
I have this same issue even after verifying that my asar package is at version 3.2.10. any ideas on how to fix this? seems to happen when I add this package @atlaskit/editor-common the error
|
@twann503 I don't think this is the same error. The error for this one is |
very similar to @bstst error though with being unable to copy because the symlink issue. |
fix the fllowing error in
electron-forge
framework