-
Notifications
You must be signed in to change notification settings - Fork 722
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
Copying fifos hangs. #727
base: master
Are you sure you want to change the base?
Copying fifos hangs. #727
Changes from 2 commits
e17d624
52c412f
e4a911d
eaaa40a
476d4b4
c71edc2
d469212
5689dc0
777fe04
8fa217b
18c513e
be93375
599a2c5
2d3af53
d942f3f
fad43aa
e6760bb
045c290
40518f7
c69ddc3
b2932cf
6a4a6a0
f7957bf
a4e5411
117894c
48a0198
0d70322
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -757,7 +757,7 @@ test('should not overwrite recently created files (not give error no-force mode) | |
t.is(shell.cat(`${t.context.tmp}/file1`).toString(), 'test1'); | ||
}); | ||
|
||
test('should copy fifos contents', t => { | ||
test('should copy fifo contents', t => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this case should also produce a warning. We'll completely drop fifo support and add it back in the plugin. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This fits in the "act like cp" sense. Being, without the From what I'd read, that's the context of where this came from in posix's cp; i.e. originally it would just treat everything as a file and do a dumb copy (creating a normal-file rather than a special-file), but then later on support was added for being clever with special-files. Hence, my brain goes "native shelljs can treat them like normal-files" and "plugin'd shelljs gains the knowledge to treat them as special-files" parallels "normal cp is dumb" and "modern cp -R is smart". If that helps frame why I'd left this in? There's a conflict between "act like cp" and "pull all the unix out" :-s There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree these goals are in conflict. I think in this case we should try to stay internally consistent. We should output a warning when copying any fifos and delegate to the plugin to provide actual posix behavior. If the plugin aims for posix compatibility, what arguments does it need passed? It sounds like it needs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, sounds right :-) (Assuming the srcFile symlink has already been resolved if (And I agree with your perspective on this: the whole reason I opened this PR in the first place was because it tripped me up, and staying internally consistent would have saved me whereas "acting like cp" wouldn't. Just hadn't quite come to that realisation yet :-p ) |
||
if (process.platform !== 'win32') { // fs.exists doesn't support fifos on windows | ||
shell.exec(`mkfifo ${t.context.tmp}/fifo`); | ||
shell.exec(`printf test1 > ${t.context.tmp}/fifo`, { async: true }); | ||
|
@@ -770,19 +770,19 @@ test('should copy fifos contents', t => { | |
} | ||
}); | ||
|
||
test('should create an equivalent fifo when copying fifos when set to recursive mode', t => { | ||
test('should warn about fifos in recursive mode', t => { | ||
if (process.platform !== 'win32') { // fs.exists doesn't support fifos on windows | ||
shell.exec(`mkfifo ${t.context.tmp}/fifo`); | ||
shell.exec(`printf test1 > ${t.context.tmp}/fifo`, { async: true }); | ||
t.truthy(fs.existsSync(`${t.context.tmp}/fifo`)); | ||
const result = shell.cp('-r', `${t.context.tmp}/fifo`, `${t.context.tmp}/newFifo`); | ||
t.falsy(shell.error()); | ||
t.is(result.code, 0); | ||
t.truthy(fs.statSync(`${t.context.tmp}/newFifo`).isFIFO()); | ||
t.truthy(shell.error()); | ||
t.is(result.code, 1); | ||
t.falsy(fs.existsSync(`${t.context.tmp}/newFifo`)); | ||
} | ||
}); | ||
|
||
test('should copy fifos contents via symlinks', t => { | ||
test('should copy fifo contents via symlinks', t => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For consistency, should this also be a warning? |
||
if (process.platform !== 'win32') { // fs.exists doesn't support fifos on windows | ||
shell.exec(`mkfifo ${t.context.tmp}/fifo`); | ||
shell.exec(`printf test1 > ${t.context.tmp}/fifo`, { async: true }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer to see There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Iirc, osx's echo doesn't support the Also worth noting named pipes aren't supported directly by Windows (only via cygwin etc, which didn't appear reliable) (Could match the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah :-) good point :-) Is it possible to easily shell out to shelljs's echo implementation? (I've shelled out here because the event loop is blocked by the cp, so need a way to get data into the fifo in the background; and atm this is shelling out to the /bin/sh impl (iirc?) and using it's echo/printf (although maybe there's a better way?)). |
||
|
@@ -798,7 +798,7 @@ test('should copy fifos contents via symlinks', t => { | |
} | ||
}); | ||
|
||
test('should create an equivalent fifo when copying fifos when set to recursive mode via symlinks', t => { | ||
test('should warn about fifos when set to recursive mode, via symlinks', t => { | ||
if (process.platform !== 'win32') { // fs.exists doesn't support fifos on windows | ||
shell.exec(`mkfifo ${t.context.tmp}/fifo`); | ||
shell.exec(`printf test1 > ${t.context.tmp}/fifo`, { async: true }); | ||
|
@@ -807,60 +807,30 @@ test('should create an equivalent fifo when copying fifos when set to recursive | |
shell.popd(); | ||
t.truthy(fs.existsSync(`${t.context.tmp}/symFifo`)); | ||
const result = shell.cp('-rL', `${t.context.tmp}/symFifo`, `${t.context.tmp}/newFifo`); | ||
t.falsy(shell.error()); | ||
t.is(result.code, 0); | ||
t.truthy(fs.statSync(`${t.context.tmp}/newFifo`).isFIFO()); | ||
} | ||
}); | ||
|
||
test('should copy character devices when set to recursive mode', t => { | ||
if (process.platform !== 'win32' && isRoot) { | ||
shell.exec(`mknod ${t.context.tmp}/zero c 1 5`); | ||
t.truthy(fs.existsSync(`${t.context.tmp}/zero`)); | ||
const result = shell.cp('-r', `${t.context.tmp}/zero`, `${t.context.tmp}/newZero`); | ||
t.falsy(shell.error()); | ||
t.is(result.code, 0); | ||
t.truthy(fs.statSync(`${t.context.tmp}/newZero`).isCharacterDevice()); | ||
t.truthy(shell.error()); | ||
t.is(result.code, 1); | ||
t.falsy(fs.existsSync(`${t.context.tmp}/newFifo`)); | ||
} | ||
}); | ||
|
||
test('should set the mode when copying character devices', t => { | ||
test('should warn about character devices when set to recursive mode', t => { | ||
if (process.platform !== 'win32' && isRoot) { | ||
shell.exec(`mknod ${t.context.tmp}/zero c 1 5`); | ||
t.truthy(fs.existsSync(`${t.context.tmp}/zero`)); | ||
const originalMode = fs.statSync(`${t.context.tmp}/zero`).mode; | ||
fs.chmodSync(`${t.context.tmp}/zero`, parseInt('777', 8)); | ||
t.not(fs.statSync(`${t.context.tmp}/zero`).mode, originalMode); | ||
const result = shell.cp('-r', `${t.context.tmp}/zero`, `${t.context.tmp}/newZero`); | ||
t.falsy(shell.error()); | ||
t.is(result.code, 0); | ||
t.truthy(fs.statSync(`${t.context.tmp}/newZero`).isCharacterDevice()); | ||
t.is(fs.statSync(`${t.context.tmp}/zero`).mode, fs.statSync(`${t.context.tmp}/newZero`).mode); | ||
} | ||
}); | ||
|
||
test('should copy block devices when set to recursive mode', t => { | ||
if (process.platform !== 'win32' && isRoot) { | ||
shell.exec(`mknod ${t.context.tmp}/sda1 b 8 1`); | ||
t.truthy(fs.existsSync(`${t.context.tmp}/sda1`)); | ||
const result = shell.cp('-r', `${t.context.tmp}/sda1`, `${t.context.tmp}/newSda1`); | ||
t.falsy(shell.error()); | ||
t.is(result.code, 0); | ||
t.truthy(fs.statSync(`${t.context.tmp}/newSda1`).isBlockDevice()); | ||
t.truthy(shell.error()); | ||
t.is(result.code, 1); | ||
t.falsy(fs.existsSync(`${t.context.tmp}/newZero`)); | ||
} | ||
}); | ||
|
||
test('should set the mode when copying block devices', t => { | ||
test('should warn about block devices when set to recursive mode', t => { | ||
if (process.platform !== 'win32' && isRoot) { | ||
shell.exec(`mknod ${t.context.tmp}/sda1 b 8 1`); | ||
t.truthy(fs.existsSync(`${t.context.tmp}/sda1`)); | ||
const originalMode = fs.statSync(`${t.context.tmp}/sda1`).mode; | ||
fs.chmodSync(`${t.context.tmp}/sda1`, parseInt('777', 8)); | ||
t.not(fs.statSync(`${t.context.tmp}/sda1`).mode, originalMode); | ||
const result = shell.cp('-r', `${t.context.tmp}/sda1`, `${t.context.tmp}/newSda1`); | ||
t.falsy(shell.error()); | ||
t.is(result.code, 0); | ||
t.truthy(fs.statSync(`${t.context.tmp}/newSda1`).isBlockDevice()); | ||
t.is(fs.statSync(`${t.context.tmp}/sda1`).mode, fs.statSync(`${t.context.tmp}/newSda1`).mode); | ||
t.truthy(shell.error()); | ||
t.is(result.code, 1); | ||
t.falsy(fs.existsSync(`${t.context.tmp}/newSda1`)); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the
common.error()
log message is sufficient. Let's get rid oflogLater
and move something like this into the docs. You can edit the//@
section in this file, and then runnpm run gendocs
.