Fix exclusions in build.watch getting applied to all components
#2554
Conversation
src/commands/watch/filters.rs
Outdated
| struct CompositeFilterer { | ||
| filterers: Vec<Box<dyn watchexec::filter::Filterer>>, | ||
| } | ||
|
|
||
| impl watchexec::filter::Filterer for CompositeFilterer { | ||
| fn check_event( | ||
| &self, | ||
| event: &watchexec::event::Event, | ||
| priority: watchexec::event::Priority, | ||
| ) -> Result<bool, watchexec::error::RuntimeError> { | ||
| for f in &self.filterers { | ||
| if f.check_event(event, priority)? { | ||
| return Ok(true); | ||
| } | ||
| } | ||
| Ok(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: A comment explaining that any(event) triggers this filter rather than all(event) might be nice.
|
|
||
| #[derive(Debug)] | ||
| struct CompositeFilterer { | ||
| filterers: Vec<Box<dyn watchexec::filter::Filterer>>, |
There was a problem hiding this comment.
Nit: Perhaps put a Vec<Box<dyn watchexec::filter::Filterer>> behind a type alias? Not sure if that is cleaner so I'll leave that up to you.
There was a problem hiding this comment.
Hmm, to my mind it doesn't occur enough, or have a distinct semantic concept other than "vector of filterers", to merit the indirection. I agree Rust's insistence on box-dyn ceremony makes it harder to read than it needs to be though. If we want to do this I think I'd alias the box-dyn and make this Vec<AnyFilterer>...
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
itowlson
force-pushed
the
watch-negation-madness
branch
from
9c91f87
to
9dc66e4
Compare
There was a problem hiding this comment.
Thanks a lot, @itowlson, for the quick rescue against the negation madness! :D
I tested the fix with multiple components and tried to exclude files included under one component in another and it still worked.
Can we assume this PR adds the feature requested in #1692 as well? It seems to exclude files under the same component with a ! operator
|
@me-diru Once this merges we can flag it on #1692 for folks to test. The reason I am not saying "yes" is that the watchexec exclusion syntax feels a bit like we lucked into whatever watchexec does rather than designing it to meet our needs, so I would rather have the folks who want the feature to confirm that it really does work the way they want it! |
This tidies some issues arising from the revelation that
spin watchunderstands!syntax for exclusions after all. Thank you @me-diru for finding these little nasties!The first issue is that an exclusion in one component affects all other components. This happens because we combine all component globs into one list so that we can run one instance of
watchexec, so any exclusions that get chucked into that list and apply list-wide.The second issue is that adding an exclusion for
spin.tomlmeans that components no longer rebuild on manifest changes (e.g. to bebuild.command). Well, okay, you did ask for the bad thing. Because of the first issue this affects all components which is a bad thing you didn't ask for.The fix in this PR is, instead of combining the globs, to create a composite filterer which treats a change as triggering if any component expresses an interest in it. A component can still use bang syntax to exclude a file, but if other components are interested in that same file then changes to that file will still cause a rebuild. There is still only one instance of
watchexec, but the filterer for the buildifier is now built out of per-component filterers, plus a separate filterer for the manifest (so that it doesn't need to be spliced into individual components, and therefore can't be banged away).The composite filterer is used only for the build patterns, because assets are derived from the actual file mounts, and therefore don't have the same problem of user-provided pattern syntax.