Support Windows paths #33

Embraser01 · 2019-03-20T00:42:38Z

Following #29, I tried to implement fromPortablePath and toPortablePath in order to make it work on windows.

For now, its just a Draft PR, it's still not fully working but I'm making progress 🎉

I will continue working on it in the coming days

EDIT: I got it to work on windows (yarn install berry itself).

Still some things to look:

When yarn config -v, it displays posix paths
How do we handle paths in .yarnrc file?
Error with ZipFS
Build berry yarn build:cli on Windows

arcanis

It's a great start, thanks for leading this! I've left a few comments, feel free to address them whenever you like :)

packages/berry-core/sources/Cache.ts

packages/berry-fslib/sources/NodeFS.ts

packages/plugin-essentials/sources/commands/_entry.ts

arcanis · 2019-03-20T20:40:58Z

When yarn config -v, it displays posix paths

Interesting - I wonder whether that's a bug or a feature 🤔

How do we handle paths in .yarnrc file?

Good point - that would be in Configuration.ts, and more precisely here. Similar to how we had to call NodeFS.toPortablePath(args[0]) in the CLI _entry command because the argument was user-provided, in this case we likely need to call NodeFS.toPortablePath(value) in case the user specified an absolute path in its configuration.

Embraser01 · 2019-03-20T21:31:42Z

When yarn config -v, it displays posix paths

For now, It might be enough if we add an explaination in the doc? Its something transparent in most cases and handling it might be more tricky than useful (IMO)

How do we handle paths in .yarnrc file?

Ok, I'm changing this now

arcanis · 2019-03-20T21:51:39Z

For now, It might be enough if we add an explaination in the doc? Its something transparent in most cases and handling it might be more tricky than useful (IMO)

Yup, I concur - I've updated the wording in the help message (I plan to auto-generate the CLI documentation from the website based on the actual source code).

Embraser01 · 2019-03-20T23:54:27Z

