Skip to content

test: add a new test case for global cache #10

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

test: add a new test case for global cache #10

wants to merge 6 commits into from

Conversation

JounQin
Copy link
Contributor

@JounQin JounQin commented Jun 2, 2025

No description provided.

@JounQin JounQin mentioned this pull request Jun 2, 2025
Merged
@JounQin JounQin changed the title chore: reproduce global cache issue test: add a new test case for global cache Jun 2, 2025
@JounQin JounQin force-pushed the repro/global-cache branch 8 times, most recently from 2e881ab to 504e487 Compare June 2, 2025 20:16
@JounQin
Copy link
Contributor Author

JounQin commented Jun 2, 2025 •
edited
Loading

https://github.com/yarnpkg/pnp-rs/actions/runs/15409138112/job/43357484921?pr=10#step:6:162

manifest.manifest_dir, D:\a\pnp-rs\pnp-rs\fixtures\global-cache
package_location: D:\a\pnp-rs\pnp-rs\fixtures\global-cache\../../../../../../C:/Users/runneradmin/AppData/Local/Yarn/Berry/cache/source-map-support-npm-0.5.21-09ca99e250-10c0.zip/node_modules/source-map-support/
normalized_location: C:Users/runneradmin/AppData/Local/Yarn/Berry/cache/source-map-support-npm-0.5.21-09ca99e250-10c0.zip/node_modules/source-map-support/
info.package_location: C:Users/runneradmin/AppData/Local/Yarn/Berry/cache/source-map-support-npm-0.5.21-09ca99e250-10c0.zip/node_modules/source-map-support/

Windows compatibility is bad...

cc @arcanis

@JounQin JounQin force-pushed the repro/global-cache branch 2 times, most recently from a3f6908 to d246536 Compare June 3, 2025 05:19
JounQin added a commit to unrs/unrs-resolver that referenced this pull request Jun 3, 2025
@JounQin
fix: pnp global cache should be supported (#129)
ac0f94a 
Windows global cache is still unsupported due to upstream

see also yarnpkg/pnp-rs#10
@JounQin JounQin mentioned this pull request Jun 3, 2025
Merged
@arcanis
Copy link
Member

arcanis commented Jun 3, 2025

Thanks! I'm looking into that.

This was referenced Jun 5, 2025
Closed
Merged
@JounQin
Copy link
Contributor Author

JounQin commented Jun 10, 2025

@arcanis Is there anything I can help to support Windows?

@arcanis
Copy link
Member

arcanis commented Jun 10, 2025 •
edited
Loading

My issue is mostly time at the moment - I'm working in parallel on a major Yarn refactoring, and coupled with regular work being intense I didn't get a chance to look at Windows yet.

I gladly accept contributions around it, although I don't want you to feel pressured into doing it - I still want to eventually address it myself if I'm not beat to the punch, although I don't have an exact timeline right now.

Design-wise, in Yarn we solved that by always treating all paths as posix, normalizing Windows path into posix internally (/c/foo) and only converting them back into Windows-style paths right as they were returned by the public api.

I wanted to try that but, while it worked fine for a full application, it feels a little overkill for a library... 🤔

@JounQin JounQin closed this Jul 7, 2025
@JounQin JounQin reopened this Jul 7, 2025
@JounQin JounQin force-pushed the repro/global-cache branch 2 times, most recently from 060219f to 4ac26c3 Compare July 7, 2025 14:28
@JounQin JounQin mentioned this pull request Jul 7, 2025
Open
mrginglymus
mrginglymus reviewed Jul 7, 2025
View reviewed changes
@JounQin JounQin mentioned this pull request Jul 11, 2025
Open
@JounQin JounQin mentioned this pull request Jul 27, 2025
Draft
@JounQin JounQin force-pushed the repro/global-cache branch 2 times, most recently from ec7e52d to c8187a9 Compare August 27, 2025 07:36
arcanis
arcanis reviewed Aug 27, 2025
View reviewed changes
.github/workflows/ci.yml
run: |
corepack enable
cd fixtures/global-cache
yarn install
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just check-in the .pnp.cjs file

Copy link
Contributor Author

@JounQin JounQin Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be different on different OS, so I'm not sure how to make it work.

@JounQin JounQin force-pushed the repro/global-cache branch from c8187a9 to ee2a58c Compare August 27, 2025 08:00
@JounQin JounQin force-pushed the repro/global-cache branch from ee2a58c to 92470fe Compare August 27, 2025 08:03
@arcanis
Copy link
Member

arcanis commented Aug 27, 2025

The test seems passing for me (on Windows); trying to see what's the difference with the CI.

@arcanis
Copy link
Member

arcanis commented Aug 27, 2025

I think it's because the cache and the project are on separate drives - when that happens the relative path contains the drive, and I suspect it confuses a path function somewhere. I have a repro, working now on a fix.

@JounQin JounQin force-pushed the repro/global-cache branch from 92470fe to 0db5418 Compare August 27, 2025 09:27
@JounQin
Copy link
Contributor Author

JounQin commented Aug 27, 2025 •
edited
Loading

Yes, it should be related to the clean-path crate, and I also tried path-clean, it doesn't work well neither.

So I opened danreeves/path-clean#16 and https://gitlab.com/foo-jin/clean-path/-/issues/1 for tracking.

@JounQin JounQin force-pushed the repro/global-cache branch from 0db5418 to 05c08a1 Compare August 27, 2025 09:48
@arcanis arcanis force-pushed the repro/global-cache branch from 05c08a1 to 9479357 Compare August 27, 2025 10:08
JounQin
JounQin commented Aug 27, 2025
View reviewed changes
JounQin
JounQin commented Aug 27, 2025
View reviewed changes
src/util.rs

if original_str.ends_with('/') && !str.ends_with('/') {
let components = str_minus_root.split(&['/', '\\'][..]);
Copy link
Contributor Author

@JounQin JounQin Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is another special driver format \\?\Volume{GUID}\ on Windows, for example \\server\share

https://github.com/oxc-project/oxc-resolver/blob/d069bb490f2e2708ce04f37b83a88c054e35a23e/src/tests/windows.rs#L10-L11

Would it work as expected at the same time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arcanis arcanis arcanis left review comments

+1 more reviewer

@mrginglymus mrginglymus mrginglymus left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JounQin @arcanis @mrginglymus