[Bug]: TaskItem normalises path to build platform, not target platform

[erikbra]

Issue Description

I've been working a bit on a bug in the dotnet/sdk#559, and stumbled upon something that might be considered a bug in either Microsoft.Build.Utilities.TaskItem, or Microsoft.Build.Shared.FileUtilities.FixFilePath. One might also argue that it's not a bug, and that it's the usage of it in Microsoft.NET.Build.Containers.Tasks.CreateNewImage that is wrong.

Anyway, the problem is, when publishing dotnet projects to containers, some properties are set on the CreateNewImage MSBuild task, that are needed runtime in the built container. So, the paths need to be compatible with the container target platform, not the platform it's built on. The properties in question are (at least) Entrypoint, EntrypointArgs, AppCommand, AppCommandArgs.

However, as the properties are of type ITaskItem, implemented by TaskItem, the paths are normalised when initialised. And, the normalisation uses the Path.DirectorySeparatorChar of the build platform, not the target.

Would you consider this a bug/shortcoming in the TaskItem, or are we/they using it in the wrong way in the MSBuild task?

Steps to Reproduce

Use a TaskItem in an MSBuild task, where you need the directory separator char of a different platform than the one you are building on (e.g. use C:\app\foo.exe when building on *nix)

Expected Behavior

The item should stay as C:\app\foo.exe

Actual Behavior

The path is changed to C:/app/foo.exe

Analysis

msbuild/src/Utilities/TaskItem.cs

Line 80 in 8632094

_itemSpec = FileUtilities.FixFilePath(itemSpec);

There is no way to tell either: 1) That you don't want normalisation of directory separator characters, or 2) which style of normalisation you want

Versions & Configurations

dotnet msbuild --version
17.11.0.25706

[rainersigwald]

Oh that's a very interesting failure mode for this. I'd say it's an MSBuild bug, but it's a hard one to fix, so we might need to figure out how to work around for the SDK.

[erikbra]

Thanks for recognizing it as a bug 😄

It's probably an edge case, as almost every property that contains a path should be correct on the build server. However, there might be some others that would make sense to resolve on the target platform as well. Maybe dotnet tools have the same issue? But, I'm not sure.

We'll have to think a bit on how to solve this. I made a first attempt in my fork of the SDK repo at "fixing" it by swapping out the ITaskItems with strings, but I don't like the approach, and I haven't been able to get it working either.

erikbra/dotnet-sdk#1

Do you have suggestions on the best way of fixing this? Is there anywhere in the build pipeline we can decide which implementation of ITaskItem that gets instantiated? I tried looking at the constructor that takes metadata, and pondered upon whether we could use a metadata item to tell the TaskItem to not normalise the paths, or normalise to a specific platform, but I couldn't finish the thought. Are there other contructs than a TaskItem that would make sense to use for path properties where we don't want the path "fixing"?

[rainersigwald]

One possible hacky option: renormalize inside the task, which should know the target OS's preferred slash direction :-/

Is there anywhere in the build pipeline we can decide which implementation of ITaskItem that gets instantiated? I tried looking at the constructor that takes metadata, and pondered upon whether we could use a metadata item to tell the TaskItem to not normalise the paths, or normalise to a specific platform, but I couldn't finish the thought.

This sounds directionally correct to me, but I'm afraid I haven't been in that portion of the code for a while and don't remember offhand where there might be some good hooks to change things. Another possible option would be an attribute on the ITaskItem-taking input of the task that tells the engine to avoid normalization (somehow!).

Are there other contructs than a TaskItem that would make sense to use for path properties where we don't want the path "fixing"?

string is all I can think of too.

[AR-May]

@baronfel Can you take a look at this issue? @rokonec and I looked at it, and we would rather to implement a work-around in Microsoft.NET.Build.Containers.Tasks.CreateNewImage task for this.

[rainersigwald]

I haven't looked at the details of the PR but I am broadly in favor of having a way for items, item types, or tasks to opt out of the path "correction"--since it's heuristic it's caused problems before.

[baronfel]

I want to keep this open and see if it bites more people - It feels like the kind of thing that could be handled on a Task-by-Task basis, perhaps with an annotation on the Task's Input itself?

[erikbra]

I think keeping this one open is a good idea. I'm not sure whether the "build for an OS that has a different directory separator char than the one you are building on, AND, having to resolve paths at build-time, that are to be used runtime" is a very, very corner-case (only applicable to container builds), or if it's a broader issue.

That said, having worked a bit more on the PR, I'm not so much a fan of my own suggestion to solve the problem. It got a bit ugly, since the evaluated (already path-replaced) string is stored on the TaskItem in so many different code paths. Of course, one might change to doing this replacement on read, but I'm not aware of potential performance issues with this. I guess there is a reason why it's stored on write, already with the paths "fixed".

Additionally, there are many implementations of the ITaskItem in the MsBuild source code. So one needs to think about whether solving it specifically for TaskItem will solve the end-problem, or if it needs to be fixed in multiple places, maybe the whole path transformation logic should be moved to a separate, explicit place (if feasible).

I fear "fixing" this in the TaskItem, as well as being rather high-risk, as the TaskItem is used as a very core citizen of MsBuild, and probably has thousands of usages out in the wild, will not fix the problem either, as there are very many places the paths are fixed elsewhere too (on the Solution, ProjectItem, etc).

One possible way is, of course, to just go all the way, and accept that Windows works well with forward-slash paths, and has done for decades, but I think people will feel this is obtrusive.

Sorry, just a lot of loose, random thoughts here, but I think the "idea bubbling phase" is important here, both to hammer out:

1. Whether this use case is a corner case or a core case
2. How one wants to solve this long-term

In my head, the short-term solution to choose, is depending on the answers to the questions above too.