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

When spin watch terminates spin up, allow it to delete its temp dir #1723

Merged

Conversation

itowlson
Copy link
Contributor

Fixes #1698.

A previous crack at this in #1711 ran aground on sychronising the spin build and spin up processes when spin up was terminating asynchronously. I couldn't get the watch state to remain in sync with what watchexec thought it was doing, resulting in either missed file changes or missed restarts.

This is, I'm afraid, a more dramatic rework, and therefore carries a higher risk - the more so because, unlike the original implementation, it does not abstract notifications or defunctionalise process control, and so it way less amenable to unit testing. I'm not sure how to address that - I'd welcome ideas. On the plus side, the individual components at work here are rather more simple-minded, so hopefully less in need of unit testing once the inevitable initial bugs have been shaken out.

The change in strategy is twofold:

  1. Instead of a single watchexec watching all interesting files, and patching up "what command is watchexec running" depending on what files have changed, this now uses two separate watchexecs, one watching artifact files, one watching build files.
  2. These watchexecs do not manage processes themselves. Instead, they send notifications to two processor tasks, the buildifier and uppificator. (Okay, yes. I should probably change the names.) These spawn and restart processes according to the notifications they receive, and coordinate so that changes triggered by a build do not start spin up until the build is complete.

This means that the processor tasks are much less dependent on state than the previous implementation - they are pretty much simple loops going round and round waiting for the change notifications they're interested in and starting or restarting a child process. (The only piece of state that needs to be passed between notifications is "don't clear the screen because build just did".)

This has so far been tested only on a small Rust application. Next step is to try it on something like the docs site, where the build process makes much more far-reaching changes to the artifacts. It should also adopt some of the "stupid operating system does what" patches from the previous implementation!

In the meantime I'm putting this out for folks to kick the tyres and yell at the design/implementation. @calebschoepp needless to say, I would particularly value your experience and insight on the practicalities as well as the design!

@itowlson
Copy link
Contributor Author

itowlson commented Aug 31, 2023

Well there's a bug where the docs site makes it go into an infinite loop so we're off to a flying start.

ETA: It looks like what is happening is that a component build.watch-es a bunch of directories in order to rebuild an index that is inside one of those directories. So when the build happens, it modifies the index, which triggers a new build. I think this would have fooled the old spin watch too so it may not be a regression, but maybe I need to handle it better all the same... (In the docs site's case I believe the answer is that it doesn't need to build.watch the output directory because the build command doesn't take any input from that directory. And when I do that, the proposed spin watch works okay.)

@calebschoepp
Copy link
Collaborator

@itowlson I'll take a look at this when I get a chance, but I won't have time today. Thanks for diving so deep on spin watch.

Copy link
Collaborator

@calebschoepp calebschoepp left a comment

Choose a reason for hiding this comment

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

Great work @itowlson! I think we're headed in the right direction

Cargo.toml Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/commands/watch.rs Outdated Show resolved Hide resolved
src/commands/watch.rs Show resolved Hide resolved
src/commands/watch.rs Outdated Show resolved Hide resolved
src/commands/watch/buildifier.rs Show resolved Hide resolved
src/commands/watch/uppificator.rs Show resolved Hide resolved
src/commands/watch/uppificator.rs Show resolved Hide resolved
src/commands/watch/uppificator.rs Show resolved Hide resolved
src/commands/watch/uppificator.rs Show resolved Hide resolved
src/commands/watch.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@calebschoepp calebschoepp left a comment

Choose a reason for hiding this comment

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

I responded to your comments and took another pass over your changes. I think we're getting close. The only stuff that I really feel is still blocking is the ignore list work.

@itowlson itowlson force-pushed the watch-tidy-temp-dirs-by-hook-or-by-crook branch from de232b5 to f588c1c Compare September 18, 2023 20:27
@itowlson
Copy link
Contributor Author

@calebschoepp I think this addresses everything. I have left the commit history intact for now so you can see changes since first review if that's useful; if approved, I'll squash before merging (well if I remember anyway).

Copy link
Collaborator

@calebschoepp calebschoepp left a comment

Choose a reason for hiding this comment

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

Great work @itowlson , LGTM! Ship it 🚢

@itowlson
Copy link
Contributor Author

@calebschoepp I implemented your fix for the Mac source notification issue. It Works On My Machine(s) TM, but if you have time to test it on yours, that would greatly increase my confidence. Thanks!

@calebschoepp
Copy link
Collaborator

@calebschoepp I implemented your fix for the Mac source notification issue. It Works On My Machine(s) TM, but if you have time to test it on yours, that would greatly increase my confidence. Thanks!

I built latest on this branch and we no longer get the infinite looping behaviour. I'm seeing one infinite loop bug persist though. I sent you a video of it in Slack.

Signed-off-by: itowlson <ivan.towlson@fermyon.com>
@itowlson itowlson force-pushed the watch-tidy-temp-dirs-by-hook-or-by-crook branch from 39085d4 to cfa75c4 Compare September 19, 2023 01:35
@itowlson itowlson merged commit c4b9f65 into fermyon:main Sep 19, 2023
9 checks passed
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.

What is the recommended solution to clear spin-related cache files in the /tmp directory?
2 participants