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

Fall back to polling w/ warning on OS X 10.6-10.8 #370

Merged
merged 2 commits into from
Apr 22, 2016

Conversation

vaz
Copy link
Contributor

@vaz vaz commented Apr 22, 2016

If OS X >= 10.9 (Darwin >= 13.0): all is well.

Else if OS X >= 10.6 (Darwin >= 10.0): if rb-fsevent > 0.9.4, fall back to polling and show warning recommending to use rb-fsevent=0.9.4

(Darwin < 10 not supported.)

See this gist for test: https://gist.github.com/vaz/fd6ee4c3563425ec11c24b96cc9da13b

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.687% when pulling e97d2e4 on vaz:old-darwin-fallback into 14e6c00 on guard:master.

ensure
$VERBOSE = warn_level
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find this a bit ugly but... otherwise on OS X 10.6 to 10.8 the spec output is full of warnings. Maybe there's a nicer way, like defaulting to calling rspec with RUBYOPT='-W0'... not sure if there are warnings you'd rather not suppress though; input appreciated.

Copy link
Contributor

Choose a reason for hiding this comment

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

What kinds of warnings? I don't see any in the Travis build log (from the earlier commit of yours).

If you really need this, I think it's best to mark which specs need this work around and use RSpec metadata to set $VERBOSE only on those tests. (see: https://www.relishapp.com/rspec/rspec-core/docs/metadata/user-defined-metadata).

You may also only set $VERBOSE for OSX at those versions.

I don't mind it being ugly - just it's not useful to make it global and on every OS the tests are run. Especially since I don't know which warnings and which specs need this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warnings are from the Kernel.warn that I added specifically for older Mac versions... so they would only show up in that case.

I was following the precedent from the BSD adapter in using Kernel.warn for that message rather than Listen::Logger.

On thinking it over I think it would be better to just take this out and the few contributors who might notice these warnings can just turn verbosity down from the command line if they really want to (RUBYOPT='-W0' bundle exec rspec). Thanks for the link on metadata though, good to know.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 97.712% when pulling 4a9aeaf on vaz:old-darwin-fallback into 14e6c00 on guard:master.

require 'rb-fsevent'
fsevent_version = Gem::Version.new(FSEvent::VERSION)
fsevent0_9_4 = Gem::Version.new('0.9.4') # supports 10.6-10.8
raise 'version too new' if fsevent_version > fsevent0_9_4
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of raising and catching, you can just return true here and show the version error only to match the case, e.g.

return true if fsevent_version <= fsevent0_9_4
Kernel.warn BUNDLER_DECLARE_GEM
false
rescue LoadError
false
end

(That's because a LoadError shouldn't talk about version problems, or that will confuse users).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also rename BUNDLER_DECLARE_GEM with e.g. UNSUPPORTED_OSX_VERSION or something like that. It's just an error message with a hint, so it should be named after the error, not the hint.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just as a shortcut, you can use String's built-in regexp capture:

return false unless (darwin_major_version_string = RbConfig::CONFIG['target_os'][OS_REGEXP, 1])
return true if Integer(darwin_major_version_string) >= 13

Or

darwin_version = Integer(RbConfig::CONFIG['target_os'][OS_REGEXP, 1] || "-1")
return false if darwin_version == -1
return true if darwin_version >= 13

Copy link
Contributor

Choose a reason for hiding this comment

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

Just so it's clear, you don't need to overried the BUNDLER_DECLARE_GEM constant at all, because a LoadError won't (shouldn't?) happen since rb-fsevent is always a dependency (even on Linux - it just does nothing). Only BSD and Windows users really need BUNDLER_DECLARE_GEM in their adapters.

So in this case, UNSUPPORTED_OSX_VERSION would be an entirely different constant - unrelated to BUNDLER_DECLARE_GEM, even if it seems similar. (In this situation it's about fixing a broken gem vs not having an optimized backend adapter for a given os).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I see what you mean, rb-fsevent will always be present, so a LoadError should not happen. I used the same name as the BSD adapter for the warning message constant since it seemed similar (add this to your gemfile or you'll be polling) but I'll make it more descriptive.

I'm simplifying Darwin.usable? quite a bit, thanks for the suggestions.

@e2
Copy link
Contributor

e2 commented Apr 22, 2016

I can merge and release this after the minor changes above.

Everything else looks good to me.

Surprisingly lots of work was needed for this.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 97.703% when pulling 014ae86 on vaz:old-darwin-fallback into 14e6c00 on guard:master.

@e2
Copy link
Contributor

e2 commented Apr 22, 2016

It's awesome work!

For readability it might be better to implicitly return false instead of relying on Kernel.warn to return nil. But I don't mind personally since that's covered by tests.

Just squash the changes (so the history isn't as intimidating for future contributors) and I'll merge and release immediately.

@vaz vaz force-pushed the old-darwin-fallback branch from 014ae86 to 859547c Compare April 22, 2016 20:40
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 97.702% when pulling 859547c on vaz:old-darwin-fallback into 14e6c00 on guard:master.

@vaz
Copy link
Contributor Author

vaz commented Apr 22, 2016

Thanks! Agreed on the explicit false return value, it is more readable. Added that and squashed.

@e2 e2 merged commit 3dea1ba into guard:master Apr 22, 2016
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