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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows fixes #43

Merged
merged 7 commits into from Mar 27, 2019
Merged

Windows fixes #43

merged 7 commits into from Mar 27, 2019

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Mar 26, 2019

This PR should fix most of the remaining issues identified in #33 馃

cc @Embraser01

@arcanis
Copy link
Member Author

arcanis commented Mar 27, 2019

Only 11 remaining 馃挭

@arcanis
Copy link
Member Author

arcanis commented Mar 27, 2019

Only 4 remaining - I'll go ahead and merge those fixes.

The 4 remaining failing tests are caused by two problems:

  • When used, the --cwd doesn't go through toPortablePath. This one will be annoying, because I'd strongly prefer not to have to do this treatment in each and every command, but there is no obvious place where to abstract it at the moment. I might have to make a change to Concierge, the CLI framework.

  • The cwd sent to execFileSync doesn't go to fromPortablePath so Windows report a ENOENT.

@arcanis arcanis merged commit 9674fac into master Mar 27, 2019
if (opts.project.configuration.get(`enableAbsoluteVirtuals`)) {
target = to;
} else {
throw new ReportError(MessageName.CROSS_DRIVE_VIRTUAL_LOCAL, `The virtual folder (${fromParse.root}) must be on the same drive as the local package it references (${toParse.root})`);
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to report an error with the paths not just the drive name?

Copy link
Member Author

Choose a reason for hiding this comment

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

The paths can be rather long, so I was worried they would impact the legibility of the error message (I try to avoid soft-wrapping as much as possible).

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see...
It's not important anyway 馃榾. I was thinking it might be confusing if the user just see 2 different drives letters without any folder names

Is it possible to make the error message go on multiple lines?
Or maybe show the path to the virtual folder and only the drive of the project?

Side question 馃槄: What was the reason for requiring virtual folder to be on the same drive?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it possible to make the error message go on multiple lines?
Or maybe show the path to the virtual folder and only the drive of the project?

We could make them multi-line, but then it becomes harder for the brain to parse how many warnings or errors a project have, and we could be overly verbose while not covering enough anyway (it might be fine for hard errors, but annoying for warnings).

The message codes are the best way to expose detailed information to the user, since you get to give them the error, explain why it's legit, and explain the alternative. I've documented the first few here, and I plan to do the same for each error type on the new website:

https://github.com/yarnpkg/berry/blob/master/docs-legacy/error-messages.md

Side question 馃槄: What was the reason for requiring virtual folder to be on the same drive?

Good question, that's typically the kind of thing I'd put in the per-code documentation, for example! 馃槃

In order to properly resolve peer dependencies while also making require.resolve work, we need each individual package instance to have a different path on the disk. Since all packages share the same cache, we need to create one symlink for each instance of a package that lists peer dependencies. All those symlinks point towards the same location for a given package (usually the cache, but in the case of the link: and portal: protocols it's the actual target on disk).

The problem is that in order to keep those symlinks portable across systems, we must use relative paths (so that if you clone your repository in a separate directory then the symlinks are still correct). On posix it's easy because all drives are mounted on a single file tree, but on Windows it's impossible because they're on different file tree - no relative path can switch from one drive to another. For this reason, the virtual folder must be located on the same drive as the cache (which is often the case), but also the project if you happen to use the link: protocol. I got this problem because my clone is in D:\berry-windows, and the tests run within temporary directories on C:\.

I've added the enableAbsoluteVirtuals settings in this PR which allows to use absolute symlinks in this case, but that's not recommended as it breaks portability (across Windows and posix, of course, but also different Windows computers if the folders aren't positioned exactly like listed in the symlinks).

Honestly the virtual thingy is a bit impractical, I've started to push for Node to adopt a better API (the whole problem is that the return value of require.resolve must work with both the fs functions AND require - meaning that if we need to store metadata for require to find the right package, then those metadata have to actually exist on the disk).

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for explaining 馃槂 It's a lot clearer for me!

I still don't understand a few things:

  • What are the benefits of setting the Cache or Virtuals folder outside the project repo?
  • Doesn't it break portability as soon as we set cache or virtuals outside the repository?

Copy link
Member Author

Choose a reason for hiding this comment

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

What are the benefits of setting the Cache or Virtuals folder outside the project repo?

Storing the cache on a per-system basis rather than per-project means that all your projects can benefit from the same cache (so if you run yarn add foo in project A, if project B already used foo, it'll be very fast since we won't need to download them again).

Storing the virtual folder in a different folder than the default one isn't recommended, though - there is very little gain, and can even be harmful (more risks of conflicting hashes).

Doesn't it break portability as soon as we set cache or virtuals outside the repository?

Not entirely, because we use relative links. So if the user takes the responsibility of always configuring their cache the exact same way relative to the project, and if it guarantees that the cache always has all the data required, it will work (for example you could imagine a service like CircleCI mounting a mega-cache-folder containing all the packages from the npm registry).

It's more complex to setup though, hence the recommendation to just store the cache within the repository (or to just keep running yarn install and disregard zero-installs altogether, for small libraries).

@arcanis arcanis deleted the windows-fixes branch May 7, 2019 13:53
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.

None yet

2 participants