-
Notifications
You must be signed in to change notification settings - Fork 391
create temp context correctly #12339
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
Conversation
| cleanup: () => { | ||
| context.diskCache.close(); | ||
| temp.cleanup(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cleanup was for singleFile render associated to const temp = globalTempContext();
So as this is already cleaned up globally, I didn't think we needed to do manual temp.cleanup in this case.
TBH, having those two different tempContext is for now not easy for me to reason about. I guess cleaning up in this finally step will not prevent or mess the last cleanup
-
Global cleanup register for
quarto-sessiondir in TEMP
Line 185 in 6164de8
onCleanup(cleanupSessionTempDir); -
Called in Exit cleanup
quarto-cli/src/core/cleanup.ts
Lines 11 to 26 in 6164de8
export function onCleanup(handler: VoidFunction) { cleanupHandlers.push(handler); } export function exitWithCleanup(code: number) { // Not using cleanupHandlers.reverse() to not mutate the original array for (let i = cleanupHandlers.length - 1; i >= 0; i--) { const handler = cleanupHandlers[i]; try { handler(); } catch (error) { info("Error occurred during cleanup: " + error); } } Deno.exit(code); }
I guess this will be unused code now if we do cleanup as part of context.cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, having those two different tempContext is for now not easy for me to reason about.
The old code is (demonstrably, see this PR!) hard for me to reason about too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And actually since this is also not a single-file render, I think we should replace globalTempContext() with a makeTempContext like we do in the other settings.
cderv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, to sum up, globalTempContext() is no longer used. Quarto will now store all temp files locally in .quarto.
For sure this is clearer to read as more scoped.
Still, I think we should watch out for the side effects of moving from the OS global TEMP folder to the project local folder. The period before the final release will be a good test!
💯 |
This is a followup fix to #12329.