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: Use cmdshim instead of symlink on win32 in base-fetcher #6621

Merged
merged 6 commits into from Nov 5, 2018

Conversation

@yoadsn
Copy link
Contributor

commented Nov 1, 2018

Summary
This is an implementation of the suggestion by @arcanis to fix #6604. In short, on win32 platforms, the symlink requires elevated privileges. This fix would use the cmdshim to work around the need for the symlink specifically on the base-fetcher module.

Test plan
Same test suit was used since platform emulation is not included today.

@arcanis

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

Perfect, thanks!

@arcanis arcanis force-pushed the yoadsn:win32-symlink-shim-base-fetcher branch from 46a8cff to 52377dd Nov 1, 2018

@arcanis

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

I fixed the cache versioning, but it seems there are issues with two tests on Windows:

Summary of all failing tests
FAIL __tests__\commands\check.js
  ● --integrity should ignore comments and whitespaces in yarn.lock
    Error: ENOENT: no such file or directory, stat 'C:\Users\appveyor\AppData\Local\Temp\1\yarn-bundled-dep-check-1541114868149-0.6443591651832778\.yarn-cache\v4\npm-nyc-10.3.2-35563cb271637e2dc922a1145d8981e948a9f5c3\node_modules\nyc\bin\nyc.js' 
    Console output:
     [1/4] Resolving packages...
    [2/4] Fetching packages...
      185 |     }
      186 |   } catch (err) {
    > 187 |     throw new Error(`${err && err.stack} \nConsole output:\n ${out}`);
      188 |   } finally {
      189 |     reporter.close();
      190 |     await fs.unlink(cwd);
      
      
      at __tests__/commands/_helpers.js:187:11
          at Generator.throw (<anonymous>)
      at step (node_modules/babel-runtime/helpers/asyncToGenerator.js:17:30)
      at node_modules/babel-runtime/helpers/asyncToGenerator.js:30:13
  ● should ignore bundled dependencies
    Error: ENOENT: no such file or directory, stat 'C:\Users\appveyor\AppData\Local\Temp\1\yarn-bundled-dep-check-1541114868149-0.6443591651832778\.yarn-cache\v4\npm-nyc-10.3.2-35563cb271637e2dc922a1145d8981e948a9f5c3\node_modules\nyc\bin\nyc.js' 
    Console output:
     [1/4] Resolving packages...
    [2/4] Fetching packages...
      185 |     }
      186 |   } catch (err) {
    > 187 |     throw new Error(`${err && err.stack} \nConsole output:\n ${out}`);
      188 |   } finally {
      189 |     reporter.close();
      190 |     await fs.unlink(cwd);
      
      
      at __tests__/commands/_helpers.js:187:11
          at Generator.throw (<anonymous>)
      at step (node_modules/babel-runtime/helpers/asyncToGenerator.js:17:30)
      at node_modules/babel-runtime/helpers/asyncToGenerator.js:30:13
Test Suites: 1 failed, 87 passed, 88 total
Tests:       2 failed, 3 skipped, 1195 passed, 1200 total
@yoadsn

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2018

Able to repro on a windows machine.
@arcanis Obviously, the check command is not happy about the shim usage. I'm not proficient enough
with the code and the check logic to determine if it's the way check is implemented or the checks should be different on win32. Maybe its the test suit itself who should have different expectations on win32.
Regardless, since the shim does not do what a symlink does - there might be other places where the tests do not cover well and yarn would still fail due to the missing real symlinks.
What would you suggest? Any other approach or should we keep going this path? Getting elevated privileges on Azure sandbox seems out of the question.

@arcanis

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

Getting elevated privileges on Azure sandbox seems out of the question.

Yep, totally!

What would you suggest? Any other approach or should we keep going this path?

I'll try to repro it when I get to a Windows machine (this evening, hopefully). I'm not sure what yarn check does that is so special (we're actually considering removing it Yarn 2).

@arcanis

This comment has been minimized.

Copy link
Member

commented Nov 3, 2018

Ok, I think I've found the problem. It's just that the test fixture in those tests is only partial, meaning that the destination of the binary link doesn't actually exist. The new code that creates it expects it to do (probably because of lockMutex, I'd say), and thus crashes. I'm not sure why it crashes during the fetch step (I'd have expected it to happen during the linking), though.

