Stop using the default temp directory and temp file paths

[KirillOsenkov]

Using Path.GetTempPath() and GetTempFileName() is an anti-pattern. We currently use paths like these: C:\Users\VssAdministrator\AppData\Local\Temp\ntg31ahj.fhf\tmp71A.tmp and the problem is the default algorithm only allows 65,536 temp files. If we overflow, we start failing to create a temp file. Users run into this periodically when their temp directory is full. These errors are usually hard to diagnose (why did this temp file failed to get created??)

It is considered a good practice to create a folder "MSBuild" under Temp, and only write everything under that folder, so that people know who owns these files. Ideally of course the OS would provide a dedicated isolated temp folder for each app.

[benvillalobos]

There are roughly ~400 instances of Path.GetTempPath() in our codebase today.

1. Do we expect all of them to be replaced with TempFileUtilities.GetTemporaryDirectory()? My assumption is yes, so...
2. Do you know of a nice "mass find and replace" tool? 🙂

[KirillOsenkov]

Not sure of a tool but it sure would make my heart sing to have all these replaced. Having our own method instead of the framework one means we have the flexibility to inject logging, modify implementation, change it for tests, etc.

[MeikTranel]

Does this one actually need design or just a brave soul willing to do the deed? Because the change should transparently just happen within testing code i assume? Usage of Path.GetTempPath() in non-testing code is pretty uncommon and we still have the option leave usage in user facing code untouched:

If so hit me up i need some punishment werk to do for mindlessly doing things without actually researching what i'm doing (don't ask :D).

[danmoseley]

I'm not on the team but I don't think this needs design. I'm also guessing that tests could be done as a separate change.

BTW Path.GetTempFileName() no longer has the 65K issue from .NET 8 on. The change is still worthwhile for down level plus cleanliness.

https://github.com/dotnet/runtime/pull/74855

[KalleOlaviNiemitalo]

On Linux, using a fixed-name MSBuild subdirectory in $TMPDIR seems likely to mess up other users or introduce a security vulnerability.

On Windows and AFAIK macOS, the temp directory is per user, making this less of a problem. Although I don't know how appcontainers or other sandboxes work with that.

[MeikTranel]

Just to be clear - i sense that the motivations are split here - i think @KirillOsenkov is talking about actual user facing usage of temp paths, while @benvillalobos' usage list highlights primarily usage within test case execution within this repository. I was offering to do the latter and get the messy search and replace work out of the way while y'all can figure out what guidance is right for tasks and engine usage of temp paths.

BTW Path.GetTempFileName() no longer has the 65K issue from .NET 8 on. The change is still worthwhile for down level plus cleanliness.

@danmoseley don't forget we're still multitargeting msbuild for netfx

[rainersigwald]

After 7f3a30cd04aa884699b7980a9209aade28f1eca2, we shouldn't have any more uses of temp in product codepaths (that aren't critical, like the passthrough to allow get-temp-path through a property function).

For test code, we have TestEnvironment.GetTempFile() and related methods--for code that has been updated to use the "new" (it's 5+ years old) TestEnvironment. If you wanted mindless work, updating tests to use TestEnvironment (and dispose it) would certainly qualify . . .

[danmoseley]

@danmoseley don't forget we're still multitargeting msbuild for netfx

Right, that's what I meant by downlevel. I just meant - the future is improved. Although, it's probably still better to have your own subfolder.