-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Automatically cleanup old artifacts of file-based apps #49666
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
Adds support for automatically cleaning up stale file-based app build artifacts and exposes a new hidden clean-file-based-app-artifacts
command.
- Introduce and wire up a new
clean-file-based-app-artifacts
command in the CLI parser - Implement automatic cleanup logic in
VirtualProjectBuildingCommand
and a standaloneCleanFileBasedAppArtifactsCommand
- Add resource strings, translations, and documentation for the new command and behavior
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/Cli/dotnet/Parser.cs | Register new clean-file-based-app-artifacts command |
src/Cli/dotnet/Commands/Clean/FileBasedAppArtifacts/CleanFileBasedAppArtifactsCommandParser.cs | Define parser and options for the cleanup command |
src/Cli/dotnet/Commands/Clean/FileBasedAppArtifacts/CleanFileBasedAppArtifactsCommand.cs | Implement the cleanup operation, metadata tracking, and automatic background launch |
src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs | Add artifact‐usage touch, cleanup trigger, and refactor temp‐path helpers |
src/Cli/dotnet/Commands/Run/RunCommand.cs | Touch artifacts folder in dotnet run entry-point |
src/Cli/dotnet/Commands/Run/Api/RunApiCommand.cs | Touch artifacts folder in API run scenario |
src/Cli/dotnet/Commands/Clean/CleanCommand.cs | Rename NoBuildMarkers to NoWriteBuildMarkers for consistency |
src/Cli/dotnet/Commands/CliCommandStrings.resx | Add .NET resource entries for new cleanup command |
src/Cli/dotnet/Commands/xlf/* | Add translation placeholders for new cleanup strings |
documentation/general/dotnet-run-file.md | Document automatic cleanup cadence, toggle, and manual command |
Comments suppressed due to low confidence (1)
src/Cli/dotnet/Parser.cs:71
- Consider adding automated tests for the new
clean-file-based-app-artifacts
command and its automatic cleanup trigger to ensure the behavior remains correct over time.
CleanFileBasedAppArtifactsCommandParser.GetCommand(),
src/Cli/dotnet/Commands/Clean/FileBasedAppArtifacts/CleanFileBasedAppArtifactsCommand.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
They are not cleaned immediately because they can be re-used on subsequent runs for better performance. | ||
The automatic cleanup can be disabled by environment variable `DOTNET_CLI_DISABLE_FILE_BASED_APP_ARTIFACTS_AUTOMATIC_CLEANUP=true`, | ||
but other parameters of the automatic cleanup are currently not configurable. | ||
The same cleanup can be performed manually via command `dotnet clean-file-based-app-artifacts`. |
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.
Thoughts on making this an option (or sub-command?) to the existing dotnet clean
command instead? e.g. dotnet clean --file-based-app-artifacts
or dotnet clean caches
or some such @baronfel
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.
dotnet clean --file-based-apps
or clean-file-based-apps
seems good to me. The word artifacts in the context of clean seems redundant.
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 does dotnet clean --file-based-apps
sound good to everyone before I go and change to that?
FWIW, I chose to make this a separate command (currently hidden from dotnet --help
) because it's fundamentally different from dotnet clean
which uses MSBuild /t:Clean
whereas dotnet clean-file-based-app-artifacts
implements its own logic which also means no flags of the normal dotnet clean
apply. That being said, I'm okay with having this as a flag for dotnet clean
, but I think we might need to check there are no other flags passed.
As for dropping "artifacts" from the command name, dotnet clean-file-based-apps
sounds a bit like your file-based apps will be deleted (not just their artifacts) but on the other hand I also dislike how unwieldy my current command name is.
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.
I'd like to hear from @baronfel. I'm curious whether we could make this a sub-command instead, so something like dotnet clean file-based-apps
. We could still make it hidden but more importantly we'd need to ensure it's not breaking in any way.
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.
I'd very much love a hidden subcommand. It's kinda a gnarly name to have a dedicated option flag, and it's really an implementation detail that we just don't have a super-great home for.
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.
The problem with a subcommand is that it also inherits all arguments and options from the parent command. But perhaps that's acceptable given it's a hidden subcommand.
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.
Changes look good. Up to the group how this is exposed, as either a hidden command or hidden option to clean
or whichever works for y'all.
The automatic cleanup can be disabled by environment variable `DOTNET_CLI_DISABLE_FILE_BASED_APP_ARTIFACTS_AUTOMATIC_CLEANUP=true`, | ||
but other parameters of the automatic cleanup are currently not configurable. | ||
The same cleanup can be performed manually via command `dotnet clean-file-based-app-artifacts`. |
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.
@baronfel please take a look at these new entry point / env names.
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.
We've already discussed the subcommand in another thread above, but yeah, please also take a look at the env var name DOTNET_CLI_DISABLE_FILE_BASED_APP_ARTIFACTS_AUTOMATIC_CLEANUP
.
|
||
if (!directory.Exists) | ||
{ | ||
Reporter.Error.WriteLine(string.Format(CliCommandStrings.CleanFileBasedAppArtifactsDirectoryNotFound, directory.FullName).Yellow()); |
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.
Why report a warning here? Won't that mean that installing the CLI then running the command produces a warning? I intuitively expect it would just return nothing because there was nothing to delete and it successfully deleted nothing 😄
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.
My thinking is that if the directory doesn't exist, something might be wrong (e.g., you have some env var set which changes where the directory points to) and it might be good to alert the user.
} | ||
|
||
// Open and lock the metadata file to ensure we are the only automatic cleanup process. | ||
return GetMetadataFile().Open(FileMode.Create, FileAccess.ReadWrite, FileShare.None); |
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.
In the case of concurrent calls to this method there will be an exception generated here. How does that manifest in terms of user experience?
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 is only called during the automatic cleanup which runs in the background and its output is not visible anywhere, so the exception won't be seen.
Resolves #48250.