There is still some problems, even if we should be able to do a yarn install without problems (except maybe isaacs/rimraf#72).

I'd like to make yarn build:ci work on Windows before merging this 😃

arcanis · 2019-03-21T17:36:17Z

Awesome! What are the current blockers that prevent build:cli from working?

Something that would be very useful too would be to add a Windows build to our Azure CI, this way we can start making sure that future PRs won't make us regress.

Embraser01 · 2019-03-22T09:00:13Z

I enabled the Windows pipeline in the config file.

Current blockers:

Handling each case where we use a third lib (e.g. globby) => It should be ok now
Got a command not found: run which is caused by

berry/packages/berry-cli/package.json

Lines 35 to 37 in 8a7b0cc

    
           "scripts": { 
        
             "build:cli": "run @berry-build-bundle", 
        
             "run:cli": "run @berry-run"

Embraser01 · 2019-03-22T12:02:03Z

UPDATE:

Now I have a problem with run.cmd file and related batch files.

The content of these files fail and I don't really know why...

berry/packages/berry-core/sources/scriptUtils.ts

Lines 15 to 22 in 4fa6508

    
           async function makePathWrapper(location: string, name: string, argv0: string, args: Array<string> = []) { 
        
             if (process.platform === `win32`) { 
        
               await xfs.writeFilePromise(`${location}/${name}.cmd`, `@"${location}\\${name}.cmd" ${args.join(` `)} %*\n`); 
        
             } else { 
        
               await xfs.writeFilePromise(`${location}/${name}`, `#!/usr/bin/env bash\n"${argv0}" ${args.map(arg => `'${arg.replace(/'/g, `'"'"'`)}'`).join(` `)} "$@"\n`); 
        
               await xfs.chmodPromise(`${location}/${name}`, 0o755); 
        
             } 
        
           }

arcanis · 2019-03-22T21:06:15Z

Current CI state:

Error: ENOENT: no such file or directory, open 'D:\a\1\s\.yarn\virtual\@babel-cli-virtual-d10f4f3334a43d2e0d02cbd0751c6eb087e6ca7cca5d5f05ea03e64051cef75c\node_modules\@babel\cli\package.json'

This is likely because .yarn\virtual\@babel-cli-virtual-[...] is a symlink to a zip archive. It's meant to be picked up as such by the zip layer (here) but it doesn't seem like it is, and it instead cascades to the native filesystem which throws a file not found exception.

To fix that we would need to add some logs in findZip to have a better sense of what it gets as input - maybe it contains backslashes which throw it off?

arcanis · 2019-03-22T21:23:25Z

realArchivePath { realArchivePath: '/mnt/d/a/1/s/.yarn/virtual/@babel-cli-virtual-d10f4f3334a43d2e0d02cbd0751c6eb087e6ca7cca5d5f05ea03e64051cef75c' }
stat { isFile: true, isDir: false, isSymlink: false }

Well, that's not great - seems like the symlink is dropped when cloning the repository 😞

arcanis · 2019-03-22T23:11:39Z

I fixed the symlink problem by enforcing core.symlinks and running git reset --hard as part of the checkout process. Not super great, but it works 😃

This problem looks weird - it's like the Node executable cannot be found, but the path looks legit:

D:\a\1\s\packages\berry-cli>"C:\hostedtoolcache\windows\node\10.15.3\x64\node.exe" D:\a\1\s\scripts\run-yarn.js run @berry-build-bundle 
Error: spawn C:\hostedtoolcache\windows\node\10.15.3\x64\node.exe ENOENT
    at Process.ChildProcess._handle.onexit (internal/child_process.js:240:19)
    at onErrorNT (internal/child_process.js:415:16)
    at process._tickCallback (internal/process/next_tick.js:63:19)

Embraser01 · 2019-03-23T06:55:56Z

Oh right forgot to mention it but I had to enable the core.symlinks to make it work...

So the problem seems to be at the second spawn, I was thinking about the @ in @berry-build-bundle that might be interpreted as something else by cmd.exe?

arcanis · 2019-03-23T15:39:37Z

Fixed! There was two problems:

On Windows the parameter to execa must be wrapped between double quotes if it contains spaces (like it does for Program Files). I made a quick patch but it kinda worries me a bit, that seems super weird ...
The cwd wasn't converted into the native format before spawning the process, hence the ENOENT even when the path didn't have actual spaces on Azure

The remaining issue seems to be related to the PnP hook, which isn't aware that it needs to transform back and forth the request and issuer parameter it receives from the user. That should be here, can I ask you to take a look when you have some time? I think the best would be to leave the functions as they are, but to expose wrappers here that would transparently make the conversion.

If you use @berry/fslib in this file, just double-check to make sure that the size of the .pnp.js doesn't go through the roof! I think it should be ok because it's already part of the bundle, but better be sure 😊

This reverts commit e616b6b.

Embraser01 · 2019-03-25T19:08:38Z

News:

I'm close to having a working build!

The native patch seems to work but I get this error when building:

D:\a\1\s\packages\berry-cli>"C:\hostedtoolcache\windows\node\8.10.0\x64\node.exe" D:\a\1\s\scripts\run-yarn.js run @berry-build-bundle 
The following plugins will be compiled in the final bundle:

- @berry/plugin-essentials
- @berry/plugin-init
- @berry/plugin-constraints
- @berry/plugin-file
- @berry/plugin-github
- @berry/plugin-http
- @berry/plugin-link
- @berry/plugin-npm
- @berry/plugin-pnp


ERROR in ./sources/cli.ts
Module build failed (from /mnt/d/a/1/s/.yarn/virtual/ts-loader-virtual-0bde21af32dc2f5de720bd65957c771fe15fbbc102355b0bfc2f837ac31d251f/node_modules/ts-loader/index.js):
TypeError: Cannot read property 'path' of undefined
    at /mnt/d/a/1/s/.yarn/cache/typescript-npm-3.2.4-b4e9592640fe6a3127f23c3a0ab3ee2d5a89b86447c83a3ca347efb5601d8e62.zip/node_modules/typescript/lib/typescript.js:57417:46
    at Map.forEach (<anonymous>)
    at createResolver (/mnt/d/a/1/s/.yarn/cache/typescript-npm-3.2.4-b4e9592640fe6a3127f23c3a0ab3ee2d5a89b86447c83a3ca347efb5601d8e62.zip/node_modules/typescript/lib/typescript.js:57412:49)
    at Object.createTypeChecker (/mnt/d/a/1/s/.yarn/cache/typescript-npm-3.2.4-b4e9592640fe6a3127f23c3a0ab3ee2d5a89b86447c83a3ca347efb5601d8e62.zip/node_modules/typescript/lib/typescript.js:30777:28)
    at Object.getTypeChecker (/mnt/d/a/1/s/.yarn/cache/typescript-npm-3.2.4-b4e9592640fe6a3127f23c3a0ab3ee2d5a89b86447c83a3ca347efb5601d8e62.zip/node_modules/typescript/lib/typescript.js:86884:79)
    at synchronizeHostData (/mnt/d/a/1/s/.yarn/cache/typescript-npm-3.2.4-b4e9592640fe6a3127f23c3a0ab3ee2d5a89b86447c83a3ca347efb5601d8e62.zip/node_modules/typescript/lib/typescript.js:117554:21)
    at Object.getProgram (/mnt/d/a/1/s/.yarn/cache/typescript-npm-3.2.4-b4e9592640fe6a3127f23c3a0ab3ee2d5a89b86447c83a3ca347efb5601d8e62.zip/node_modules/typescript/lib/typescript.js:117627:13)
    at successfulTypeScriptInstance (/mnt/d/a/1/s/.yarn/virtual/ts-loader-virtual-0bde21af32dc2f5de720bd65957c771fe15fbbc102355b0bfc2f837ac31d251f/node_modules/ts-loader/dist/instances.js:162:80)
    at Object.getTypeScriptInstance (/mnt/d/a/1/s/.yarn/virtual/ts-loader-virtual-0bde21af32dc2f5de720bd65957c771fe15fbbc102355b0bfc2f837ac31d251f/node_modules/ts-loader/dist/instances.js:34:12)
    at Object.loader (/mnt/d/a/1/s/.yarn/virtual/ts-loader-virtual-0bde21af32dc2f5de720bd65957c771fe15fbbc102355b0bfc2f837ac31d251f/node_modules/ts-loader/dist/index.js:17:41)
Error: spawn "C:\hostedtoolcache\windows\node\8.10.0\x64\node.exe" ENOENT
    at notFoundError (/mnt/d/a/1/s/packages/berry-cli/bin/berry.js:67753:26)
    at verifyENOENT (/mnt/d/a/1/s/packages/berry-cli/bin/berry.js:67787:16)
    at ChildProcess.cp.emit (/mnt/d/a/1/s/packages/berry-cli/bin/berry.js:67774:25)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:198:12)

