New adapter for Windows and safer specs for Travis #56

Merged
merged 16 commits into from Aug 18, 2012

Conversation

Projects
None yet
7 participants
@Maher4Ever
Contributor

Maher4Ever commented Jul 25, 2012

Hello everyone,

For the last couple of weeks I've been working on a new gem called WDM to fix the performance issue Listen had on Windows. I've benchmarked it against FChange and the results were encouraging for me to keep developing it. WDM has some disadvantages too, so here is the quick run down:

Advantages of WDM:

  • It's 65175% faster than FChange in reporting changes.
  • It can report more changes.
  • It simplified the specs because it can detect changes recursively (changes in sub-directories).
  • It reports the path of the change and the type of the change.

Disadvantages:

  • It only works on ruby > 1.9.2.
  • It's an extension, so it needs Devkit on Windows to be installed. But then again, FChange relies on FFI and that's also an extension.
  • It's an MRI extension, so it won't run on JRuby or the other implementation for the moment.
  • It can't be included as a dependency in the gemspec of Listen as it will blow up while compiling (I still can't find a way in rubygems to specify that a gem can only be compiled on Windows).

WDM has been stable enough for the past 3 days and didn't crash once while I was using it.

As you may know, Listen has had a performance issue on Windows. Sometimes, it didn't even report changes. So I decided to implement the windows adapter in Listen with WDM and see how it works. It worked quite good and all the specs pass.

I also took the opportunity to make an end to the "russian roulette" we were playing with travis (thanks @netzpirat for this great description :) ). Adapters can now report changes on demand instead of using the latency, which allows us to wait until all tests has run and then check the results. The only disadvantage of this is that tests now require the developer to specify how many change he expects to get.

@guard/listen I'm wondering what you think about these changes. I would say WDM is in the alpha phase right now, so do you think it's a good decision to replace FChange with WDM any time soon? Also, what do you think about the fix for the travis problem?

Maher4Ever added some commits Jul 22, 2012

Stop relying on luck when testing adapters
Adapters can now be set to not report changes until explicitly commanded
to. This allows waiting for the expected amount of changes in tests.
Lower tests latency
This latency has another meaning in tests: It's the interval between
checking if the expected amount of changes is reached. So lowering it
translates into faster tests!
@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 25, 2012

This pull request fails (merged 38eec03 into e985b25).

This pull request fails (merged 38eec03 into e985b25).

@netzpirat

This comment has been minimized.

Show comment
Hide comment
@netzpirat

netzpirat Jul 26, 2012

Contributor

Oh wow, great work @Maher4Ever

It only works on ruby > 1.9.2.

Ruby 1.8.7 EOL is in 10 months, so it's not a big disadvantage.

It's an MRI extension, so it won't run on JRuby or the other implementation for the moment.

Oh, I think I can help with this and add Java 7 WatchService API support to listen and JRuby is a first class citizen again on all plattforms. I anyway planned to give some JRuby love to Guard, since it's a confession of failure currently.

It can't be included as a dependency in the gemspec of Listen as it will blow up while compiling (I still can't find a way in rubygems to specify that a gem can only be compiled on Windows).

I do not like these dependencies either and I'm in favor of #54

WDM has been stable enough for the past 3 days and didn't crash once while I was using it.

MERGE!

do you think it's a good decision to replace FChange with WDM any time soon?

Sounds like it is and the ultimate reason to do so is that it's maintained by you.

Also, what do you think about the fix for the travis problem?

I'd not care to specify the expected change count, since we do similar with mocks, but I prefer to have solid specs :P

Great work Maher.

Contributor

netzpirat commented Jul 26, 2012

Oh wow, great work @Maher4Ever

It only works on ruby > 1.9.2.

Ruby 1.8.7 EOL is in 10 months, so it's not a big disadvantage.

It's an MRI extension, so it won't run on JRuby or the other implementation for the moment.

Oh, I think I can help with this and add Java 7 WatchService API support to listen and JRuby is a first class citizen again on all plattforms. I anyway planned to give some JRuby love to Guard, since it's a confession of failure currently.

It can't be included as a dependency in the gemspec of Listen as it will blow up while compiling (I still can't find a way in rubygems to specify that a gem can only be compiled on Windows).

I do not like these dependencies either and I'm in favor of #54

WDM has been stable enough for the past 3 days and didn't crash once while I was using it.

MERGE!

do you think it's a good decision to replace FChange with WDM any time soon?

Sounds like it is and the ultimate reason to do so is that it's maintained by you.

Also, what do you think about the fix for the travis problem?

I'd not care to specify the expected change count, since we do similar with mocks, but I prefer to have solid specs :P

Great work Maher.

@rymai

This comment has been minimized.

Show comment
Hide comment
@rymai

rymai Jul 26, 2012

Member

Awesome @Maher4Ever!!! Thanks for taking care of Windows users! :)

Member

rymai commented Jul 26, 2012

Awesome @Maher4Ever!!! Thanks for taking care of Windows users! :)

@thibaudgg

This comment has been minimized.

Show comment
Hide comment
@thibaudgg

thibaudgg Jul 30, 2012

Member

Nice work, all green for me :)

Member

thibaudgg commented Jul 30, 2012

Nice work, all green for me :)

@Maher4Ever

This comment has been minimized.

Show comment
Hide comment
@Maher4Ever

Maher4Ever Jul 30, 2012

Contributor

Glad you all liked it :). I'll merge this after fixing #54

Contributor

Maher4Ever commented Jul 30, 2012

