Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

respect argument order when compiling multiple files to stdout #3087

Closed
wants to merge 1 commit into from

4 participants

@davidchambers

I expected this to produce a JavaScript file containing the compiled output of each of the three source files in the specified order:

dist/quux.js: src/foo.coffee src/bar.coffee src/baz.coffee
    @node_modules/.bin/coffee --compile --print $+ > $@

Apparently this usage was not anticipated, as order in which the compiled code is written to stdout in nondeterministic (i.e. not necessarily foo then bar then baz).

@vendethiel
Collaborator

I think that's your OS's fault tho, isn't it ? We only see the final command

@davidchambers

I think that's your OS's fault tho, isn't it ? We only see the final command

I don't follow, @Nami-Doc. My understanding is that process.argv grants access to the arguments in the order in which they were specified, and that coffee could choose to respect this order.

Perhaps I shouldn't have used a Makefile snippet to demonstrate the point, as the issue does not lie with Make; coffee --compile --print src/foo.coffee src/bar.coffee src/baz.coffee is sufficient to reproduce the issue.

@vendethiel
Collaborator

EDIT : link was for a directory.

@davidchambers

That's a different issue, isn't it? In that case coffee is being asked to compile all the files in a directory. Here, it's being asked to compile three files explicitly. The question is whether coffee should respect the order in which the arguments are provided. I would argue this should be the case when --print is specified, otherwise --print is only useful when compiling one file (or stdin).

@vendethiel
Collaborator

e581f7d/command.coffee#L107

we'd have to wait, one way or another, before triggering script n+1 compilation. Promise, triggering compilation, etc, but it gets quite complicated when you mix files and paths

@davidchambers

it gets quite complicated when you mix files and paths

We needn't worry about that case. As was discussed in #3003, directory enumeration order should not be relied upon. Passing multiple file paths to coffee --compile --print should be deterministic, though. This is something I'd very much like to see fixed, so I'll have a go at it myself and open a pull request if I have success.

@vendethiel
Collaborator

I'm talking about coffee lib/ file.coffee. lib's content should be compiled then file.coffee, no?

@davidchambers

I'm talking about coffee lib/ file.coffee. lib's content should be compiled then file.coffee, no?

Take the following directory structure:

├── file.coffee
└── lib
    ├── a.coffee
    ├── b.coffee
    └── c.coffee

coffee --compile --print lib is nondeterministic, since there are six (3!) possible orders in which the operating system will enumerate the directory. Given this, I see no value in ensuring that lib's content is compiled before file.coffee. All we'd be doing is ensuring that the output is one of six—rather than twelve or twenty-four—possible strings.

Having thought about this some more, I think coffee should throw an exception if a directory path is passed to coffee --compile --print. Otherwise, if lib/b.coffee and lib/c.coffee depend on lib/a.coffee, running coffee --compile --print lib will sometimes produce an unusable JavaScript file.

@vendethiel
Collaborator

Given this, I see no value in ensuring that lib's content is compiled before file.coffee.

We still need it to work with the single-file implementation

@davidchambers

We still need it to work with the single-file implementation

What do you mean by this?

@vendethiel
Collaborator

If we make the single-file implementation use, say, a promise-like, or trigger file n+1, we still need to change the way it's handled in the directory compilation, in cases they're files with the dir

@davidchambers

I'm with you. I think the code could be structured something like this:

if '--print' in process.argv
  for source in sources
    if fs.statSync(source).isDirectory()
      throw new Error 'Directory enumeration would produce nondeterministic output'
  # compile files synchronously, in order
else
  # existing code
src/command.coffee
@@ -76,8 +76,28 @@ exports.run = ->
literals = if opts.run then sources.splice 1 else []
process.argv = process.argv[0..1].concat literals
process.argv[0] = 'coffee'
- for source in sources
- compilePath source, yes, path.normalize source
+ if opts.print
+ output = []
+ # XXX: Redefine printLine (called by compileScript) so nothing is
+ # written to stdout until all files have been compiled successfully.
+ printLine = (line) -> output.push line

This is unsavoury, but I couldn't think of another way to reuse the logic in compileScript. Any ideas?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/command.coffee
@@ -76,8 +76,28 @@ exports.run = ->
literals = if opts.run then sources.splice 1 else []
process.argv = process.argv[0..1].concat literals
process.argv[0] = 'coffee'
- for source in sources
- compilePath source, yes, path.normalize source
+ if opts.print
+ output = []
+ # XXX: Redefine printLine (called by compileScript) so nothing is
+ # written to stdout until all files have been compiled successfully.
+ printLine = (line) -> output.push line
+ for source in sources
+ try
+ code = fs.readFileSync source
+ catch err then switch err.code
+ when 'ENOENT'
+ process.stderr.write "File not found: #{source}\n"

