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: allow traversal of symlinks in glob #2134

Merged
merged 2 commits into from Jan 7, 2023

Conversation

boneskull
Copy link
Contributor

Closes #2130

This adds a followSymlinks option to the glob() function, and enables it when searching for entry points.

If true, and a symbolic link is encountered, a stat will be taken of the symlink's target. If a dir or file, the symlink is handled like any other dir or file. If a symbolic link itself (symlink to a symlink), we take a stat of the next until we find a dir or file, then handle it.

There is a little bit of caching to avoid extra I/O, and protection from recursive symlinks. However, it's possible (though unlikely) that the FS could cause a "max call stack exceeded" exception. If someone runs into this, we can change the implementation to a loop instead of a recursive function.

Apologies for blasting the do..while. I love a good do myself, but splitting out the lambda functions make it untenable.

@boneskull
Copy link
Contributor Author

I should also note that I'm unsure how well glob() works on Windows. I didn't feel confident that swapping "/" for path.sep was a good idea though.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Dec 31, 2022

I didn't feel confident that swapping "/" for path.sep was a good idea though.

Your intuition was correct - minimatch expects paths to be joined with /. I'll test this on a Windows box before merging.

@Dayday10
Copy link

Good looking out.

@boneskull
Copy link
Contributor Author

I confirmed it myself: this doesn't work on Windows. 😄

Will get it working

@boneskull
Copy link
Contributor Author

(FWIW, the "glob handles root match" test fails on Windows in master; I'll fix that too)

Closes TypeStrong#2130

This adds a `followSymlinks` option to the `glob()` function, and enables it when searching for entry points.

If `true`, and a symbolic link is encountered, a stat will be taken of the symlink's target. If a dir or file, the symlink is handled like any other dir or file.  If a symbolic link itself (symlink to a symlink), we take a stat of the next until we find a dir or file, then handle it.

There is a little bit of caching to avoid extra I/O, and protection from recursive symlinks.  However, it's possible (though unlikely) that the FS could cause a "max call stack exceeded" exception.  If someone runs into this, we can change the implementation to a loop instead of a recursive function.

Apologies for blasting the `do..while`.  I love a good `do` myself, but splitting out the lambda functions make it untenable.
@boneskull
Copy link
Contributor Author

OK, I have the tests passing in Windows (Win 10, PowerShell). The code itself was fine, but the tests made poor assumptions.

The test fix I made for an unrelated failure on Windows (6c1b4ac) is dubious and I think it'd necessitate a PR into typestrong/fs-fixture-builder to correct the underlying problem. What I was seeing was that fix.cwd contained both POSIX and Win32 path separators. Likely there's a path.resolve() or path.join() in there somewhere that needs to be path.posix.x() to jive with minimatch

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jan 7, 2023

That's fun, the normalize is a good idea anyways - I'd rather not care what separators fix.cwd uses, I bet tests have been broken on Windows for a while, haven't booted it up to test for a few months. Will get to this today :)

@Gerrit0 Gerrit0 merged commit 26480e7 into TypeStrong:master Jan 7, 2023
@Gerrit0
Copy link
Collaborator

Gerrit0 commented Jan 7, 2023

Thanks!

@boneskull boneskull deleted the boneskull/issue2130 branch January 10, 2023 17:36
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.

cannot use symlinked entry points with 'packages' strategy
3 participants