pnpm / supi

Fast, disk space efficient installation engine. Used by pnpm
MIT License
24 stars 5 forks source link

feat: add packageImportMethod option #32

Closed etamponi closed 6 years ago

etamponi commented 6 years ago

I didn't want to spend too much time figuring out the right signature for the execFilePromise function, but everything else should be there. As for the tests, in this case, I would have some troubles as reflinks only work on some kind of filesystems.

I can mock child_process if you want, using mockery or similar tools.

zkochan commented 6 years ago

Related: https://github.com/pnpm/pnpm/issues/771

etamponi commented 6 years ago

I tried to make as few changes as possible, but if the switch statement is too "clever", let me know, I'll change it.

zkochan commented 6 years ago

A switch is a good choice in this case but I would suggest declaring a separate function for creating the reflinks - reflinkPkg. And you can pass reflinkPkg to linkAllPkgs(). Same way as linkPkg and copyPkg are passed there. You can rename linkPkg to hardlinkPkg

zkochan commented 6 years ago

If you don't know how to cover reflink with a test, skip it for now, but let's create an issue for it.

For copying you can create a test, I believe

And please document the new option in the README

etamponi commented 6 years ago

I'll write the tests either later today or next year ;)

etamponi commented 6 years ago

Fun: the copy test is failing. Has the copy path been tested before? This is the error:

$ node_modules/.bin/ts-node test/packageImportMethods.ts                                                                                                                   ⬡ 6.12.2 [±master-upstream ●●]
TAP version 13
# packageImportMethod can be set to copy
ok 1 create testing package 1
not ok 2 Error: ENOENT: no such file or directory, mkdir '/Users/etamponi/code/.tmp/1/project/node_modules/.localhost+4873/is-negative/2.1.0/node_modules/is-negative'
  ---
    operator: error
    expected: |-
      undefined
    actual: |-
      [ { [Error: ENOENT: no such file or directory, mkdir '/Users/etamponi/code/.tmp/1/project/node_modules/.localhost+4873/is-negative/2.1.0/node_modules/is-negative'] errno: -2, code: 'ENOENT', syscall: 'mkdir', path: '/Users/etamponi/code/.tmp/1/project/node_modules/.localhost+4873/is-negative/2.1.0/node_modules/is-negative' } ]
    at: bound (/Users/etamponi/code/supi/node_modules/.registry.npmjs.org/onetime/2.0.1/node_modules/onetime/index.js:30:12)
    stack: |-
      Error: Error: ENOENT: no such file or directory, mkdir '/Users/etamponi/code/.tmp/1/project/node_modules/.localhost+4873/is-negative/2.1.0/node_modules/is-negative'
          at Test.assert [as _assert] (/Users/etamponi/code/supi/node_modules/.registry.npmjs.org/tape/4.8.0/node_modules/tape/lib/test.js:212:54)
          at Test.bound [as _assert] (/Users/etamponi/code/supi/node_modules/.registry.npmjs.org/tape/4.8.0/node_modules/tape/lib/test.js:64:32)
          at Test.error.Test.ifError.Test.ifErr.Test.iferror (/Users/etamponi/code/supi/node_modules/.registry.npmjs.org/tape/4.8.0/node_modules/tape/lib/test.js:332:10)
          at Test.bound [as ifError] (/Users/etamponi/code/supi/node_modules/.registry.npmjs.org/tape/4.8.0/node_modules/tape/lib/test.js:64:32)
          at Test.end (/Users/etamponi/code/supi/node_modules/.registry.npmjs.org/tape/4.8.0/node_modules/tape/lib/test.js:137:14)
          at bound (/Users/etamponi/code/supi/node_modules/.registry.npmjs.org/tape/4.8.0/node_modules/tape/lib/test.js:64:32)
          at bound (/Users/etamponi/code/supi/node_modules/.registry.npmjs.org/onetime/2.0.1/node_modules/onetime/index.js:30:12)
  ...
zkochan commented 6 years ago

Maybe it is this issue? https://github.com/pnpm/pnpm/issues/866

etamponi commented 6 years ago

I don't know, but an await mkdirp(dependency.hardlinkedLocation) before the call to ncp fixed it :)

zkochan commented 6 years ago

wow, if it will fix that issue, you are the hero of the day!

etamponi commented 6 years ago

Here you go! I've added the copy test. I'd love to at least unit-test the reflink method using a mock of child_process, but if you're ok with it, we can do it later. I'll open a new issue

zkochan commented 6 years ago

I am ok with later. I would prefer the real thing. Mocks are not that reliable

etamponi commented 6 years ago

The problem with the real thing is that reflinks only work on btrfs and ocfs2...