-
Notifications
You must be signed in to change notification settings - Fork 231
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
Zeus doesn't reload code after change on macOS sierra #589
Comments
I wonder if something in https://github.com/fsnotify/fsevents/blob/master/fsevents.go needs to be changed to support Sierra? Sadly that project has no tests so it's hard to isolate :-/. Zeus' test should catch this but I'd need a VM running Sierra, I'll see if I can spin one up. |
@metcalf, seems so, I've recompiled the gem locally using go1.7.1 darwin/amd64, but the error is still there and the code reloading doesn't work :( |
@pavel-d: I got Zeus running inside a Sierra VM and can't reproduce this issue. What version of the Sierra preview are you running? Can you try creating a new, clean project with Zeus in it and see if the same issue occurs? |
@pavel-d: I upgraded my VM to the latest GM release of Sierra and still can't reproduce this. It'd be helpful to know if this repros for you on the latest Sierra release and with a new, clean project. |
@metcalf, thanks! Sure, I'll dig a bit later this week |
Hmm, having the same problem with an existing project in Sierra, will see if I can reproduce it on a new project too. |
I just uncovered a slew of other bugs and edge cases in Zeus' file monitoring code (e.g. if you replace a watched file by moving another on top of it, it's no longer watched because the underlying inode changed). The work required to handle all these edge cases is great enough that I'm looking into ways to use Facebook's watchman in place of the current file monitoring code. I'm not sure when I'll be able to land that (or if it will work) so it'd still be great to have a repro of the Sierra issues, but if you can't repro then hopefully a solution is in the works! |
I'm having the same exact issues as @pavel-d, Sierra 10.12 and zeus 0.15.10. Being stuck on Rails 3 never felt so bad. :( |
Is this still open? It seems like it's more of an issue with fsevents |
I'm having the same issue too. Sierra 10.12 (16A323), ruby 2.1.6p336, zeus 0.15.10.
and zeus does not reload code
|
@kyleholzinger Yes it's still open. The "closed" above your comment refers to the PR on the left which mentions this PR. |
Right, was just wondering since this seems like an issue with cgo, not zeus @zubin |
I've been researching this (since it's a blocker for our upgrade to macOS Sierra), and it seems something changed in Sierra to cause zeus's fsevents program to not work correctly anymore - the advice at facebook/react-native#9309 (comment) says to bump kernel file limits (they were very low in 10.11, and I wonder if they got bumped even lower in 10.12). The other piece of advice is to upgrade to watchman 4.6 (we don't use watchman in Zeus, but bear with me). It includes a bunch of fsevents-related changes over 4.5, mostly in stream setup, and the handling of the |
@antifuchs it's not zeus' fsevents specifically, it's cgo-- this is the line that it's breaking on, which is an external call But either way, a switch off of fsevents would fix the issue 😂 |
Oh, interesting, I thought zeus still ships its own objc fsevents wrapper! Disregard all I said, then (: |
@antifuchs I'm not 100% positive but I think it uses the go one |
+1 |
I was on sierra with 0.15.4 and was not experiencing this 2016-10-11 15:56 zeus-darwin-amd64[86241] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-22) After upgrading to 0.15.10 the warning comes up. |
Interesting, I wonder if they moved from native fsevents/self-packaged to cgo fsevents @pietdaniel |
Was anyone able to resolve this? I come from React-native side and following their steps didnt help to resolve it. |
I was able to resolve it as follows: facebook/react-native#9309 (comment) |
Changing max files as suggested does not fix it for me. |
@metcalf need a computer running Sierra to test? Wanna swing by one of these days?! I'm also experiencing this error and would be happy to help :-p |
@pariser (or anyone else) any chance you have a Go environment set up and can try running |
(err, sorry @andrew-stripe == @metcalf, I'm signed in from my work account) |
@andrew-stripe running now |
@andrew-stripe @metcalf just got this:
I tried both adding it to |
@kyleholzinger: Go likes a very particular directory structure. You need to set a |
@andrew-stripe I just had a clean run on eb88f08 with
|
That snippet of output only refers to the Ruby tests, I'd have expected to see failures (if any) in the Go tests. Those look like:
|
Sorry about that, here's the output I have:
|
Hmm, doesn't fail tests, that makes life more difficult. That would be consistent with the explanation that it has to do with max file limits since the tests watch very few files. Recent versions of Watchman encourage you to watch an entire project directory which may help avoid this. I'm not sure whether errors from the file watching code were surfaced in the same way in 0.15.4. It may have the same issue and is just swallowing the error message. Can anyone confirm whether they observe reloading on file changes on Sierra in 0.15.4 and not 0.15.10 (regardless of any error messages)? |
I'm not seeing any reloading behavior in v0.15.4 or v0.15.10. (Nor am I seeing any error messages in v0.15.4). |
Sounds like this was always broken and the old file monitor just swallowed the error messages. That's not surprising given how it worked. My barely-informed guess is that this has to do with Zeus tracking each individual file instead of setting up a watch on the project directory. The test suite doesn't track enough files to hit any kind of limit and raising the maxfiles limit works for some projects. Assuming this is correct, the fix could be local to the filemonitor package but it wouldn't be trivial. It's also possible that something in https://github.com/fsnotify/fsevents/blob/master/fsevents.go is broken for Sierra. We've decided to move away from Zeus in favor of our own code loader with Watchman for file monitoring. I'm happy to help review changes and answer questions but I don't expect to be investing much more in Zeus. |
@andrew-stripe Do you have some examples of replacing zeus with watchman? |
Unfortunately nothing off the shelf. We're building internal tooling that's On Thu, Nov 10, 2016 at 5:41 AM, Alexey Kuznetsov notifications@github.com
|
I've switched to using spring, seems it still works with the latest OSX sierra. |
From what I read in facebook/react-native#9309 , it looks like it's a permission issue? |
I wonder if a quick fix for that would be to monitor directory instead of files on osx, it seems to work everywhere except osx. I ran into it today as well but only on a larger rails app trying to figure out the limit now. |
This seems to be related to the number of monitored files as I have 2 apps 1 causes the issue (the bigger) and a smaller which does not experience it, this is on the same machine with the same version of zeus. To resolve this the way to go seems to be to monitor the directory instead of the files for changes and then filter in the filemonitor for events we are interested in (based on the file). I took a look at the code in filemonitor_darwin and it seems to be reasonable enough to change it here without the need to touch much else. First step so is to add a test as I wonder what the amount of files needed is. I'm gonna give this a shot when I have a Mac available (aka work). |
It seems like there is a problem with fsevents when dealing with a large amount of files on OSX sierra this added test fails reliably on mac Macbook Pro, with the error ``` 2017-01-17 22:52 filemonitor.test[69205] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-22) ``` This more or less reporduces #589 #605 #607 on the basis of this being related to many files the fix would be to not monitor files individualy on OSX but rather monitor directories and filter client side, this should reduce the number of files / directories to be monitored.
It would be nice to see that commit by @sideshowcoder merged into master and a new gem released. Having to constantly restart zeus is kind of antithetical to its existence. |
@siannopollo sadly the commit is not a fix yet, but just a failing test and a description of the fix I'm working towards, any help is greatly appricieated and I'm happy to merge as soon as it is ready 👍 |
@siannopollo if you fancy testing a local built, #620 now includes a potential fix for the problem, I have to do some more local testing before merging but it seems like it fixes the issue. |
Please check version 0.15.13.pre and see if it fixes the problem it seems to work for my setup. |
|
I'd consider this a fix then, seem to have the same success on other machines as I can see locally here 👍 |
|
I feel like from now on if this fixed anyones issue just respond with a reaction to one of the other messages haha. Plz no more emails ❤️ |
@kyleholzinger Don't you find it ironic that in order to voice that opinion, you had to leave a comment that would send emails to everyone on this thread? I mean, Github provides a Subscribe/Unsubscribe button at the top of each issue if you don't want to be notified any more... ¯_(ツ)_/¯ |
@kyleholzinger But you have no control over the actions of others. So instead of trying to dictate how others should behave to suit your preferences, why don't you use the tools that Github provides to achieve the experience you desire? |
@kyleholzinger Is classifying a seemingly civil discourse as "meaningless babel" also proper GitHub etiquette? Since this has devolved to name-calling, I'll unsubscribe from this thread now. |
🎭 |
Description of Problem
Zeus doesn't reload code after changes on macOS sierra
System details
Steps to Reproduce
zeus start
in a new rails projectIt boots fine, but one error is printed to the stdout:
touch config/application.rb
Observed Behavior
Nothing happens
Expected Behavior
zeus should reload the code
The text was updated successfully, but these errors were encountered: