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

Fix hot reloading for config.toml in zola serve on Linux #2498

Merged
merged 1 commit into from
May 24, 2024

Conversation

iamorphen
Copy link
Contributor

@iamorphen iamorphen commented May 16, 2024

Introduction

Fixes #2266 , alternative to #2269 which appears to be stalled and has a large diff against next. In particular, this fixes hot reloading for changes made to config.toml via the caching strategy used by editors such as vim on Linux. At the time of this writing, changes written to config.toml via at least vim and neovim on Linux are not noticed at all by zola serve.

Code changes

  1. Watch config.toml's containing folder--not config.toml directly--for changes. See Details for why.
  2. Each item to watch tracks whether to watch it recursively.
  3. Ignore changes to files peer to config.toml, assuming that all other files we care about are nested more deeply than the config file. This is the case at the time of this writing, and the Zola docs suggest that config.toml is the only top-level file we watch in a zola project.
  4. When announcing the files we're watching, replace the config file parent directory with the config file name to not confuse the end user, since we ignore all ordinary files peer to config.toml.

Details

Editors such as vi, vim, and neovim cache changes to swap files. When a user wants to write to the original file, such editors may delete the original file and replace it with the swap file. This changes the inode of the "original" file. The file-watching strategy on Linux is inotify which watches for changes via inodes, not file names. So when an editor like vim deletes the file we're watching, we lose event information on that file after the final delete event. If we instead watch the owning directory, we can observe events on all file changes and with some filtering pretend like we're watching the same config.toml file all along.

The reason this bug is not observed on macOS is because macOS uses FSEvents to watch for file system changes. FSEvents monitors on the directory level. The change here makes behavior on Linux approximate that of FSEvents.

zola uses Notify v4, and notify::watcher returns a RecommendedWatcher. The recommended watcher on macOS is FsEventWatcher whereas it is INotifyWatcher on Linux.

Testing

Due to where the ignore logic is done in serve.rs:serve, we can't really extend the tests in the same module to check this feature.

I have manually checked zola serve on Linux and macOS by editing config.toml and theme files and observing hot reloading. I also edited theme.toml at the top-level of a zola project, and such changes were correctly ignored.

I don't have a Windows platform to test on.

UI

Due to the watch list remapping, zola serve still announces the following to the end user (note config.toml even though we watch the parent directory).

Listening for changes in /foo/bar/{config.toml,content,static,templates}
Press Ctrl+C to stop

@iamorphen
Copy link
Contributor Author

CI shows a compilation error in notify on Windows. I can't personally debug that locally. It seems to be a notify-internal issue. I could try to bump the notify version zola uses and see if that fixes the problem. I can try that in a separate draft MR.

@Keats
Copy link
Collaborator

Keats commented May 16, 2024

For notify, there's #2077 and it looks like it's on v6 now. We will need to upgrade before the next release anyway so if you have time that would be great

@iamorphen iamorphen changed the title Fix hot reloading for config.toml in zola serve Fix hot reloading for config.toml in zola serve on Linux May 16, 2024
@iamorphen
Copy link
Contributor Author

My hacked up experiment with notify-debouncer-full in #2499 passed all CI. I'll see about making the implementation production-worthy.

@iamorphen
Copy link
Contributor Author

For posterity, the reason the Windows build issue popped up seems to be because libc in the CI run is 0.2.154 whereas other successful CI runs (ex) use 0.2.151. The versions of winapi and notify otherwise seem identical across recent CI runs. I found master to use 0.2.151, but something in next moved us to 0.2.154.

@iamorphen iamorphen force-pushed the feat/orphen/fix-config-hotload-linux branch from c866b8c to b2ce686 Compare May 24, 2024 01:37
@iamorphen
Copy link
Contributor Author

I rebased and re-tested locally on Linux. I'm just one data point, but all's working on my end.

@Keats
Copy link
Collaborator

Keats commented May 24, 2024

Works on mac as well

@Keats Keats merged commit 4b9bf84 into getzola:next May 24, 2024
5 checks passed
@Keats
Copy link
Collaborator

Keats commented May 24, 2024

Thanks!

@Raymi306 Raymi306 mentioned this pull request May 28, 2024
3 tasks
@iamorphen
Copy link
Contributor Author

Sorry, was away for a bit, just wanted to say thank you also for your time here!

@iamorphen iamorphen deleted the feat/orphen/fix-config-hotload-linux branch June 5, 2024 05:59
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

2 participants