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

Fix relative symlink paths #293

Merged
merged 1 commit into from
Oct 28, 2017
Merged

Fix relative symlink paths #293

merged 1 commit into from
Oct 28, 2017

Conversation

jswidler
Copy link
Contributor

@jswidler jswidler commented Oct 27, 2017

I found a bug with relative symlinks. The linkname would get confused and resolve to a relative path that included the location of my node.js server, that had nothing to do with the path of the symlink (either the symlink or the location of the file it pointed to).

For instance, I creeated a relative symlink named thing2 inside the directory /workspace.
The symlink points to a file with absolute location of /workspace/x/y/z/thing, but the symlink is relative and so uses x/y/z/thing

The variables in scope of the changes work out as follows:

task.filepath == "/workspace/thing2"
linkpath ==  "x/y/z/thing"

BEFORE CHANGE:

path.resolve(linkPath) == "/Users/jesse/dev/gravity/roles/spacegate/files/workspacesync/x/y/z/thing"
path.relative(...) == "../Users/jesse/dev/gravity/roles/spacegate/files/workspacesync/x/y/z/thing"

AFTER CHANGE:

path.relative(...) ==  "x/y/z/thing"

@jswidler
Copy link
Contributor Author

I'm looking into the broken test so I can make a better fix.

@jswidler
Copy link
Contributor Author

Looks like the test is broken as well; it creates a symlink to a file that does not actually exist.

@jswidler
Copy link
Contributor Author

jswidler commented Oct 27, 2017

I fixed the test as well. Unfortunately, the behavior of fs.symlinkSync is confusing and the documentation does not help to confirm my changes are legit.

On both my own system and the CI system, I believe the original call to fs.symlink created a file like so:

level0link.txt -> test/fixtures/directory/level0.txt

However, relative to level0link.txt. the path should be ../level0.txt . Indeed - When I removed the unlink function calls at the end of the test and checked the symlinks, they were already pointing to files that did not exist. The bug that I fixed caused this library to change the broken symlink, which had the affect of ... "fixing" ... the symlink and allowing the test to pass.

@ctalkington
Copy link
Member

thanks for taking the time to outline the issue.

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 this pull request may close these issues.

2 participants