-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add the condition to redirect debugPath to temp folder to avoid infinite build loops with infinite log file creation #12540
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
…ite build loops with infinite log file creation
…p-with-infinity-log-files
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 need to discuss what can be the good fix in this case. MSBuild temp directory is not a very good place for the logs.
else if (!string.IsNullOrWhiteSpace(debugDirectory) && IsPathInSolutionDirectory(debugDirectory)) | ||
{ | ||
// Redirect to temp to avoid infinite build loops in Visual Studio | ||
debugDirectory = Path.Combine(FileUtilities.TempFileDirectory, "MSBuild_Logs"); |
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.
While we have a fallback behavior on line 52 below that place MSBuild_Logs
to the temp folder, it is not actually desired behavior. We should rather remove that fallback below and add the graceful failure when we hit that case. For the debugDirectory
bug, we should think of another approach for dealing with it than silently falling back to using temp directory.
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 think it should not fail the build. My preferred solution would be to log a message (in this codepath we don't have logger access though, so probably just console write) that it's redirected to temp and proceed.
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.
Though when we soon plan to remove temp on endbuild we need a directory somewhere like string home = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile);
to persist this kind of debug info
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.
filed #12543
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.
@rainersigwald do you think it's acceptable to (1) use the redirection to temp+log to address the bug and defer resolving where the location for redirected logs should be? Or (2) draft this PR since the codepath will need to be adjusted and resume when we have a better place for redirected logs? (3) fail the build with nice message?
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 don't really like this approach. Can we instead talk to the VS project system folks about avoiding rebuilds when .binlog
is added? Or add a default exclusion at the SDK layer?
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.
it's not only binlog, but also nodecomm and scheduler logs 🤔 and those are .txt
. I was also worried about perf of checking suffixes of all files in projectsystem, that's why I suggested doing it this way. By sdk layer you mean to put an item exclusion in VS CommonTargets?
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.
Original author of the issue here: It seems silly/inefficient for VS to trigger a build when adding any file to the VS solution folder tree that has nothing to do with the build whatsoever. I could be building an application with a large embedded folder of assets such as media files, and I could be editing them in a separate application. As a VS user, I shouldn't have to remember to close Visual Studio before I edit the files in the solution folder tree. Or remember to place my assets outside the VS solution folder tree for VS to work correctly.
Each VS project already holds a list of input files included in a build (either explicit or a glob with include/exclude filters), plus a few special files that might affect the build, such as .editconfig
files or .config
files, that could be listed explicitly and that usually reside at the top level Solution/Project folder. Why can't the VS file watchers simply consult that list when a new file is added to some nested subfolder, before triggering a build? And if it's too difficult to be done, frankly as a user, I'd rather manually press the build button in VS on the rare occasion when the build is stale due to externally modified files, rather than deal with an avalanche of unwanted builds that slow down my computer that I didn't ask 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.
Why can't the VS file watchers simply consult that list when a new file is added to some nested subfolder, before triggering a build?
They do--the crux of the problem here is that these files fall into a default SDK glob for "miscellaneous stuff in the project folder".
else if (!string.IsNullOrWhiteSpace(debugDirectory) && IsPathInSolutionDirectory(debugDirectory)) | ||
{ | ||
// Redirect to temp to avoid infinite build loops in Visual Studio | ||
debugDirectory = Path.Combine(FileUtilities.TempFileDirectory, "MSBuild_Logs"); |
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 think it should not fail the build. My preferred solution would be to log a message (in this codepath we don't have logger access though, so probably just console write) that it's redirected to temp and proceed.
string resolvedPath = Path.GetFullPath(debugPath).TrimEnd(Path.DirectorySeparatorChar); | ||
string currentDir = Path.GetFullPath(Directory.GetCurrentDirectory()).TrimEnd(Path.DirectorySeparatorChar); | ||
|
||
// On macOS, when current directory is in temp folder, Path.GetFullPath() |
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 MacOS temp path logic seems like overengineering, let's remove it. On MacOS you don't even have VS which causes the bug the pr is supposed to address.
{ | ||
// Debug directory is writable; no need for fallbacks | ||
} | ||
else if (!string.IsNullOrWhiteSpace(debugDirectory) && IsPathInSolutionDirectory(debugDirectory)) |
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 remove this else if branch and add the && !IsPathInSolutionDirectory(debugDirectory)
also to the next else if
so that the code is less duplicated and the semantics is clear: "the specified directories are not available, so we go to temp"
else if (!string.IsNullOrWhiteSpace(debugDirectory) && IsPathInSolutionDirectory(debugDirectory)) | ||
{ | ||
// Redirect to temp to avoid infinite build loops in Visual Studio | ||
debugDirectory = Path.Combine(FileUtilities.TempFileDirectory, "MSBuild_Logs"); |
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.
Though when we soon plan to remove temp on endbuild we need a directory somewhere like string home = Environment.GetFolderPath(Environment.SpecialFolder.UserProfile);
to persist this kind of debug info
else if (!string.IsNullOrWhiteSpace(debugDirectory) && IsPathInSolutionDirectory(debugDirectory)) | ||
{ | ||
// Redirect to temp to avoid infinite build loops in Visual Studio | ||
debugDirectory = Path.Combine(FileUtilities.TempFileDirectory, "MSBuild_Logs"); |
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 don't really like this approach. Can we instead talk to the VS project system folks about avoiding rebuilds when .binlog
is added? Or add a default exclusion at the SDK layer?
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 it's included in sdk here (thanks @huulinhnguyen-dev for spotting that):
https://github.com/dotnet/sdk/blob/2ca3933a3441fd0dc35e39bd1ff9b7b19e2dbe60/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.DefaultItems.props#L43
and there is this handy exclude glob for folders starting with .
we could piggyback on:
https://github.com/dotnet/sdk/blob/2ca3933a3441fd0dc35e39bd1ff9b7b19e2dbe60/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.DefaultItems.targets#L51
do you see any risk with renaming the folder to .MSBuild_Logs
? @rainersigwald that would be a minimal change that I think resolves it.
Fixes #
Fix the issue: Infinite build loop with infinite log file creation when MSBUILDDEBUGPATH is set to a relative path #12375
Context
MSBuild was experiencing infinite build loops with infinite log file creation when debug logging was enabled with paths pointing to the solution directory. This occurred because Visual Studio would detect new log files in the solution directory and trigger rebuilds, creating a cycle of builds and log generation
Changes Made
automatically redirect it to a temporary folder (MSBuild_Logs in temp directory) to prevent infinite build loops
within the current solution directory
directory and redirect appropriately
Testing
solution directory
directory
not redirected
Notes