Glad you all liked it :). I'll merge this after fixing #54

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 31, 2012

This pull request fails (merged 2b7e352 into e985b25).

This pull request fails (merged 2b7e352 into e985b25).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 31, 2012

This pull request fails (merged cbb2d50 into e985b25).

This pull request fails (merged cbb2d50 into e985b25).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jul 31, 2012

This pull request fails (merged d7ca568 into e985b25).

This pull request fails (merged d7ca568 into e985b25).

Fix specs on rubinius
Use a patched version of rb-inotify until a new version is released.
@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 4, 2012

This pull request fails (merged ee7167d into e985b25).

This pull request fails (merged ee7167d into e985b25).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 4, 2012

This pull request fails (merged 4955b8c into e985b25).

This pull request fails (merged 4955b8c into e985b25).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 4, 2012

This pull request fails (merged 91e7eb1 into e985b25).

This pull request fails (merged 91e7eb1 into e985b25).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 15, 2012

This pull request passes (merged a62e0b1 into e985b25).

This pull request passes (merged a62e0b1 into e985b25).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 15, 2012

This pull request passes (merged ed6a39a into e985b25).

This pull request passes (merged ed6a39a into e985b25).

@thibaudgg

This comment has been minimized.

Show comment
Hide comment
@thibaudgg

thibaudgg Aug 16, 2012

Member

Ready to be merged? :)

Member

thibaudgg commented Aug 16, 2012

Ready to be merged? :)

@Maher4Ever

This comment has been minimized.

Show comment
Hide comment
@Maher4Ever

Maher4Ever Aug 16, 2012

Contributor

Yes it is :). Could you just confirm it works one more time (because of the new changes to WDM) so we don't end up segfaulting on user's machines?

Contributor

Maher4Ever commented Aug 16, 2012

Yes it is :). Could you just confirm it works one more time (because of the new changes to WDM) so we don't end up segfaulting on user's machines?

@thibaudgg

This comment has been minimized.

Show comment
Hide comment
@thibaudgg

thibaudgg Aug 16, 2012

Member

I'll try to give it a try this evening, thanks!

Member

thibaudgg commented Aug 16, 2012

I'll try to give it a try this evening, thanks!

@ghost ghost assigned Maher4Ever Aug 16, 2012

@thibaudgg

This comment has been minimized.

Show comment
Hide comment
@thibaudgg

thibaudgg Aug 17, 2012

Member

@Maher4Ever all specs are green on OS X! Congrats!

Member

thibaudgg commented Aug 17, 2012

@Maher4Ever all specs are green on OS X! Congrats!

@Maher4Ever

This comment has been minimized.

Show comment
Hide comment
@Maher4Ever

Maher4Ever Aug 18, 2012

Contributor

@thibaudgg Awesome! Thanks for testing :)

Contributor

Maher4Ever commented Aug 18, 2012

@thibaudgg Awesome! Thanks for testing :)

Maher4Ever added a commit that referenced this pull request Aug 18, 2012

Merge pull request #56 from guard/wdm
Replace FChange with WDM as the platform-specific adapter for Windows.

@Maher4Ever Maher4Ever merged commit c37d9aa into master Aug 18, 2012

1 check passed

default The Travis build passed
Details
@prusswan

This comment has been minimized.

Show comment
Hide comment
@prusswan

prusswan Sep 25, 2012

Hi, how is the new dependency manager supposed to work with guard? I just realized that on Ubuntu, the rb-inotify, rb-fchange, rb-fsevent gems are no longer included, and a warning will be shown:

> [Listen warning]:
  Missing dependency 'rb-inotify' (version '~> 0.8.8')!
  Please add the following to your Gemfile to satisfy the dependency:
    gem 'rb-inotify', '~> 0.8.8'

  For a better performance, it's recommended that you satisfy the missing dependency.
  Listen will be polling changes. Learn more at https://github.com/guard/listen#polling-fallback.

Note that this is not immediately noticeable for those who updated from older versions of guard, since those gems may still be present in Gemfile.lock

Hi, how is the new dependency manager supposed to work with guard? I just realized that on Ubuntu, the rb-inotify, rb-fchange, rb-fsevent gems are no longer included, and a warning will be shown:

> [Listen warning]:
  Missing dependency 'rb-inotify' (version '~> 0.8.8')!
  Please add the following to your Gemfile to satisfy the dependency:
    gem 'rb-inotify', '~> 0.8.8'

  For a better performance, it's recommended that you satisfy the missing dependency.
  Listen will be polling changes. Learn more at https://github.com/guard/listen#polling-fallback.

Note that this is not immediately noticeable for those who updated from older versions of guard, since those gems may still be present in Gemfile.lock

This comment has been minimized.

Show comment
Hide comment
@thibaudgg

thibaudgg Sep 25, 2012

Member

Just add

gem 'rb-inotify', '~> 0.8.8'

before gem 'guard' and you'll be good to go.

Member

thibaudgg replied Sep 25, 2012

Just add

gem 'rb-inotify', '~> 0.8.8'

before gem 'guard' and you'll be good to go.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 16, 2013

Coverage Status

Changes Unknown when pulling ed6a39a on wdm into * on master*.

Coverage Status

Changes Unknown when pulling ed6a39a on wdm into * on master*.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 22, 2013

Coverage Status

Changes Unknown when pulling ed6a39a on wdm into * on master*.

Coverage Status

Changes Unknown when pulling ed6a39a on wdm into * on master*.

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