Any idea why?

arcanis · 2019-03-25T19:23:21Z

Oh interesting - I think it's just because we should be calling fromPortablePath here (ts-pnp directly uses resolveToUnqualified: https://github.com/arcanis/ts-pnp/blob/master/index.js#L22) 😮

arcanis · 2019-03-25T19:26:05Z

I gave a quick look - it seems that you made it so the API always returns portable paths. I think this detail should be hidden from the API consumer, so the functions should both accept native paths and return them. Does it make sense?

Embraser01 · 2019-03-25T19:51:04Z

I gave a quick look - it seems that you made it so the API always returns portable paths. I think this detail should be hidden from the API consumer, so the functions should both accept native paths and return them. Does it make sense?

Yes, it makes sense... I used only portable path because PnP patch fs and so NodeJS doesn't understand all paths (like files inside Zip).

I can make the API transform back to physical path but it wouldn't work until we transform it back to a portable path.

Example:
pnpapi.resolveToUnqualified('@manaflair/concierge', './') would give

Portable path: /mnt/n/berry/.yarn/cache/@manaflair-concierge-npm-0.12.3-<checksum>.zip/node_modules/@manaflair/concierge
Physical path: N:\berry\.yarn\cache\@manaflair-concierge-npm-0.12.3-<checksum>.zip\node_modules\@manaflair\concierge

