search

jakejs / jake

JavaScript build tool, similar to Make or Rake. Built to work with Node.js.
http://jakejs.com
Apache License 2.0
1.97k stars 190 forks source link

Set correct task status when starting a task #275

Closed veldsla closed 9 years ago

veldsla commented 9 years ago

Added simple test that detects simple cyclic dependencies at runtime.

mde commented 9 years ago

Could you write a test for ... this test? :D Don't worry about the breaking build right now. It's just stupid Node 0.8.

veldsla commented 9 years ago

Sure, but not before next Monday. It also bothers me that it only catches the parallel execution case. Although this could be fixed using the same proposal I did in issue #269. This might have other side effects which I haven't considered, but it does pass all the tests.

May be an easier approach is to catch the issue at the constructor level by simply comparing the task name to the prereq names. Both is maybe also a good idea...

veldsla commented 9 years ago

Tests added

SirAnthony commented 9 years ago

Prereq check in init will be skipped on dependency adding to existing task: https://github.com/jakejs/jake/blob/master/lib/jake.js#L201

mde commented 9 years ago

Oh, interesting. Yes, probably don't want to leave this loophole here.

mde commented 9 years ago

But if this is a partial fix for your issue, please feel free to merge if it's passing the Node v0.10 tests.

SirAnthony commented 9 years ago

I had not seen any checks for recursive dependencies, it might be an issue also. @mde, I already have some really nice stuff for jake, but it is lack of testing and depend on our internal code. But we planning to contribute back anyway, so I'll bring patches in nearest future.

mde commented 9 years ago

Would appreciate any contributions, thanks!

veldsla commented 9 years ago

Would it be better to move the checks to the createTask instead of the Task constructor? It looks like the only place new Task is called with arguments.

In my local version I also added a check that validates the prerequisites to be an array of strings. I found out that Jake silently exits when a task depends on a taskname that is undefined.

mde commented 9 years ago

Just be aware that createTask is called for other types of tasks than just simple Task: https://github.com/jakejs/jake/blob/master/lib/api.js

Adding some tests for this stuff would be awesome too.

SirAnthony commented 9 years ago

I merged it for now. But better dependency checking is still needed.

mde commented 9 years ago

Excellent, thanks for shepherding this!