Will fix that next week.

@yoadsn

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2018

So again, since I read this code-base for the first time - In the failing fixture's runInstall, which creates an Install and calls init() on it would cause a bunch of steps to be queued and executed. The first queued step is the resolve, the second queued step would be a fetch which would in turn call the fetch() on the fetcher. This package-fetcher would use the patched code in base-fetcher which expects the target bin file to exist but is not since the fixture does not run something which usually would have been. Strange.
The linker should be the one calling linkBin which would place the file where it should be. Since fetch runs before that (from all I can figure out) - What would you expect to run before that to make the test complete? Did I understand correctly?

Is it safe to assume the original code did not fail since symlink does not check/depend on the existence of the src (first arg, usually called target though) but the patched code relies on it to exist?
In that case - My thought it that the fact that cmdShim does expect the file to exist is the culprit and in fact, if the package's bin has not bin linked before, this is a valid scenario (src does not exist).
When patching cmdShim to use cmdShim.ifExists the tests seem to pass. So it is semantically close in that regard to the original symlink.

WDYT?

@buildsize

This comment has been minimized.

Copy link

commented Nov 3, 2018

File name Previous Size New Size Change
yarn-[version].noarch.rpm 1.09 MB 1.09 MB 319 bytes (0%)
yarn-[version].js 4.27 MB 4.27 MB 454 bytes (0%)
yarn-legacy-[version].js 4.44 MB 4.45 MB 450 bytes (0%)
yarn-v[version].tar.gz 1.1 MB 1.1 MB 1.63 KB (0%)
yarn_[version]all.deb 805.17 KB 805.04 KB -136 bytes (0%)

@arcanis arcanis merged commit 4e3b2f6 into yarnpkg:master Nov 5, 2018

1 of 13 checks passed

ci/circleci: build CircleCI is running your tests
Details
ci/circleci: lint CircleCI is running your tests
Details
ci/circleci: test-linux-node10 Your tests are queued behind your running builds
Details
ci/circleci: test-linux-node4 CircleCI is running your tests
Details
ci/circleci: test-linux-node6 CircleCI is running your tests
Details
ci/circleci: test-linux-node8 CircleCI is running your tests
Details
ci/circleci: test-macos-node10 Your tests are queued behind your running builds
Details
ci/circleci: test-macos-node6 Your tests are queued behind your running builds
Details
ci/circleci: test-macos-node8 Your tests are queued behind your running builds
Details
ci/circleci: test-pkg-tests-linux-node10 Your tests are queued behind your running builds
Details
ci/circleci: test-pkg-tests-linux-node8 Your tests are queued behind your running builds
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
ci/circleci: install Your tests passed on CircleCI!
Details

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 22, 2018

yarn: Update to 1.12.3.
Changelog tracks back up to 1.12.0 only.

## 1.12.3

**Important:** This release contains a cache bump. It will cause the very first install following the upgrade to take slightly more time, especially if you don't use the [Offline Mirror](https://yarnpkg.com/blog/2016/11/24/offline-mirror/) feature. After that everything will be back to normal.

