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

Add GOPATH to the list of directories that is monitored for changes. #153

Closed
wants to merge 2 commits into from

Conversation

vanackere
Copy link
Contributor

I find very useful to have the application also automatically recompiled when modifying external libraries located in GOPATH. The following trivial change does just that (the new config option 'watch.gopath' allows to disable this behaviour).

The new config option 'watch.gopath' allows to disable this behaviour.
@vanackere
Copy link
Contributor Author

Only watching the dependencies of the app code is workable but will involve adding/removing watchers each time dependencies are added/removed in the code so I feel this is a lot of work for little gain.
I have no access to OSX systems so cannot make any test/suggestion... On Linux at least, I believe that inotify can watch directories and so we're not likely to encounter any limit on a regular project. If you're worried, I'd suggest including the feature, but disable it by default in a first time, then we can work out solutions (eg a fallback to some regular polling ...) if we encounter cases that don't work.

@jgraham909
Copy link
Contributor

+1 for this... when revel doesn't watch the import path you can sometimes end up with errors in browser like;

Oops, an error occured

This exception has been logged.

But there is no error logged in the console. I have found this issue to show up when import code, that I'm working on, doesn't compile cleanly. This would be a huge nice to have and would have saved me a bit of time tracking this down. It's mainly awkward because it is inconsistent with the workflow that revel encourages, and then provides no helpful information about what went wrong.

@robfig
Copy link
Contributor

robfig commented May 28, 2013

Hey Jeff,

I think that is actually an unrelated issue -- that unhelpful error page appears when mode.dev = false, as that controls revel.DevMode, which is the switch used in revel/templates/errors/500.html to decide whether or not to show the stack trace. Perhaps you have an old app.conf that lacks that, and it probably defaults to false. There should be no issue showing you build error in dependencies right now, it just won't re-compile if the app is running and there was no change to app code.

Of course, in any case it should be logging it to ERROR, although that may be going to a file instead of the console. (I am personally unhappy with the current logging situation and would love to replace it with something better.)

RE GOPATH-watching -- The other thing I would be concerned about when watching the whole GOPATH is that editing unrelated files would make Revel recompile the app needlessly.

I do see the argument that all of these reasons not to do it only apply after they have a larger codebase, and a sophisticated user could easily flip that switch to off if it was causing them problems. Perhaps this is really helpful for the new users, which we are trying to woo. I think I'm starting to come around...

@jgraham909
Copy link
Contributor

Doh! You're right I had fixed the app.conf dev issue but didn't push it and was working on a different machine. Sorry for the clutter.

This could also be addressed via documentation. eg. "If you are editing imported dependencies or modules you will need to edit files within your application for force a recompile." or do we also need a go clean of the appropriate import paths?

@robfig
Copy link
Contributor

robfig commented May 29, 2013

No "go clean" is necessary. Whenever I'm working on a dependency, I usually just switch back to an app file and re-save it to trigger a recompile.

@vanackere
Copy link
Contributor Author

Hi again,

Would you accept to merge this patch now that this option is disabled by default ?

gopaths := filepath.SplitList(build.Default.GOPATH)
paths = append(paths, gopaths...)
}
paths = append(paths, revel.CodePaths...)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's any need to add the CodePaths if you're already watching all of the gopaths

@robfig
Copy link
Contributor

robfig commented Jun 15, 2013

I suppose this doesn't hurt anything, and could be helpful. I'll try developing with it on and see how it goes.

@robfig
Copy link
Contributor

robfig commented Jul 22, 2013

This doesn't work on OSX -- even filtering down the directories further (ignoring any starting with dot, skipping the standard library), I run out of file descriptors less than halfway through my GOPATH. ulimit -a tells me I have a limit of 4864 open files.

It seems like a watch implementation based on FSEvents instead of kqueue could handle this, though..

@brendensoares
Copy link
Member

Help me out here guys, but I'm not seeing a good reason to add support for this. What's wrong with simply restarting the app when you update your packages? Even though file descriptor limits can be manually increased by the user, every fd occupies system memory that could be used by your app.

If we did add support for this feature, perhaps watch.gopath should be watch.dependencies since you're not really watching the entire $GOPATH so much as you're watching a few imported packages, right?

All that said, I would rather see a way to allow a Revel app to monitor custom paths as this would have real world usage like monitoring a directory for external events (e.g. emails, logs, uploads). This feature seems more like a convenience, but it doesn't really save any time as far as I can tell.

