Allow symlinks in MountFS #5474
Conversation
|
The tests seem to accept that, so I'd tend to be fine with merging this since it shouldn't impact existing codebases 🤔 |
There was a problem hiding this comment.
This used to be supported until it was removed for "performance" reasons in #1474
Removing support for symlinks wasn't the goal of that PR and since no tests failed the regression wasn't spotted.
Please add a test so we can avoid future regressions, I see the linked issue contains one which can be added to https://github.com/yarnpkg/berry/blob/e20f8369510e2c1c9e82cae2bef7d96e05dbffed/packages/acceptance-tests/pkg-tests-specs/sources/pnp.test.js
thatsmydoing
force-pushed
the
allow-symlink-zip
branch
4 times, most recently
from
19a5d76 to
79c9f98
Compare
|
I've added the tests. The acceptance tests were timing out on windows so I just excluded them. For some reason, the unit test works though so I'm just letting it run unconditionally. |
|
Side note: I'm getting this error on Vercel when setting
|
thatsmydoing
force-pushed
the
allow-symlink-zip
branch
from
79c9f98 to
ac174f8
Compare
thatsmydoing
force-pushed
the
allow-symlink-zip
branch
from
ac174f8 to
c5d15d5
Compare
|
@merceyz are there any other changes you want me to make? |
|
@merceyz Sorry for pinging, but could this be merged or are there any blockers? |
|
@thatsmydoing would you be able to rebase on master? |
thatsmydoing
force-pushed
the
allow-symlink-zip
branch
from
c5d15d5 to
c323d3a
Compare
|
@arcanis Any way this could be merged? |
|
Hey @thatsmydoing @merceyz -- is this safe to be merged? |
thatsmydoing
force-pushed
the
allow-symlink-zip
branch
from
c323d3a to
f4aea20
Compare
|
@thatsmydoing I have asked on the Discord, and the Yarn team would need permissions to update your fork: https://discord.com/channels/226791405589233664/1297852972713377863 |
|
It seems I can't grant permissions since I've forked it in an org and not my personal account. I can go and rebase this on master though. |
thatsmydoing
force-pushed
the
allow-symlink-zip
branch
from
f4aea20 to
870da46
Compare
This is the same as #5474 but allows edits by maintainer **What's the problem this PR addresses?** <!-- Describe the rationale of your PR. --> Yarn shouldn't care if zips in the cache are symlinks or not. The only reason it doesn't work is that finding the mount point skips zips if they're not regular files. This used to be supported until it was removed for "performance" reasons in #1474. The bulk of the logic in the linked PR involves resolving the real path of the symlink but I don't think that's needed for this? <!-- Link all issues that it closes. (Closes/Resolves #xxxx.) --> Resolves #3514 Closes #5474 (supersedes it) **How did you fix it?** I switched the check to use `stat` from `lstat`. **Checklist** <!--- Don't worry if you miss something, chores are automatically tested. --> <!--- This checklist exists to help you remember doing the chores when you submit a PR. --> <!--- Put an `x` in all the boxes that apply. --> - [X] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing). <!-- See https://yarnpkg.com/advanced/contributing#preparing-your-pr-to-be-released for more details. --> <!-- Check with `yarn version check` and fix with `yarn version check -i` --> - [X] I have set the packages that need to be released for my changes to be effective. <!-- The "Testing chores" workflow validates that your PR follows our guidelines. --> <!-- If it doesn't pass, click on it to see details as to what your PR might be missing. --> - [x] I will check that all automated PR checks pass before the PR gets reviewed. --------- Co-authored-by: merceyz <[email protected]>
What's the problem this PR addresses?
Yarn shouldn't care if zips in the cache are symlinks or not. The only reason it doesn't work is that finding the mount point skips zips if they're not regular files. This used to be supported until it was removed for "performance" reasons in #1474. The bulk of the logic in the linked PR involves resolving the real path of the symlink but I don't think that's needed for this?
Resolves #3514
How did you fix it?
I switched the check to use
statfromlstat.Checklist