Skip to content
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

Refactor Cake tasks #4440

Merged
merged 9 commits into from
Feb 18, 2017
Merged

Refactor Cake tasks #4440

merged 9 commits into from
Feb 18, 2017

Conversation

GeoffreyBooth
Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth commented Feb 14, 2017

  • All the documentation tasks now have watch and non-watch versions.
  • New cake release task to cut a release.
  • Remove install task.
  • New cake build:watch and cake build:watch:harmony tasks to build and test and rebuild on changes to the source or tests; based on https://github.com/GeoffreyBooth/coffeescript-gulp

@GeoffreyBooth GeoffreyBooth requested a review from lydell February 14, 2017 06:34

buildAnnotatedSource = (watch = no) ->
do generateAnnotatedSource = ->
exec "node_modules/docco/bin/docco src/*.*coffee --output docs/v#{majorVersion}/annotated-source", (err) -> throw err if err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most minor thing ever ... I know that you didn't change anything here, but I got curious. I always thought ./node_modules/.bin/whatever was the way to go when invoking "binaries" installed locally with npm. I guess it doesn't matter. :)

Copy link
Collaborator

@lydell lydell left a comment

Choose a reason for hiding this comment

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

This is great! Good job! 👍

@jashkenas
Copy link
Owner

Very nice.

@GeoffreyBooth GeoffreyBooth changed the title [wip] Refactor Cake tasks Refactor Cake tasks Feb 15, 2017
@GeoffreyBooth
Copy link
Collaborator Author

Hey guys, I think I have build:watch working. It required quite a bit of refactoring so I’d appreciate a second or third pair of eyes reviewing this. @jashkenas @lydell

So there are now cake build:watch and cake build:watch:harmony tasks to build and test and rebuild on changes to the source or tests.

@GeoffreyBooth GeoffreyBooth mentioned this pull request Feb 15, 2017
3 tasks
@GeoffreyBooth
Copy link
Collaborator Author

@jashkenas I’ve updated the code per your comment. Do you or @lydell have any other notes?

@lydell
Copy link
Collaborator

lydell commented Feb 17, 2017

I had some trouble with cake build:watch.

First, the terminal screen was cleared. Is that intended?

Then, the following message appeared:

building, including parser...

After that, I ended up in an endless loop of:

building... 
passed 827 tests in 12.88 seconds 

testing... 

One line appears at a time, and then the screen is cleared before staring over again.


Here's me trying cake doc:site:watch and making a single edit to documentation/sections/cake.md:

$ cake doc:site:watch
compiled documentation/index.html → docs/v1/index.html
watching... 
compiled documentation/index.html → docs/v1/index.html
compiled documentation/index.html → docs/v1/index.html
compiled documentation/index.html → docs/v1/index.html
compiled documentation/index.html → docs/v1/index.html
compiled documentation/index.html → docs/v1/index.html
compiled documentation/index.html → docs/v1/index.html
compiled documentation/index.html → docs/v1/index.html

I'd expected compiled documentation/index.html → docs/v1/index.html to be printed only once.

Something similar happens for cake doc:test:watch:

$ cake doc:test:watch
compiled documentation/test.html → docs/v1/test.html
watching... 
compiled documentation/test.html → docs/v1/test.html
compiled documentation/test.html → docs/v1/test.html
compiled documentation/test.html → docs/v1/test.html

And for cake:source:watch:

$ cake doc:source:watch
generated annotated source in docs/v1/annotated-source/
watching... 
generated annotated source in docs/v1/annotated-source/
generated annotated source in docs/v1/annotated-source/
generated annotated source in docs/v1/annotated-source/
generated annotated source in docs/v1/annotated-source/
generated annotated source in docs/v1/annotated-source/
generated annotated source in docs/v1/annotated-source/
generated annotated source in docs/v1/annotated-source/
generated annotated source in docs/v1/annotated-source/
generated annotated source in docs/v1/annotated-source/
/home/lydell/forks/coffeescript/Cakefile:382
          throw err;
          ^