FWIW, I verified that a directory's modification timestamp changes when creating, renaming or deleting one of its files (on OS X Mavericks and likely any Unix-like OS). So we could reduce the fd count if we only watch directories (e.g. src/somepackage) instead of all files under a directory as long as we don't care about file changes. I hope I'm not way off base on that assumption (I've never used fsnotify or the like myself)

As this issue is a bit stale, I'm going to close it, but feel free to re-open it if you feel there is more to be said.

Thank you @vanackere for your contribution to the community.

@vanackere
Copy link
Contributor Author

2013/12/9 Brenden Soares notifications@github.com

Help me out here guys, but I'm not seeing a good reason to add support for
this.

To the contrary I do not see a good reason to not support this on systems
where this is working fine (so far, at least on Linux this is working fine
for me and quite a number of coworkers who really gain a lot of time with
this patch in their daily workflow).

What's wrong with simply restarting the app when you update your packages?

When you're updating only the packages in question this is quite annoying.
I do have a custom gopath containing several internal go packages (shared
with several revel & non-revel apps) and while editing those packages it is
nice to have the revel app automatically rebuilt. Moreover this is the
behaviour of "go build" so i'd argue this should also be the behaviour of
the revel watcher...

Even though file descriptor limits can be manually increased by the user,
every fd occupies system memory that could be used by your app.

In my opinion, unless you're developping on a raspberry Pi, this should
really have a minimal impact on your system (and if your system memory is
so important that it matters you should really not be using a framework
like revel anyway...).

If we did add support for this feature, perhaps watch.gopath should be
watch.dependencies since you're not really watching the entire $GOPATH so
much as you're watching a few imported packages, right?

All that said, I would rather see a way to allow a Revel app to monitor
custom paths as this would have real world usage like monitoring a
directory for external events (e.g. emails, logs, uploads). This feature
seems more like a convenience, but it doesn't really save any time as far
as I can tell.

FWIW, I verified that a directory's modification timestamp changes when
creating, renaming or deleting one of its files (on OS X Mavericks and
likely any Unix-like OS). So we could reduce the fd count if we only
watch directories (e.g. src/somepackage) instead of all files under a
directory as long as we don't care about file changes. I hope I'm not way
off base on that assumption (I've never used fsnotify or the like myself)

As this issue is a bit stale, I'm going to close it, but feel free to
re-open it if you feel there is more to be said.

AFAICT this patch is still working fine and it seems that the fix to have
the feature working on OS X should be made in the fsnotify package instead.
In the meantime this option is disabled by default so have no impact on
people that do not explicitely enable this feature...

Please reopen this issue (unless @robfig has decided that this is really a
bad idea, in which case I'll have to live with my private fork of revel for
the forseable future...).

@brendensoares
Copy link
Member

Ok, I can see how working on your own imported packages would required multiple rebuilds per day.

As far as your comment about memory, when you're building a web app that is intended to scale responsibly, you should be mindful of how you're using your limited resources. It's healthy to design with constraints, because it forces you to actually think about what you're doing.

Assuming we do pull your changes in, what are your thoughts on the naming of watch.gopath that I mentioned?

@robfig Any final remarks? I think @vanackere has made his case enough to include this PR, especially if it's off by default and not enabled in production (though we can leave that up to the user).

@brendensoares brendensoares reopened this Dec 9, 2013
@robfig
Copy link
Contributor

robfig commented Dec 14, 2013

I think the goal of rebuilding when any dependencies change (not just within the app) is a good one. However, this implementation:

  • Works only for linux
  • Watches the whole world, rather than figuring out the app's dependencies and watching only those.

which is why I was hoping we could develop a better solution. I suppose something is better than nothing though.

@brendensoares
Copy link
Member

I think this PR is a good first step, especially since it is disabled by default; it comes with a "buyers beware" YMMV warning in a sense.

We can always improve this later and look into ensuring cross platform support. I'll be merging into the develop branch soon.

Thanks @vanackere

@brendensoares
Copy link
Member

merged into develop.

@vanackere Any chance you could contribute some documentation?

@verdverm
Copy link
Contributor

#555 may help with the 'watch the whole world' issue
It's a slightly different attempt at watching external dependencies

I also ran into the OSX file descriptor limit today
the ulimit was set to 256 by default
upped it to 1024 and everything is ok again

brendensoares added a commit that referenced this pull request Jan 23, 2016
kumakichi pushed a commit to kumakichi/revel that referenced this pull request Nov 2, 2018
Updated tool to give more meaningful messages
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.

None yet

5 participants