getpelican / pelican

Static site generator that supports Markdown and reST syntax. Powered by Python.
https://getpelican.com
GNU Affero General Public License v3.0
12.58k stars 1.81k forks source link

autoreload #286

Closed AndreaCrotti closed 12 years ago

AndreaCrotti commented 12 years ago

Looking at the autoreload code makes me think that it's not such a good idea:

    if args.autoreload:
        while True:
            try:
                # Check source dir for changed files ending with the given
                # extension in the settings. In the theme dir is no such
                # restriction; all files are recursively checked if they
                # have changed, no matter what extension the filenames
                # have.
                if files_changed(pelican.path, pelican.markup) or \
                        files_changed(pelican.theme, ['']):
                    pelican.run()
                time.sleep(.5)  # sleep to avoid cpu load

What if the process of checking and running takes more than half a second for example?

And is anyone really actually using this feature? If we want some real autoreloading for me it might be interesting to add optional support for http://home.gna.org/py-notify/

inotify is Linux-specific, but there are alternatives also for the other OS, and we could then avoid the naughtl time.sleep ;)

almet commented 12 years ago

I don't quite get what's your concern here. I will try to answer the questions in order:

AndreaCrotti commented 12 years ago

Yes right sorry I don't know why but I was thinking about multi-threads. There is only one thread so the waiting time is not really an issue..

The problem imho is just that it's probably not very scalable, for a big website it might kill your machine.. Anyway not a big deal, I think I'll try the inotify approach myself to see how it works, but I understand that you don't want to include OS-specific code..

almet commented 12 years ago

Okay, closing for now then.

josch commented 7 years ago

But would you accept a PR adding inotify support and using it on systems where it's available and falling back to the old methods on systems where it's not available?

The advantage of using inotify would be that instead of polling the filesystem every half second, the disk and the CPU could remain idle until something is actually changed in the filesystem. So using inotify would reduce the load on disk and CPU (and thus saves battery). Even though both loads are tiny, they are frequent. With the current situation I would not be comfortable of having pelican running for hours on my system while it's on battery.

cunhaax commented 7 years ago

The CPU usage introduced by pelican is relatively high, considering that we always have at least a browser and other applications open at the same time... I work with an i5 laptop and the cpu usage by pelican is very noticeable.

@josch I noticed that you have a forked repo with some changes, did you implement these changes using inotify? @almet can you consider, reopening this issue?

justinmayer commented 7 years ago

@cunhaax: Pull request #2176 has already been submitted by @josch, so there is no need to re-open this issue; any further discussion can and should occur there.

If you would like to test @josch's work and provide feedback via comments on his pull request, that would be helpful and much appreciated.

josch commented 7 years ago

@cunhaax don't mix up my pull request #2176 with this issue. They are separate topics.

I did not work on implementing autoreload using inotify yet. I will only start preparing a patch implementing that feature if it has the blessing of the pelican devs.

justinmayer commented 7 years ago

@josch: Quite right. I think your concept has merit and would think that a PR that added inotify support would be warmly received. Do you need any further guidance?

josch commented 7 years ago

@justinmayer: Awesome! Then I'll look into it soon. :)

We'll talk details in the PR I'll open about it. ^^