How do we deal with this behaviour?

arcanis · 2019-03-25T20:10:46Z

Indeed, I forgot about this! I think the solution would be for the PnP API not to use the actual Node filesystem in the first place - since we need the Zip layer anyway, it only makes sense to directly use the ZipOpenFS instance we've created. I've made a patch to try something in this sense, hopefully that'll help!

arcanis · 2019-03-25T23:42:50Z

We're almost there! The build now works!! 😃

Now there's about 21 tests that fail (out of 167, so that's pretty good!). After a quick look:

I think one problem is caused by the syml (@berry/parser) parser and/or stringifier when they face backslashes. They should be compatible with Yaml so I think they are meant to be doubled and surrounded by double quote, but maybe it's not the case.
yarn link registers paths using portable paths rather than physical paths. I think I'm fine with keeping this one as-is, but the tests should be fixed by using toPortablePath within the toMatchObject matcher.
The link:, portal:, and file: protocols don't properly join the paths together. Given that multiple tests use them to reproduce some tree layouts, fixing this should fix at least 10 tests at once.
The file: resolver doesn't support windows-style paths in its regexp.
I think the following mask check should be removed to just always ignore chmod on folders.

How do you wish to proceed? I was thinking maybe we can merge this PR to prevent further conflicts, and work on making the remaining tests pass on separate PRs, what do you think?

arcanis · 2019-03-25T23:46:47Z

Additionally, I'm about to order cool new Yarn tshirts to celebrate the v2. To thank you for your help I'd be happy to send you one once I get them. If that's of any interest to you, forward me by email a postal address where you wish to receive it and I'll let you know once I'll have shipped it 🙂

Embraser01 · 2019-03-26T10:49:01Z

We're almost there! The build now works!! 😃

Awesome! 🎉 🎉
Now I'll see how it behave on a middle sized project 😉

How do you wish to proceed? I was thinking maybe we can merge this PR to prevent further conflicts, and work on making the remaining tests pass on separate PRs, what do you think?

Works for me! I'll start working on it this week I think.

Additionally, I'm about to order cool new Yarn tshirts to celebrate the v2. To thank you for your help I'd be happy to send you one once I get them. If that's of any interest to you, forward me by email a postal address where you wish to receive it and I'll let you know once I'll have shipped it 🙂

Awesome, thanks! I'll email you my postal address soon 😃

arcanis reviewed Mar 20, 2019

View reviewed changes

Embraser01 marked this pull request as ready for review March 20, 2019 20:13

Embraser01 commented Mar 20, 2019

View reviewed changes

packages/plugin-essentials/sources/commands/_entry.ts Outdated Show resolved Hide resolved

Embraser01 and others added 15 commits March 24, 2019 18:22

Base work for portable path

cf63763

Add conversion where needed (still missing some!)

3ea7ac5

Got it to work on Windows!

ca12605

Convert to portable path when calling 'yarn <path> install'

9d9af0e

Update config.ts

e7f88b3

Use posix where possible

5afdbfe

Use xfs in json-proxy instead of native fs

9d8c602

Fix berry json proxy dependencies

9f7baa7

Handle more special cases (globby, pnp), activate Windows Pipeline

293d4c4

Fix PATH env variable when spawn process

08434df

Fix azure yaml

91da8f7

Fixes plugin loading

9e2c73a

Updates the checked-in build

a84f28a

Fixes yarn-path execution

fd90c88

Debug: Adds logging to findZip

15e5a14

Maël Nison and others added 9 commits March 24, 2019 18:24

