jonschlinkert / copy

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

Cannot read file - illegal operation #24

Closed jnthnjns closed 7 years ago

jnthnjns commented 7 years ago

Edit: Windows 10 npm 5.2.0

I can't upgrade npm right now because they have a bug in the most recent version that breaks another npm package for me, but there is a fix committed and will soon be included in a future release.


I am getting an unexpected behavior with the callback. Everything seems to work perfectly (the way I have it) but I am getting an error.

copy(["./src/**/*.html", "./src/fonts/**/*"], './dist/', function (err, file) {
    if (err) console.error(err);
});

Console output:

{ Error: [copy base] cannot read file > "C:\path\to\site\src\fonts\material-icons": EISDIR: illegal operation on a directory, read at Error (native) errno: -4068, code: 'EISDIR', syscall: 'read' }

I have compared the two directories ./src/ and ./dist/ and it looks like everything worked great. I just can't move forward because I am handling the error as thought it failed.


Before I run copy I completely wipe ./dist/ with rimraf so I know the copy is happening. Here's the full code without the long winded error handling:

var globby = require('globby');
var rimraf = require('rimraf');
var mkdirp = require('mkdirp');
var copy = require('copy');
var shell = require('shelljs');

globby(["./dist/*"]).then(function (files) {

    // Clean
    files.map(function (file) {
        rimraf.sync(file);
    });

    // Create any necessary directories
    mkdirp('./dist/css/', function (err) {
        if (err) console.error(err);
        // handle errors, kill process
    });

    copy(["./src/**/*.html", "./src/fonts/**/*"], './dist/', function (err, file) {
        if (err) console.error(err);
        // handle errors, kill process
    });

    // compile sass
    shell.exec("sass ./src/css/main.scss:./dist/css/main.css --style compressed");

    // compile typescript
    shell.exec("tsc");
});
doowb commented 7 years ago

EISDIR: illegal operation on a directory

This indicates that copy is trying to read the directory path as a file. I'll try to take a look at this later, but I'm not sure when I'll get to it.

I haven't dug into the code, but if you get a chance, I would look to see if there is any checking happening to see if the file path is a directory or not.

jnthnjns commented 7 years ago

@doowb

I did have a few minutes to spare this morning so I cloned the repo and setup my own test

'use strict';

require('mocha');
var path = require('path');
var support = require('./support');
var assert = require('assert');
require('assert-path')(assert);
require('assert-fs')(assert);
var exists = support.exists;
var copy = require('..');
var rimraf = require('rimraf');

describe('jnthnjns', function() {
  it('should not die', function(cb) {
    var src = ["./test/src/**/*.html", "./test/src/fonts/**/*"];
    var dest = './test/dist';
    rimraf.sync('./test/dist/*');

    copy(src, dest, function(err) {
      if (err) return cb(err);
      exists(dest, cb);
    });
  });
});

In index.js, adding the following file check fixes the issue for me and all tests still pass:

// using node fs because I am unfamiliar with graceful-fs and this was a quick solution
var nodeFS = require('fs');
toDest(dir, file, opts, function(err, out) {
  if (err) {
    cb(err);
    return;
  }

  // added file check, this `base()` method dies in `createReadStream()` on directories
  if (nodeFS.lstatSync(file).isFile()) {
    base(file, out.path, opts, function(err) {
      if (err) {
        cb(err);
        return;
      }

      cb(null, out);
    });
  } else {
    // Not sure what you'd like the file param to be in the cb here
    cb(null, null);
  }
});

I haven't submitted a PR because I'm not sure if this is the best resolution so I figured I would leave it up to you guys and I figured more testing needed to be done.

My test results:

copy.base
    √ should copy a file (79ms)
    √ should error when dest is missing
    √ should throw an error when callback is not a function
    √ should error when src is missing

  copy.each
    √ should copy an array of files to a directory (117ms)
    √ should copy an array of files from a cwd
    √ should copy an array of files to a destBase (40ms)
    √ should copy an array of files using cwd and destBase
    √ should flatten the basename of src paths to a destBase
    √ should copy an array of files from a cwd to a destBase
    √ should error when dest is missing
    √ should throw an error when callback is not a function
    √ should error when src is missing

  copy
    √ should copy a file to a directory (43ms)
    √ should use cwd for src and dest
    √ should flatten the filepath and join src basename to dest path
    √ should copy a file to a file
    √ should copy a glob of files to a directory (54ms)
    √ should copy an array of files to a directory
    √ should copy an array of files from a cwd
    √ should copy an array of files to a destBase
    √ should copy an array of files using cwd and destBase
    √ should flatten the basename of src paths to a destBase (41ms)
    √ should copy an array of files from a cwd to a destBase (147ms)
    √ should error when dest is missing
    √ should throw an error when callback is not a function
    √ should error when src is missing

  copy.one
    √ should copy a file from an absolute path to an absolute path
    √ should copy a file (51ms)
    √ should error when dest is missing
    √ should throw an error when callback is not a function
    √ should error when src is missing

  dest
    √ should join dest directory to src filepath
    √ should strip glob parent from the src path
    √ should strip srcBase from the src path
    √ should flatten src basename onto dest directory
    √ should replace dest extension with options.ext
    √ should strip dest extension when options.ext is an empty string

  jnthnjns
    √ should not die (130ms)

  once
    √ should only call a function once

  recurse.async
    √ should return an array of files (46ms)
    √ should create file objects
    √ should create file objects

  recurse.promise
    √ should return an array of file objects (103ms)
    √ should create file objects
    √ should read contents from files as a buffer

  recurse.sync
    √ should return an array of files
    √ should create file objects
    √ should read contents from files as a buffer

  walk
    √ should return an array of file objects

  50 passing (2s)
doowb commented 7 years ago

@jnthnjns thanks for digging into this some more.

I am unfamiliar with graceful-fs

It's basically a wrapper around fs, but I think it's fine to use fs here if graceful-fs doesn't support those stat methods.

// Not sure what you'd like the file param to be in the cb here

I think that returning an error from copy.one would make sense, then in copy where the globbing is done, handle removing directories from the list of files sent to copy.each (which calls copy.one).

I'll double check on how the recursion is done and how this should work.

doowb commented 7 years ago

@jnthnjns I published version 0.3.1 to npm with a fix for this bug.

While doing the test, I noticed that you might need to set the srcBase option depending on how you want the files to end up in dist. I would try the following if you have any issues:

copy(['src/**/*.html', 'src/fonts/**/*'], 'dist', {srcBase: 'src'}, function (err, files) {
  if (err) {
    console.error(err);
    return;
  }
  // copied files
  console.log(files);
});
jnthnjns commented 7 years ago

@doowb Works great thank you for quick turn around. I really appreciate the help.