Skip to content

lib: port rimraf to core as shutil.rmtree #28208

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.

Sign up for GitHub

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

Closed
iansu wants to merge 8 commits into from
Closed

lib: port rimraf to core as shutil.rmtree #28208

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fa504e0
lib: add shutil module with rmtree method
iansu Apr 28, 2019
b04be50
Add tests
iansu Apr 28, 2019
9c2436e
Move helper functions to internal module
iansu Apr 28, 2019
c765d7c
Remove assert
iansu Apr 28, 2019
d98fff1
Fix lint errors
iansu Jun 11, 2019
4c372b6
Only validate error argument if it's passed
iansu Jun 13, 2019
39706c6
Vendor rimraf as fs.rmtree
iansu Aug 14, 2019
1908b52
Remove last trace of shutil
iansu Aug 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix lint errors
  • Loading branch information
@iansu
iansu committed Jun 13, 2019
commit d98fff1d5d9f1ad16db72fef9e83a81b0bb4d37f
6 changes: 3 additions & 3 deletions lib/internal/modules/cjs/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ function stripBOM(content) {
const builtinLibs = [
'assert', 'async_hooks', 'buffer', 'child_process', 'cluster', 'crypto',
'dgram', 'dns', 'domain', 'events', 'fs', 'http', 'http2', 'https', 'net',
'os', 'path', 'perf_hooks', 'punycode', 'querystring', 'readline', 'repl', 'shutil',
'stream', 'string_decoder', 'tls', 'trace_events', 'tty', 'url', 'util',
'v8', 'vm', 'worker_threads', 'zlib'
'os', 'path', 'perf_hooks', 'punycode', 'querystring', 'readline', 'repl',
'shutil', 'stream', 'string_decoder', 'tls', 'trace_events', 'tty', 'url',
'util', 'v8', 'vm', 'worker_threads', 'zlib'
];

if (typeof internalBinding('inspector').open === 'function') {
Expand Down
60 changes: 36 additions & 24 deletions lib/internal/shutil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,16 @@
'use strict';
Copy link
Copy Markdown
Contributor

@sam-github sam-github Jun 13, 2019

Choose a reason for hiding this comment

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

ISC license and copyright is missing, isn't it?

It's ISC for a reason, so you of course already have permission to take the code, as long as the copyright and permission stick around. If it's easier for Node.js to relicense it as MIT, that's probably doable.

Copy link
Copy Markdown
Contributor Author

@iansu iansu Jun 13, 2019

Choose a reason for hiding this comment

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

I wasn't sure about the correct way to incorporate them so that's why they're missing. It's on the todo list though.

Copy link
Copy Markdown
Contributor

@silverwind silverwind Jun 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think it should be enough to add it to LICENSE like this, no need to litter files with licensing comments.

Copy link
Copy Markdown
Contributor

@MylesBorins MylesBorins Jun 17, 2019

Choose a reason for hiding this comment

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

if we are going to vendor rimraf could we do it similarly to how we've vendored other modules. Please look at the experimental fetch PR for an example of how to do this (including the license)

#27979


const fs = require('fs');
const { join } = require('path');
const { validatePath } = require('internal/fs/utils');
const { setTimeout } = require('timers');
const {
codes: {
ERR_INVALID_ARG_TYPE,
ERR_INVALID_CALLBACK
}
codes: { ERR_INVALID_ARG_TYPE, ERR_INVALID_CALLBACK }
} = require('internal/errors');

const isWindows = process.platform === 'win32';
const _0666 = parseInt('666', 8);

// for EMFILE handling
// For EMFILE handling
let timeout = 0;

function rmtree(path, options, callback) {
Expand All @@ -30,25 +29,27 @@ function rmtree(path, options, callback) {
rmtree_(path, options, function CB(er) {
if (er) {
if (
(er.code === 'EBUSY' || er.code === 'ENOTEMPTY' || er.code === 'EPERM') &&
(er.code === 'EBUSY' ||
er.code === 'ENOTEMPTY' ||
er.code === 'EPERM') &&
busyTries < options.maxBusyTries
) {
busyTries++;
let time = busyTries * 100;
// try again, with the same exact callback as this one.
const time = busyTries * 100;
// Try again, with the same exact callback as this one.
return setTimeout(function() {
rmtree_(path, options, CB);
}, time);
}

// this one won't happen if graceful-fs is used.
// This one won't happen if graceful-fs is used.
if (er.code === 'EMFILE' && timeout < options.emfileWait) {
return setTimeout(function() {
rmtree_(path, options, CB);
}, timeout++);
}

// already gone
// Already gone
if (er.code === 'ENOENT') er = null;

callback(er);
Expand All @@ -75,31 +76,33 @@ function rmtree_(path, options, callback) {
validateOptions(options);
validateCallback(callback);

// sunos lets the root user unlink directories, which is... weird.
// so we have to lstat here and make sure it's not a dir.
// Sunos lets the root user unlink directories, which is... weird.
// So we have to lstat here and make sure it's not a dir.
options.lstat(path, function(er, st) {
if (er && er.code === 'ENOENT') return callback(null);

// Windows can EPERM on stat. Life is suffering.
if (er && er.code === 'EPERM' && isWindows) fixWinEPERM(path, options, er, callback);
// Windows can EPERM on stat. Life is suffering.
if (er && er.code === 'EPERM' && isWindows) {
fixWinEPERM(path, options, er, callback);
}

if (st && st.isDirectory()) return rmdir(path, options, er, callback);

options.unlink(path, function(er) {
if (er) {
if (er.code === 'ENOENT') return callback(null);
if (er.code === 'EPERM')
return isWindows
? fixWinEPERM(path, options, er, callback)
: rmdir(path, options, er, callback);
return isWindows ?
fixWinEPERM(path, options, er, callback) :
rmdir(path, options, er, callback);
if (er.code === 'EISDIR') return rmdir(path, options, er, callback);
}
return callback(er);
});
});
}

// this looks simpler, and is strictly *faster*, but will
// This looks simpler, and is strictly *faster*, but will
// tie up the JavaScript thread and fail on excessively
// deep directory trees.
function rmtreeSync(path, options) {
Expand All @@ -118,13 +121,15 @@ function rmtreeSync(path, options) {
}

try {
// sunos lets the root user unlink directories, which is... weird.
// Sunos lets the root user unlink directories, which is... weird.
if (st && st.isDirectory()) rmdirSync(path, options, null);
else options.unlinkSync(path);
} catch (er) {
if (er.code === 'ENOENT') return;
if (er.code === 'EPERM')
return isWindows ? fixWinEPERMSync(path, options, er) : rmdirSync(path, options, er);
return isWindows ?
fixWinEPERMSync(path, options, er) :
rmdirSync(path, options, er);
if (er.code !== 'EISDIR') throw er;

rmdirSync(path, options, er);
Expand All @@ -146,7 +151,7 @@ function validateCallback(callback) {
}

function validateError(error) {
if (!error instanceof Error) {
if (!(error instanceof Error)) {
throw new ERR_INVALID_ARG_TYPE('error', 'Error', error);
}
}
Expand Down Expand Up @@ -214,11 +219,15 @@ function rmdir(path, options, originalEr, callback) {
validateError(originalEr);
validateCallback(callback);

// try to rmdir first, and only readdir on ENOTEMPTY or EEXIST (SunOS)
// Try to rmdir first, and only readdir on ENOTEMPTY or EEXIST (SunOS)
// if we guessed wrong, and it's not a directory, then
// raise the original error.
options.rmdir(path, function(er) {
if (er && (er.code === 'ENOTEMPTY' || er.code === 'EEXIST' || er.code === 'EPERM'))
if (er &&
(er.code === 'ENOTEMPTY' ||
er.code === 'EEXIST' ||
er.code === 'EPERM')
)
rmkids(path, options, callback);
else if (er && er.code === 'ENOTDIR') callback(originalEr);
else callback(er);
Expand Down Expand Up @@ -283,6 +292,9 @@ function rmkidsSync(path, options) {
threw = false;
return ret;
} finally {
// This is taken directly from rimraf. Fixing the lint error could
// subtly change the behavior
// eslint-disable-next-line
if (++i < retries && threw) continue;
}
} while (true);
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-shutil-rmtree.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ const path = require('path');
const assert = require('assert');
const shutil = require('shutil');

const tmpdir = require('../common/tmpdir');
const common = require('../common');
const tmpdir = common.tmpdir;

const tmpDir = tmpdir.path;
const baseDir = path.join(tmpDir, 'shutil-rmtree');
Expand Down