Skip to content

Use global temp directory for run file outputs #48169

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 8, 2025
Merged

Use global temp directory for run file outputs #48169

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
46bd143
Use global temp directory for run file outputs
jjonescz Apr 4, 2025
cb2dfbb
Improve artifacts path computation
jjonescz Apr 5, 2025
c28a00a
Include file name
jjonescz Apr 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 35 additions & 3 deletions src/Cli/dotnet/Commands/Run/VirtualProjectBuildingCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,12 @@ ProjectRootElement CreateProjectRootElement(ProjectCollection projectCollection)

var projectFileFullPath = Path.ChangeExtension(EntryPointFileFullPath, ".csproj");
var projectFileWriter = new StringWriter();
WriteProjectFile(projectFileWriter, _directives, isVirtualProject: true, targetFilePath: _targetFilePath);
WriteProjectFile(
projectFileWriter,
_directives,
isVirtualProject: true,
targetFilePath: _targetFilePath,
artifactsPath: GetArtifactsPath(EntryPointFileFullPath));
var projectFileText = projectFileWriter.ToString();

using var reader = new StringReader(projectFileText);
Expand All @@ -192,14 +197,34 @@ ProjectRootElement CreateProjectRootElement(ProjectCollection projectCollection)
projectRoot.FullPath = projectFileFullPath;
return projectRoot;
}

static string GetArtifactsPath(string entryPointFilePath)
{
// We want a location where permissions are expected to be restricted to the current user.
string directory = RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
? Path.GetTempPath()
: Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData);
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to think about cleanup routines for when we're not on Windows using temp? Perhaps it could be a specific option to the run command so that every N runs we do a fire & forget process launch of e.g. dotnet run --clean-run-cache so that it doesn't run inside the process the user invoked.

Copy link
Member

Choose a reason for hiding this comment

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

I thought we discussed the compiler server itself as a model for this, there is a temp subdirectory for each named pipe or some such, and the server will clean up the unused ones on startup.

Copy link
Member Author

@jjonescz jjonescz Apr 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Cleaning this automatically would hurt perf because the build artifacts are used for incremental builds (and will be even more when we have caching). But I'd say dotnet clean could be updated to clean up this location.

Copy link
Member

@DamianEdwards DamianEdwards Apr 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That challenge with using dotnet clean is that it's specific project-centric (you run it on a specific project/solution), whereas in this case I think we'd want more of a cache-centric approach where it cleans up all items using an LRU approach.

But agreed on the point of not wanting to impact run performance at all. The scenario is quite different to that of starting a long-running server.

Copy link
Member

@RikkiGibson RikkiGibson Apr 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's similar to the compiler server in that we likely don't have a point where user will realistically run a general clean-fbp tool or point clean at particular file-based program(s) whose artifacts should be cleaned up. Cleaning on startup (not necessarily every or most runs of dotnet run app.cs) feels plausible to me, whether it is done by leaving the N most recent builds in place or builds which are less than a week or a month old or some other basis.

Copy link
Member

Choose a reason for hiding this comment

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

I think we're saying similar things, just that I think we'd want to do it in a way that doesn't impact the run time of an individual invocation, hence my original comment about making it a fire and forget launch of the clean operation on every N runs.

Copy link
Member Author

@jjonescz jjonescz Apr 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Makes sense, but this is something we want to do in a separate PR, or no?

Copy link
Member

Choose a reason for hiding this comment

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

Clean can come later

Copy link
Member

Choose a reason for hiding this comment

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

let's get an issue logged

Copy link
Member Author

Choose a reason for hiding this comment

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

#48250


// Include entry point file name so the directory name is not completely opaque.
string fileName = Path.GetFileNameWithoutExtension(entryPointFilePath);
string hash = Sha256Hasher.HashWithNormalizedCasing(entryPointFilePath);
string directoryName = $"{fileName}-{hash}";

return Path.Join(directory, "dotnet", "runfile", directoryName);
}
}

public static void WriteProjectFile(TextWriter writer, ImmutableArray<CSharpDirective> directives)
{
WriteProjectFile(writer, directives, isVirtualProject: false, targetFilePath: null);
WriteProjectFile(writer, directives, isVirtualProject: false, targetFilePath: null, artifactsPath: null);
}

private static void WriteProjectFile(TextWriter writer, ImmutableArray<CSharpDirective> directives, bool isVirtualProject, string? targetFilePath)
private static void WriteProjectFile(
TextWriter writer,
ImmutableArray<CSharpDirective> directives,
bool isVirtualProject,
string? targetFilePath,
string? artifactsPath)
{
int processedDirectives = 0;

Expand All @@ -217,9 +242,16 @@ private static void WriteProjectFile(TextWriter writer, ImmutableArray<CSharpDir

if (isVirtualProject)
{
Debug.Assert(!string.IsNullOrWhiteSpace(artifactsPath));

writer.WriteLine($"""
<Project>

<PropertyGroup>
<IncludeProjectNameInArtifactsPaths>false</IncludeProjectNameInArtifactsPaths>
<ArtifactsPath>{EscapeValue(artifactsPath)}</ArtifactsPath>
</PropertyGroup>

<!-- We need to explicitly import Sdk props/targets so we can override the targets below. -->
<Import Project="Sdk.props" Sdk="{EscapeValue(sdkValue)}" />
""");
Expand Down