-
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: fix symlink pointing error in filesystem.insertLink #301
fix: fix symlink pointing error in filesystem.insertLink #301
Conversation
Happy Chinese new year to @BlackHole1! Let me answer the two questions you asked. In our scenarios, we use |
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.
Thank you for answering my questions. I don't think addressing your "requirement" is the responsibility of asar, but I believe your PR is still meaningful because it does address a bug.
In the original implementation, if the case of A -> B -> C
exists, asar would lose B (due to fs.realpathSync
returning the last non-symlink path).
In your fix, you use fs.readlinkSync
to circumvent this issue, and I think this solution is sound.
PTAL @erickzhao @dsanders11
🎉 This PR is included in version 3.2.9 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Hi, after this change I can't package my project
|
@viplifes thanks for the report, we'll take a look. For the time being, you can downgrade the |
@viplifes, I'm not able to repro this locally. Could you give more detailed information about the versions of packages you're using? Specifically versions of Electron Forge and |
@dsanders11 Here is an example of a project that reproduces this problem https://github.com/viplifes/testapp_asar I think there is a problem module: open -> is-inside-container -> is-docker |
Thank you @viplifes! That repro case is very helpful. It appears the issue on macOS is one of the paths starting with |
In MacOS, if I run
asar.createPackage()
to pack a folder into.asar
, which contains "symlink", it would get wrong after I runasar.extractAll()
from the generated.asar
.Let's look at this Beyond Compare result:
The folder in left is the origin one, and the folder in right was generated by using
asar.extractAll()
from the origin one's.asar
( folderLeft --> folderLeft.asar --> folderRight).Inside the left folder, file Resources were pointed to
Versions/Current/Resources
, but the same file inside the right folder were pointed toVersions/A/Resources
, it's wrong.I fixed this bug and added a test unit. I create a new folder in
test/input/packthis-with-symlink
, then create a/Current
symlink folder from folder/A
, then create areal.txt
from folder/Current
:We could see that the new
real.txt
were pointing toCurrent/real.txt
.After preparing these materials, I made a new test unit:
and updated the
compFiles()
function:The new
compFiles()
function would detect whether the file is a symlink, and compare the real pointing byfs.readlinkSync()
. With the bug infilesystem.js
before I fixed, the test would throw an error:After fixing this bug, everything runs well.