Error: Command failed: node_modules/docco/bin/docco src/*.*coffee --output docs/v1/annotated-source

/home/lydell/forks/coffeescript/node_modules/docco/docco.js:16
          throw error;
          ^
Error: ENOENT: no such file or directory, chmod '/home/lydell/forks/coffeescript/docs/v1/annotated-source/docco.css'

    at ChildProcess.exithandler (child_process.js:211:12)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:191:7)
    at maybeClose (internal/child_process.js:885:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:226:5)

I'm on Ubuntu 16.04 if that helps.

@GeoffreyBooth
Copy link
Collaborator Author

GeoffreyBooth commented Feb 17, 2017

The clearing is intended. That’s how my Gulp version works. When tests fail, they quickly fill up the screen and often spill into several screenfuls; I found it convenient to be able to quickly scroll up to the first failed test, without needing to scan for when the previous batch of failed tests ended.

I’ve only tested on OS X. Maybe fs.watch behaves differently in Ubuntu? From working with Chokidar I’ve become well aware of fs.watch‘s bugs and deficiencies in different operating systems; read the top of this readme, especially “fs.watch . . . often reports events twice.” I guess I could reimplement this to use Chokidar, or add some kind of checks that the rebuild isn’t triggered within a certain timeout or MD5 of the file changed . . . or is watch mode in Ubuntu something we want to support? Presumably you would’ve had the same issues in the old cake doc:site, as it has the same implementation as all the new watch tasks. Also what version of Node are you running? Maybe fs.watch has improved in newer versions.

@lydell
Copy link
Collaborator

lydell commented Feb 17, 2017

I'm running Node.js 7.2. If you didn't change the implementation of the watching I guess everything is fine, since this PR isn't about fixing watch bugs. If you want to use chokidar I think that would be a good idea, though.

Still, isn't it weird that cake build:watch entered some infinite loop while the other watchers didn't?

@GeoffreyBooth
Copy link
Collaborator Author

GeoffreyBooth commented Feb 17, 2017

A test in error_messages.coffee creates a file in test called syntax_error.coffee. On Mac this shows up as a rename event, which I’ve filtered out; but it must appear differently in Ubuntu.

It shouldn’t be creating a file in test in the first place; I’ve updated it to put it in os.tmpdir like the similar REPL test.

I tested it in a Docker node container, which is Ubuntu, and it seems to work. Editing files causes the change events to fire twice so the code builds and tests twice, but it’s not in an endless loop. I think this is related to the known bugs in fs.watch . . . I guess improving the watch accuracy can be a separate issue/PR?

@lydell
Copy link
Collaborator

lydell commented Feb 18, 2017

Nice! That fixed the infinite loop problem.

building... 
testing... 
testing... 
testing... 
testing... 
testing... 
testing... 
testing... 
passed 827 tests in 37.91 seconds 

passed 827 tests in 37.55 seconds 

fs.js:996
  return binding.unlink(pathModule._makeLong(path));
                 ^

Error: ENOENT: no such file or directory, unlink '/tmp/.coffee_history_test'
    at Object.fs.unlinkSync (fs.js:1:1)
    at process.<anonymous> (/home/lydell/forks/coffeescript/test/repl.coffee:1:1)
    at emitOne (events.js:1:1)
    at process.emit (events.js:1:1)


passed 827 tests in 35.33 seconds 

fs.js:996
  return binding.unlink(pathModule._makeLong(path));
                 ^

Error: ENOENT: no such file or directory, unlink '/tmp/.coffee_history_test'
    at Object.fs.unlinkSync (fs.js:1:1)
    at process.<anonymous> (/home/lydell/forks/coffeescript/test/repl.coffee:1:1)
    at emitOne (events.js:1:1)
    at process.emit (events.js:1:1)


failed 1 and passed 826 tests in 37.40 seconds 



  reads history file 

  AssertionError: Expected 3 to equal coffee> 
    at exports.eq (/home/lydell/forks/coffeescript/test/support/helpers.coffee:16:34)
    at /home/lydell/forks/coffeescript/test/repl.coffee:1:1
    at Function.<anonymous> (/home/lydell/forks/coffeescript/test/repl.coffee:1:1)
    at global.test (/home/lydell/forks/coffeescript/Cakefile:1:1)
    at testRepl (/home/lydell/forks/coffeescript/test/repl.coffee:1:1)
    at Object.<anonymous> (/home/lydell/forks/coffeescript/test/repl.coffee:1:1)
    at Object.<anonymous> (/home/lydell/forks/coffeescript/test/repl.coffee:1:1)
    at Module._compile (module.js:1:1)
    at Object.exports.run (/home/lydell/forks/coffeescript/lib/coffee-script/coffee-script.js:1:1)
    at runTests (/home/lydell/forks/coffeescript/Cakefile:1:1)
    at Object.action (/home/lydell/forks/coffeescript/Cakefile:1:1)
    at invoke (/home/lydell/forks/coffeescript/lib/coffee-script/cake.js:1:1)
    at Object.exports.run (/home/lydell/forks/coffeescript/lib/coffee-script/cake.js:1:1)
    at Object.<anonymous> (/home/lydell/forks/coffeescript/bin/cake:1:1)
    at Module._compile (module.js:1:1)
    at Object.Module._extensions..js (module.js:1:1)
    at Module.load (module.js:1:1)
    at tryModuleLoad (module.js:1:1)
    at Function.Module._load (module.js:1:1)
    at Module.runMain (module.js:1:1)
    at run (bootstrap_node.js:1:1)
    at startup (bootstrap_node.js:1:1)
    at bootstrap_node.js:1:1
 

  function () {
      return fn(input, output, repl);
    }

fs.js:996
  return binding.unlink(pathModule._makeLong(path));
                 ^

Error: ENOENT: no such file or directory, unlink '/tmp/.coffee_history_test'
    at Object.fs.unlinkSync (fs.js:1:1)
    at process.<anonymous> (/home/lydell/forks/coffeescript/test/repl.coffee:1:1)
    at emitOne (events.js:1:1)
    at process.emit (events.js:1:1)


passed 827 tests in 39.54 seconds 

passed 827 tests in 36.16 seconds 

passed 827 tests in 37.25 seconds 

fs.js:996
  return binding.unlink(pathModule._makeLong(path));
                 ^

Error: ENOENT: no such file or directory, unlink '/tmp/.coffee_history_test'
    at Object.fs.unlinkSync (fs.js:1:1)
    at process.<anonymous> (/home/lydell/forks/coffeescript/test/repl.coffee:1:1)
    at emitOne (events.js:1:1)
    at process.emit (events.js:1:1)

@GeoffreyBooth
Copy link
Collaborator Author

Such weirdness around files cross-platform. Okay, I added a check around the deletion of the REPL test history file. Now all good?

@lydell
Copy link
Collaborator

lydell commented Feb 18, 2017

I think we're all good now, because I suspect the following happens due to compiling and/or testing several times rapidly:

building... 
testing... 
testing... 
testing... 
testing... 
testing... 
/home/lydell/forks/coffeescript/lib/coffee-script/coffee-script.js:285
  lexer = new Lexer;
          ^

TypeError: Lexer is not a constructor
    at Object.<anonymous> (/home/lydell/forks/coffeescript/lib/coffee-script/coffee-script.js:285:11)
    at Object.<anonymous> (/home/lydell/forks/coffeescript/lib/coffee-script/coffee-script.js:425:4)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/home/lydell/forks/coffeescript/lib/coffee-script/cake.js:13:18)

/home/lydell/forks/coffeescript/lib/coffee-script/coffee-script.js:54
        throw helpers.updateSyntaxError(err, code, options.filename);
        ^

TypeError: yy.addLocationDataFn is not a function
    at Object.anonymous (/home/lydell/forks/coffeescript/lib/coffee-script/parser.js:123:13)
    at Parser.parse (/home/lydell/forks/coffeescript/lib/coffee-script/parser.js:829:36)
    at /home/lydell/forks/coffeescript/lib/coffee-script/coffee-script.js:94:24
    at /home/lydell/forks/coffeescript/lib/coffee-script/coffee-script.js:48:19


passed 827 tests in 14.15 seconds 

passed 827 tests in 14.48 seconds 

passed 827 tests in 14.70 seconds 

But, as you say, improving watch could be another PR.

So, for me, it's OK to merge now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants