Support '/dev/stdout' as a valid '--out' parameter

[MartyIX]

Proposed fix for issue #4841.

I have updated writeFile function for Node system. Essentially, I've ended with the code that @mklement0 suggested in his comment. The documentation states that:

process.stderr and process.stdout are unlike other streams in Node.js in that they cannot be closed (end() will throw), they never emit the finish event and that writes are always blocking [added emphasis].

The new behavior can be tested this way:

node built/local/tsc.js --out /dev/stdout hello.ts
node built/local/tsc.js --out - hello.ts

Can you comment on a proper way of testing this feature?

Thanks!

---

^{I don't know how WScript system works and if the issue is present there too or not.}

[mhegazy]

why is fs.writeFileSync not sufficient here? what can we change to make it work for all cases without the special handling?

[mklement0]

@mhegazy: Not using fs.writeFileSync is at least currently required for supporting output to stdout. With fs.writeFileSync, if you target /dev/stdout, as previously stated, you get error 'ESPIPE, invalid seek' on Linux and 'ENXIO, no such device or address' on OS X.

what can we change to make it work for all cases without the special handling?

Talk to the Node.js team to investigate why writing to /dev/stdout with fs.writeFileSync doesn't work and/or convince them to change / fix it.

Conversely: What problems do you see with this proposed approach to special-casing stdout output?

After all, note that @MartyIX's quote from the Node.js documentation implies that stdout and stderr are special:

process.stderr and process.stdout are unlike other streams in Node.js [...]

[MartyIX]

@mhegazy @mklement0 I tried to analyze why node returns ESPIPE, invalid seek so I cloned the node repo from Github, compiled it and run:

$ echo "require('fs').writeFileSync('/dev/stdout', 'hello');" | ./node 

fs.js:707
    return binding.writeBuffer(fd, buffer, offset, length, position);
                   ^

Error: ESPIPE: invalid seek, write
    at Error (native)
    at Object.fs.writeSync (fs.js:707:20)
    at Object.fs.writeFileSync (fs.js:1235:24)
    at [stdin]:1:15
    at Object.exports.runInThisContext (vm.js:54:17)
    at Object.<anonymous> ([stdin]-wrapper:6:22)
    at Module._compile (module.js:425:26)
    at node.js:589:27
    at doNTCallback0 (node.js:430:9)
    at process._tickCallback (node.js:359:13)

So we have the line with binding.writeBuffer where parameter values are 11 <Buffer 68 65 6c 6c 6f> 0 5 0 (I simply used console.log() to output parameter values).

binding.writeBuffer call leads to calling SYNC_CALL(write, nullptr, fd, &uvbuf, 1, pos) and this command throws an exception (a debug statement of mine after this call is not printed). SYNC_CALL(write, ...) leads to calling uv_fs_write function and it finishes. I don't know where exactly is the exception thrown (it's not in SYNC_DEST_CALL).

To make a conclusion, the problem seems to be in the fact that fs.writeFileSync uses a buffer implementation which sets offset value 0 for writing to a file (it is stdout in our case). However, this is inconclusive because I don't know Node architecture and my knowledge of C++ is fairly basic.

[DanielRosenwasser]

Why can't you just make a call to openSync and use the returned file descriptor with writeSync? That should do it for /dev/stdout.

[MartyIX]

@DanielRosenwasser You are right. This seems to work:

var fd = _fs.openSync(fileName, 'w');
_fs.writeSync(fd, data, undefined, "utf8");  // I can't use `0` as the third parameter (position) but I still need to set encoding parameter. I don't like it very much though.
_fs.closeSync(fd);

[MartyIX]

The behavior with the latest commit is:

node built/local/tsc.js --out /dev/stdout hello.ts # works
node built/local/tsc.js --out - hello.ts # creates '-' file :-)

[mklement0]

@MartyIX :) So just adding if (fileName === "-") { fileName = "/dev/stdout" } would do the trick?

@DanielRosenwasser: By saying that it's not necessarily cross-platform, are you just referring to the - convention of representing stdin/stdout? If so: true, that's a Unix convention, but it would be convenient on Windows, too - the only caveat is that creating files literally named - would then require ./- as a workaround, but I don't think that's much of a real-world concern.

[MartyIX]

@mklement0 I think so, yes.

I think that Daniel simply does not want to handle special cases because it's typically hard to do all the conventions right for all platforms. If one can come up with a solution without special handling, it also means that it will be easier to maintain it in future. My point here is not that "it's too much work, we won't do it". Anyway, that's only my interpretation and my understanding.

[mklement0]

@MartyIX: Understood.

Come to think of it, it is the /dev/stdout that's not portable - Windows has no analog, as far as I know (there is CON, which works in certain cases, but tsc simply creates a file by that name, which, due to using a reserved name, is then quite hard to get rid of, incidentally).

So the use of - could become the portable solution by fiat - it will then be the platform-neutral representation of stdout when using tsc - but that also means that just mapping - to /dev/stdout behind the scenes will not work on Windows.

That brings us back to process.stdout, which is Node.js' portable method for writing to stdout, it seems to me - though there may well be issues I'm unaware of.

Using - to mean "stdout, whatever it is on your platform" is a nice abstraction that requires minimal special handling - especially given that Node.js does the heavy lifting of handling stdout on all platforms.

As it stands (just to summarize), your proposed PR would only benefit Unix users that use literal target /dev/stdout; in other words:

• It's inconvenient to use on Unix systems
• Windows users are out of luck altogether.

[MartyIX]

@mklement0 I agree with you. I like the former solution better than the current one too.

Yet, any accepted patch is better than none from my point of view because the current behavior of tsc is really awkward on my Linux box.

[mklement0]

@MartyIX

Understood. Your patch is worth applying in any event, because it also fixes the inability to write to other kinds of special files, such as FIFOs (though that may be a more exotic use case).

And kudos for figuring out the issue with fs.writeFile[Sync] - you have to wonder whether the explicit 0 offset is intentional.

Ceterum censeo "-" esse implementam.

[DanielRosenwasser]

I was referring to /dev/stdout special handling being inappropriate.

Glad to hear the fix worked @MartyIX. Dealing with general file descriptors is the right way to handle it, so I think this is the appropriate fix. I'll test it out in a bit.

I'm not totally against allowing - as an argument with special meaning, but that would address #1226 specifically. I'm also not concerned with WScript handling for this. I assume dealing with #1226 would be when we'd think about it.

[mklement0]

@DanielRosenwasser: Glad to hear that - is not off the table; I just realized that I completely missed that this PR fixes #4841, not #1226 (because a comment of mine from the latter was referenced) - dealing with those two issues separately makes perfect sense.

[DanielRosenwasser]

So this doesn't work when you request non-inline sourcemaps, but that's not a big deal.

Can you wrap the change in a try/finally? Open and write in the try, close in the finally.

Basically, initialize the file descriptor to undefined (but typed as a number) and if it's not defined in the finally block, don't close it.

[MartyIX]

@DanielRosenwasser Sure

[DanielRosenwasser]

Thanks @MartyIX!