Revert "Debug: Adds logging to findZip"

1006c08

This reverts commit e616b6b.

Enforces symlinks on Windows

06f4cad

Fixes the cmd scripts

935ef94

Adds a retry for EBUSY and ENOTEMPTY

47996f1

Fixes process spawning on win32

e5cc92a

More portable path, prevent translating to portable path if already one

bff06d3

Some cleanup for PnP hook on Windows

c03bdb4

Merge branch 'master' into windows-portable-path

73b0472

Use only portable path in Native resolution

3ea0c94

Moves the portable path conversion in dedicated wrappers

a25e7fd

Maël Nison added 2 commits March 25, 2019 20:09

Makes fakeFs a parameter to instantiate a PnP API

9b58424

Updates the PnP hook

7c49825

Maël Nison and others added 6 commits March 25, 2019 20:21

Fixes accidental infinite loop

4633d4f

Updates the checked-in PnP hook

b9d8553

Fixes how the PnP linker interacts with the PnP API on Windows

22edde3

Implements a fs layer to convert paths before they reach zipopenfs

2891c1a

Adds a few extra conversions

010898a

Updates the checked-in build

a9bf0fe

arcanis merged commit b6dbb68 into yarnpkg:master Mar 26, 2019

Embraser01 deleted the windows-portable-path branch March 26, 2019 12:24

arcanis mentioned this pull request Mar 26, 2019

Windows fixes #43

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Windows paths #33

Support Windows paths #33

Embraser01 commented Mar 20, 2019 •

edited

Loading

arcanis left a comment

arcanis commented Mar 20, 2019

Embraser01 commented Mar 20, 2019

arcanis commented Mar 20, 2019

Embraser01 commented Mar 20, 2019

arcanis commented Mar 21, 2019

Embraser01 commented Mar 22, 2019

Embraser01 commented Mar 22, 2019

arcanis commented Mar 22, 2019 •

edited

Loading

arcanis commented Mar 22, 2019

arcanis commented Mar 22, 2019

Embraser01 commented Mar 23, 2019

arcanis commented Mar 23, 2019

Embraser01 commented Mar 25, 2019

arcanis commented Mar 25, 2019 •

edited

Loading

arcanis commented Mar 25, 2019 •

edited

Loading

Embraser01 commented Mar 25, 2019

arcanis commented Mar 25, 2019

arcanis commented Mar 25, 2019

arcanis commented Mar 25, 2019 •

edited

Loading

Embraser01 commented Mar 26, 2019

Support Windows paths #33

Support Windows paths #33

Conversation

Embraser01 commented Mar 20, 2019 • edited Loading

arcanis left a comment

Choose a reason for hiding this comment

arcanis commented Mar 20, 2019

Embraser01 commented Mar 20, 2019

arcanis commented Mar 20, 2019

Embraser01 commented Mar 20, 2019

arcanis commented Mar 21, 2019

Embraser01 commented Mar 22, 2019

Embraser01 commented Mar 22, 2019

arcanis commented Mar 22, 2019 • edited Loading

arcanis commented Mar 22, 2019

arcanis commented Mar 22, 2019

Embraser01 commented Mar 23, 2019

arcanis commented Mar 23, 2019

Embraser01 commented Mar 25, 2019

arcanis commented Mar 25, 2019 • edited Loading

arcanis commented Mar 25, 2019 • edited Loading

Embraser01 commented Mar 25, 2019

arcanis commented Mar 25, 2019

arcanis commented Mar 25, 2019

arcanis commented Mar 25, 2019 • edited Loading

Embraser01 commented Mar 26, 2019

Embraser01 commented Mar 20, 2019 •

edited

Loading

arcanis commented Mar 22, 2019 •

edited

Loading

arcanis commented Mar 25, 2019 •

edited

Loading

arcanis commented Mar 25, 2019 •

edited

Loading

arcanis commented Mar 25, 2019 •

edited

Loading