Changing the model file triggers multiple rebuilds about fornjot HOT 12 CLOSED

Thank you for looking into this!

But I'm not sure if this is really the best idea. The file watch event handling is OS dependent which makes the event filter a bit messy with the multiple different event types to react to. Maybe it would easier and safer to let the user trigger the reload manually e.g. by pressing R or CTRL+R ? This would also avoid accidentally triggering the rebuild of a model when saving an intermediate state of the model file.

I think manually reloading the model would be fine as a stop-gap solution, but long-term, it's not the user experience I would like. First, it's a manual step that you'd basically always want to take anyway, at least for the typical use case. Second, I plan to rely on an external editor/IDE indefinitely, so it would even be a two-step process (switch to Fornjot window, trigger reload).

It's true that the watch event handling code is a bit messy, but the commit you linked doesn't really seem to add to that. It just adds a check in a single place in the code. If you submitted that as a pull request, I'd be happy to merge that, mostly as-is.

In addition to figuring out the multiple rebuilds, I think it would be great to take the following measures:

1. Clean up the platform-specific code. My idea is to have a platform module with a Platform trait (and one implementation per supported platform) that provides an API to platform-specific functionality (like a should_trigger_rebuild(&self, event: ...) -> bool method for this case). I've made a note and will open an issue once I find a bit of time. (edit: I've opened #36)
2. Add an option to disable automatic rebuilds. Again, I've made a note and will open an issue soon-ish. (edit: I 've opened #37)

from fornjot.

I've opened #36 and #37 as a follow-up to my previous comment here.

from fornjot.

Adding the https://github.com/hannobraun/Fornjot/labels/good%20first%20issue label, since this is a fairly isolated problem that doesn't affect a lot of code. As of this writing, all the relevant code is located in main.rs. I think it would make sense to move it to model.rs though, but this doesn't need to be a part of this fix.

from fornjot.

The watcher is watching the complete 'mode/{model_name}/src' subfolder for changes. Events will be triggered for lib.rs but also for temporary and hidden files (e.g. Vim swap files during editing or intermediate compilation files). Would it make sense to restrict the watcher to just changes to model/{model_name}/src/lib.rs or only *.rs files?

from fornjot.

Thank you, for the suggestions, @mxdamien!

I'm not sure what solution would be best. The intention is that a models can written in Rust, not just some restricted dialect. So watching just lib.rs would be too restrictive, as a non-trivial model could easily consist of multiple modules. Watching for all *.rs files would be better, but someone could include some non-Rust data file in their model (through something like include_str!/include_bytes!).

Maybe I'm being too paranoid about this last one, but I've experienced time and time again that software that's trying to be clever without being clever enough will become a huge frustration at some point.

How about this idea:

• Watch all the Cargo files that could influence a build (Cargo.toml, Cargo.lock, build.rs, .cargo/config, .cargo/config.toml are the ones I can think of off the top of my head).
• In addition, watch everything under src/.
• Filter out events according to some built-in blacklist. That would include things like Vim swap files.

Would that work, or am I missing something? Maybe it would be better to leave out that first point, just watch everything and have a blacklist.

from fornjot.

Watching everything under src/ and blacklisting file extensions has the desired effect. The rebuild/reload will only be triggered once.

mxdamien@c857ca7

But I'm not sure if this is really the best idea. The file watch event handling is OS dependent which makes the event filter a bit messy with the multiple different event types to react to. Maybe it would easier and safer to let the user trigger the reload manually e.g. by pressing R or CTRL+R ? This would also avoid accidentally triggering the rebuild of a model when saving an intermediate state of the model file.

from fornjot.

It's true that the watch event handling code is a bit messy, but the commit you linked doesn't really seem to add to that. It just adds a check in a single place in the code. If you submitted that as a pull request, I'd be happy to merge that, mostly as-is.

All right.
I played around with different file editors a bit more. I found that one editor I tried (VSCode) still triggers multiple rebuilds. It seems like it really modifies the file multiple times (notify::event::DataChange::Content triggers multiple times for lib.rs). The other editors I tried (vim, nano, gedit) now only trigger the rebuild once and only when the file is saved and not during editing.

I cleaned up the commit a bit and here is the PR:

#39

from fornjot.

Thank you for looking into this, @mxdamien, I appreciate it!

I found that one editor I tried (VSCode) still triggers multiple rebuilds.

Interesting. I'm using VS Code, so that's probably what I've been seeing in the first place.

I'm wondering if we should even do anything about it. We could start tracking if something changed since the last rebuild, but that seems overkill, in addition to duplicating what Cargo already does. A more low-tech approach would be to delay the rebuild by some short time (50ms, or something along those lines) and ignore any events while a rebuild is scheduled anyway. That would have the disadvantage of adding to the compile time delay.

Probably best to just leave it for now and see if it turns into an actual problem.

from fornjot.

I found that one editor I tried (VSCode) still triggers multiple rebuilds.

The 2 file changes with VS Code might be due to it saving the file first, and auto-formatting.

@hannobraun Are you using the rust-analyzer or the Rust extensions?

from fornjot.

The 2 file changes with VS Code might be due to it saving the file first, and auto-formatting.

Makes sense. That could be it.

Are you using the rust-analyzer or the Rust extensions?

I'm using rust-analyzer. The "Rust" extension has been inferior to rust-analyzer for a long time now, and my understanding is that rust-analyzer is going to replace it as the official Rust extension.

from fornjot.

Removing https://github.com/hannobraun/Fornjot/labels/good%20first%20issue label. It's not clear at all whether this is an issue that we can or should solve.

from fornjot.

I've decided to close this. It's unclear whether we can or should do anything here.

If you run into a problem related to this, please comment here, or open a new issue.

from fornjot.