- Fixes an issue with `yarn audit` when using workspaces

  [6625](yarnpkg/yarn#6639) - [**Jeff Valore**](https://twitter.com/codingwithspike)

- Uses `NODE_OPTIONS` to instruct Node to load the PnP hook, instead of raw CLI arguments

  **Caveat:** This change might cause issues for PnP users having a space inside their cwd (cf [nodejs/node24065](nodejs/node#24065))

  [6479](yarnpkg/yarn#6629) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes Gulp when used with Plug'n'Play

  [6623](yarnpkg/yarn#6623) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes an issue with `yarn audit` when the root package was missing a name

  [6611](yarnpkg/yarn#6611) - [**Jack Zhao**](https://github.com/bugzpodder)

- Fixes an issue with `yarn audit` when a package was depending on an empty range

  [6611](yarnpkg/yarn#6611) - [**Jack Zhao**](https://github.com/bugzpodder)

- Fixes an issue with how symlinks are setup into the cache on Windows

  [6621](yarnpkg/yarn#6621) - [**Yoad Snapir**](https://github.com/yoadsn)

- Upgrades `inquirer`, fixing `upgrade-interactive` for users using both Node 10 and Windows

  [6635](yarnpkg/yarn#6635) - [**Philipp Feigl**](https://github.com/pfeigl)

- Exposes the path to the PnP file using `require.resolve('pnpapi')`

  [6643](yarnpkg/yarn#6643) - [**Maël Nison**](https://twitter.com/arcanis)

## 1.12.2

This release doesn't actually exists and was caused by a quirk in our systems.

## 1.12.1

- Ensures the engine check is ran before showing the UI for `upgrade-interactive`

  [6536](yarnpkg/yarn#6536) - [**Orta Therox**](https://github.com/orta)

- Restores Node v4 support by downgrading `cli-table3`

  [6535](yarnpkg/yarn#6535) - [**Mark Stacey**](https://github.com/Gudahtt)

- Prevents infinite loop when parsing corrupted lockfiles with unterminated strings

  [4965](yarnpkg/yarn#4965) - [**Ryan Hendrickson**](https://github.com/rhendric)

- Environment variables now have to **start** with `YARN_` (instead of just contain it) to be considered

  [6518](yarnpkg/yarn#6518) - [**Michael Gmelin**](https://blog.grem.de)

- Fixes the `extensions` option when used by `resolveRequest`

  [6479](yarnpkg/yarn#6479) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes handling of empty string entries for `bin` in package.json

  [6515](yarnpkg/yarn#6515) - [**Ryan Burrows**](https://github.com/rhburrows)

- Adds support for basic auth for registries with paths, such as artifactory

  [5322](yarnpkg/yarn#5322) - [**Karolis Narkevicius**](https://twitter.com/KidkArolis)

- Adds 2FA (Two Factor Authentication) support to publish & alike

  [6555](yarnpkg/yarn#6555) - [**Krzysztof Zbudniewek**](https://github.com/neonowy)

- Fixes how the `files` property is interpreted to bring it in line with npm

  [6562](yarnpkg/yarn#6562) - [**Bertrand Marron**](https://github.com/tusbar)

- Fixes Yarn invocations on Darwin when the `yarn` binary was symlinked

  [6568](yarnpkg/yarn#6568) - [**Hidde Boomsma**](https://github.com/hboomsma)

- Fixes `require.resolve` when used together with the `paths` option

  [6565](yarnpkg/yarn#6565) - [**Maël Nison**](https://twitter.com/arcanis)

## 1.12.0

- Adds initial support for PnP on Windows

  [6447](yarnpkg/yarn#6447) - [**John-David Dalton**](https://twitter.com/jdalton)

- Adds `yarn audit` (and the `--audit` flag for all installs)

  [6409](yarnpkg/yarn#6409) - [**Jeff Valore**](https://github.com/rally25rs)

- Adds a special logic to PnP for ESLint compatibility (temporary, until [eslint/eslint10125](eslint/eslint#10125) is fixed)

  [6449](yarnpkg/yarn#6449) - [**Maël Nison**](https://twitter.com/arcanis)

- Makes the PnP hook inject a `process.versions.pnp` variable when setup (equals to `VERSIONS.std`)

  [6464](yarnpkg/yarn#6464) - [**Maël Nison**](https://twitter.com/arcanis)

- Disables by default (configurable) the automatic migration of the `integrity` field. **It will be re-enabled in 2.0.**

  [6465](yarnpkg/yarn#6465) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes the display name of the faulty package when the NPM registry returns corrupted data

  [6455](yarnpkg/yarn#6455) - [**Grey Baker**](https://github.com/greysteil)

- Prevents crashes when running `yarn outdated` and the NPM registry forgets to return the `latest` tag

  [6454](yarnpkg/yarn#6454) - [**mad-mike**](https://github.com/mad-mike)

- Fixes `yarn run` when used together with workspaces and PnP

  [6444](yarnpkg/yarn#6444) - [**Maël Nison**](https://twitter.com/arcanis)

- Fixes an edge case when peer dependencies were resolved multiple levels deep (`webpack-dev-server`)

  [6443](yarnpkg/yarn#6443) - [**Maël Nison**](https://twitter.com/arcanis)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.