Which is preferred: process.stderr.write, console.error, or printWarn?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/command.coffee
((8 lines not shown))
+ # XXX: Redefine printLine (called by compileScript) so nothing is
+ # written to stdout until all files have been compiled successfully.
+ printLine = (line) -> output.push line
+ for source in sources
+ try
+ code = fs.readFileSync source
+ catch err then switch err.code
+ when 'ENOENT'
+ process.stderr.write "File not found: #{source}\n"
+ process.exit 1
+ when 'EISDIR'
+ process.stderr.write 'Directory enumeration would yield nondeterministic --print output\n'
+ process.exit 1
+ else
+ throw err
+ compileScript source, code.toString(), path.normalize source

Which is preferred: code.toString() or "#{code}"? (I went with the former as it's used elsewhere in this file.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davidchambers

Does this approach seem reasonable, @Nami-Doc? It's a bit more involved than I imagined; I'd appreciate suggestions on how this patch could be improved.

@vendethiel
Collaborator

Yeah, I read it when you committed, but it seems a bit too involved to my taste.

(btw, I'll have a hard time being present for august, starting tomorrow)

@davidchambers

Another option is simply to disallow directory paths in conjunction with --print, since one always has the option of calling coffee n times. The Makefile snippet, for example, could be rewritten as follows:

dist/quux.js: src/foo.coffee src/bar.coffee src/baz.coffee
    @> $@
    @node_modules/.bin/coffee --compile --print src/foo.coffee >> $@
    @node_modules/.bin/coffee --compile --print src/bar.coffee >> $@
    @node_modules/.bin/coffee --compile --print src/baz.coffee >> $@

The only unacceptable outcome is leaving things as they are now. Having coffee --compile generate a working JavaScript file some of the time is madness inducing. To update the compiled JavaScript file in one of my projects I currently have to run the following commands repeatedly until I get lucky and the files are compiled in the right order:

$ rm -r dist
$ make
$ git diff dist  # check to see whether the files were included in the right order
@davidchambers

Would you mind chiming in on this, @jashkenas?

@jashkenas
Owner

It would be ideal to compile the files in order. I agree.

@davidchambers

Is the pull request acceptable in its current form? I'm happy to make any necessary changes.

@jashkenas
Owner

Not when it's still got an XXX todo note in it ;) Finish it up the best you're able, and I'll be happy to take a peek.

@davidchambers

The XXX is to draw attention to the fact that we're assigning not to a local variable, but to a variable shared by all the functions in the file. This is frowned upon—for good reason—but in this case I consider it warranted. Other options are to add a printLine parameter to compileScript (defaulting to the function of the same name), or to duplicate the relevant code in compileScript. These options seem less appealing than redefining printLine before calling compileScript.

@jashkenas
Owner

Right. So if you then call compile again, it'll break, because the function has been redefined? How is that going to work?

@vendethiel
Collaborator

Btw, calling compile a second time already break on some cases, like when you're passing the same option object - we mutate it. There's another issue for that, tho

@davidchambers

Right. So if you then call compile again, it'll break, because the function has been redefined? How is that going to work?

I was thinking only of the case where this file is run via coffee --compile. To ensure the module works correctly when required, we'd need to reinstate the original printLine:

originalPrintLine = printLine
printLine = (line) -> output.append line
# ...
printLine = originalPrintLine

I'll update the pull request shortly.

lib/coffee-script/command.js
((5 lines not shown))
if (itExists) {
return compile();
} else {
- return mkdirp(jsDir, function(err) {
- if (err) {
- printLine("Error while creating dir " + jsDir + ": " + err);
- exec("mkdir -p " + jsDir);
- }
- return compile();
- });
+ return mkdirp(jsDir, compile);

It looks as though this file needs to be updated on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davidchambers davidchambers commented on the diff
src/command.coffee
((14 lines not shown))
+ code = fs.readFileSync source
+ catch err then switch err.code
+ when 'ENOENT'
+ printWarn "File not found: #{source}"
+ process.exit 1
+ when 'EISDIR'
+ printWarn 'Directory enumeration would yield nondeterministic --print output'
+ process.exit 1
+ else
+ throw err
+ compileScript source, code.toString(), path.normalize source
+ printLine = originalPrintLine
+ printLine line for line in queue
+ else
+ for source in sources
+ compilePath source, yes, path.normalize source

Do we care about the verbosity of the resulting JavaScript? Adding return here would obviate the need for accumulation in both branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@davidchambers

Is the originalPrintLine = printLine trick okay with you, @jashkenas?

@epidemian

Couldn't the compileScript function return the compiled output instead of writing it directly to stdout? That was the functions that call compileScript would be able to choose what to do with the output (e.g. print it immediately or store it into an array). Another option could be to parametrize that behaviour as a callback function in compileScript.

@davidchambers

Couldn't the compileScript function return the compiled output instead of writing it directly to stdout?

Certainly. The function is called in quite a few places, so such a change isn't trivial. It would be nice to get rid of the hack, though, so I'll see what I can do. :)

@epidemian

http://imgur.com/IoDY7YS

Hope you can find a way to make it nicer, @davidchambers. I think the code in command.coffee is already quite more complicated than what it should be :sweat:

@jashkenas
Owner

Is the originalPrintLine = printLine trick okay with you, @jashkenas?

Not really. We should be calling functions that are named and doing clear things, not surprise-patching. Looking forward to the cleanup.

@jashkenas
Owner

I'm not seeing this problem myself, but I assume that it only happens for large files, that might take a little bit longer to fs.readFile in.

Should we simply change to a readFileSync here? Or are there good reasons to leave compilePath asynchronous?

@jashkenas jashkenas referenced this pull request
Closed

release 1.6.4 #3141

@davidchambers

I'm not seeing this problem myself, but I assume that it only happens for large files, that might take a little bit longer to fs.readFile in.

Yep. The nastiest thing about the current behaviour is that a build script may do the right thing 95% of the time.

Should we simply change to a readFileSync here? Or are there good reasons to leave compilePath asynchronous?

I'm unsure of the consequences — compilePath is called in watchDir — but if you decide it's a good idea I'm happy to take a stab at it.

I've spent some time thinking about how we could fix this issue without my nasty hack, but the following factors conspire to make the task difficult:

  • compileScript is called in several places;

  • it does one of several things when called; and

  • what it does is determined by state stored in the opts object in module scope.

Making compilePath synchronous might be a simpler fix, but as I say I don't understand the impact of this change.

@marchaefner marchaefner referenced this pull request from a commit in marchaefner/coffee-script
@marchaefner marchaefner Fixes #3087 -- Use `fs.*Sync` for CLI compilation
 * Make `compilePath` synchronous
 * Remove unused variable
96ae98f
@jashkenas jashkenas closed this in #3234
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 66 additions and 8 deletions.
  1. +42 −6 lib/coffee-script/command.js
  2. +24 −2 src/command.coffee
View
48 lib/coffee-script/command.js
@@ -53,7 +53,7 @@
optionParser = null;
exports.run = function() {
- var literals, source, _i, _len, _results;
+ var code, err, line, literals, originalPrintLine, queue, source, _i, _j, _k, _len, _len1, _len2, _results, _results1;
parseOptions();
if (opts.nodejs) {
return forkNode();
@@ -82,12 +82,48 @@
literals = opts.run ? sources.splice(1) : [];
process.argv = process.argv.slice(0, 2).concat(literals);
process.argv[0] = 'coffee';
- _results = [];
- for (_i = 0, _len = sources.length; _i < _len; _i++) {
- source = sources[_i];
- _results.push(compilePath(source, true, path.normalize(source)));
+ if (opts.print) {
+ queue = [];
+ originalPrintLine = printLine;
+ printLine = function(line) {
+ return queue.push(line);
+ };
+ for (_i = 0, _len = sources.length; _i < _len; _i++) {
+ source = sources[_i];
+ try {
+ code = fs.readFileSync(source);
+ } catch (_error) {
+ err = _error;
+ switch (err.code) {
+ case 'ENOENT':
+ printWarn("File not found: " + source);
+ process.exit(1);
+ break;
+ case 'EISDIR':
+ printWarn('Directory enumeration would yield nondeterministic --print output');
+ process.exit(1);
+ break;
+ default:
+ throw err;
+ }
+ }
+ compileScript(source, code.toString(), path.normalize(source));
+ }
+ printLine = originalPrintLine;
+ _results = [];
+ for (_j = 0, _len1 = queue.length; _j < _len1; _j++) {
+ line = queue[_j];
+ _results.push(printLine(line));
+ }
+ return _results;
+ } else {
+ _results1 = [];
+ for (_k = 0, _len2 = sources.length; _k < _len2; _k++) {
+ source = sources[_k];
+ _results1.push(compilePath(source, true, path.normalize(source)));
+ }
+ return _results1;
}
- return _results;
};
compilePath = function(source, topLevel, base) {
View
26 src/command.coffee
@@ -77,8 +77,30 @@ exports.run = ->
literals = if opts.run then sources.splice 1 else []
process.argv = process.argv[0..1].concat literals
process.argv[0] = 'coffee'
- for source in sources
- compilePath source, yes, path.normalize source
+ if opts.print
+ queue = []
+ # XXX: Redefine printLine (called by compileScript) so nothing is
+ # written to stdout until all files have been compiled successfully.
+ originalPrintLine = printLine
+ printLine = (line) -> queue.push line
+ for source in sources
+ try
+ code = fs.readFileSync source
+ catch err then switch err.code
+ when 'ENOENT'
+ printWarn "File not found: #{source}"
+ process.exit 1
+ when 'EISDIR'
+ printWarn 'Directory enumeration would yield nondeterministic --print output'
+ process.exit 1
+ else
+ throw err
+ compileScript source, code.toString(), path.normalize source
+ printLine = originalPrintLine
+ printLine line for line in queue
+ else
+ for source in sources
+ compilePath source, yes, path.normalize source

Do we care about the verbosity of the resulting JavaScript? Adding return here would obviate the need for accumulation in both branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
# Compile a path, which could be a script or a directory. If a directory
# is passed, recursively compile all '.coffee', '.litcoffee', and '.coffee.md'
Something went wrong with that request. Please try again.