jonschlinkert / copy

Copy files using glob patterns. Sync, async, promise or streams. (node.js utility)
MIT License
94 stars 125 forks source link

cwd is not calculating correctly #8

Closed binarykitchen closed 8 years ago

binarykitchen commented 8 years ago

I have absolute paths here, a single file and a target directory:

        copy.one(file, destinationDirectory, opts, function(err, file) {
            cb(err, file)
        })

file = /home/michael-heuberger/binarykitchen/code/videomail.io/var/local/tmp/clients/videomail.io/11e6-0555-3e17b940-b805-ef6a2fea069a/videomail_preview.webm

destinationDirectory = /home/michael-heuberger/binarykitchen/code/videomail.io/var/local/uploads/videomail.io/videomail/11e6/05/55/11e6-0555-3e17b940-b805-ef6a2fea069a

but it gets ugly, the video gets copied into

/var/local/uploads/videomail.io/videomail/11e6/05/55/11e6-0555-3e17b940-b805-ef6a2fea069a/var/local/tmp/clients/videomail.io/11e6-0555-3e17b940-b805-ef6a2fea069a/videomail_preview.webm

which is not what i want. probably the cwd is messing here. how can the cwd be ignored, and just copy from an absolute file to an absolute destination directory?

bid commented 8 years ago

The same happened to me. It seems quite counter-intuitive for a copy function to work this way, especially without mentioning that behavior in the readme.

It looks like you may get around this by using "cwd" in the options object, but since there's no documentation about the available options, I'll just switch to one of the countless alternative modules offering the same functionality.

binarykitchen commented 8 years ago

Exactly!

binarykitchen commented 8 years ago

On the other hand, we cannot blame the maintainers. It is a major sem-version change, so this is justified. Just the documentation could be better ...

jonschlinkert commented 8 years ago

sorry for the late reply. I had a funeral this week. this seems like a bug, I'll take a look

jonschlinkert commented 8 years ago

changed the title for clarity

jonschlinkert commented 8 years ago

Hmm, I've been trying to reproduce this locally but it seems to just work for me - with absolute paths, relative, with and without cwd.

were there any other options set or something else that might give me a hint about where the bug is happening? Or was it just src/dest paths?

The same happened to me. It seems quite counter-intuitive for a copy function to work this way, especially without mentioning that behavior in the readme.

I'm still trying to figure out which part is counter-intuitive. What behavior shouldn't be happening? Sorry if I'm being dense, I really want to fix this.

doowb commented 8 years ago

I'm able to reproduce this when process.cwd is outside of the path being copied:

var copy = require('copy');
var opts = {};

var src = '/Users/doowb/work/jonschlinkert/copy/test/fixtures/a.txt';
var dest = '/Users/doowb/work/jonschlinkert/copy/actual';
console.log(`Copying "${src}" => "${dest}"`);
copy.one(src, dest, opts, function(err) {
  if (err) throw err;
  console.log('done!');
});

When I run this from /Users/doowb/work/jonschlinkert/copy or /Users/doowb/work/jonschlinkert and it works fine.

When I run this from /Users/doowb/work/assemble/assemble-handlebars-helpers (arbitrary path for testing the issue) it writes the file to /Users/doowb/work/jonschlinkert/jonschlinkert/copy/test/fixtures/a.txt.

This is happening because when the src file is loaded using a vinyl object, the .relative property is calculated from the current process.cwd (in this case .relative ends up being ../../jonschlinkert/copy/test/fixtures/a.txt. Then the final destination is calculated using resolve from the destDir to the .relative property resulting in a destination relative to the actual destination.

Using var opts = { flatten: true }; works fine because the basename of the file is used instead of .relative.

I tried out some different methods of trying to figure out the relative path from the specified destination instead of the process.cwd but I haven't managed to get the correct results.

jonschlinkert commented 8 years ago

Awesome, thanks @doowb. that helps a lot

jonschlinkert commented 8 years ago

I believe this was fixed in commits I made a couple of weeks ago. closing, but feel free to reopen if the issue still occurs