On windows there can be problems with Tools that do not support Unicode

[hknielsen]

Having a Username containing unicode chars, and tools that does not handle that well, makes it difficult when ToolTask are defined from NuGet Packages.

Fixes #9229

Context

Tmp folder can be changed by updating the User env vars, but Username are not possible. Not prepending the Username makes it possible for Users to handle these rare cases.

Changes Made

Changed to not append Username on Windows to tmp rsp file location.

Testing

Tests already for ToolTasks using this.

Notes

[KalleOlaviNiemitalo]

On Linux, you have to include the user name or something equally unique. Otherwise, MSBuild processes of a different user could attempt to use the same directory in tmp and fail because they don't have access to the directory.

Omit the user name only on Windows where the Unicode problem occurs.

On macOS, I think it could be implemented either way, but I'd prefer keeping the current behavior if no actual problem has been reported with it.

[hknielsen]

@KalleOlaviNiemitalo is it Runtime check for windows are used on MsBuild or do we have preprocessors for it?

[KalleOlaviNiemitalo]

There is a NativeMethodsShared.IsLinux check right below the code you changed.

msbuild/src/Shared/TempFileUtilities.cs

Line 41 in 1b84c9b

if (NativeMethodsShared.IsLinux && NativeMethodsShared.mkdir(basePath, userRWX) != 0)

msbuild/src/Tasks/AssemblyInfo.cs

Line 4 in 1b84c9b

global using NativeMethodsShared = Microsoft.Build.Framework.NativeMethods;

I expect that Windows would be checked in a similar way. There is already a property for that.

msbuild/src/Framework/NativeMethods.cs

Line 702 in 1b84c9b

internal static bool IsWindows

[hknielsen]

@KalleOlaviNiemitalo pushed new change, thanks!
Now we just need to determine if we should use the SecurityIdentifier - not sure how important it is

[rainersigwald]

I think this is a reasonable approach, and we'll check with security folks. Another option would be "on Windows, coerce the username to ANSI".

That said, I think there's room for #9229 to be fixed in ToolTask itself too, since the "change TEMP then call task" dance is pretty awkward.

[hknielsen]

I think this is a reasonable approach, and we'll check with security folks. Another option would be "on Windows, coerce the username to ANSI".

Sound good, let me know. we can do the conversion to ANSI or use SecurityIdentifier depending on that.

That said, I think there's room for #9229 to be fixed in ToolTask itself too, since the "change TEMP then call task" dance is pretty awkward.

I agree :) Lets discuss possible solutions on that Issue.

[Forgind]

Looks good to me! Approval is pending security review as rainersigwald mentioned.

[KalleOlaviNiemitalo]

The following do not mention "MSBuildTemp" and thus would not need to be updated for this change:

• Any files in https://github.com/MicrosoftDocs/visualstudio-docs/
• XML documentation comments in https://github.com/dotnet/msbuild/

[JanKrivanek]

LGTM