OutputPath Error Check Fails in EndToEnd.csproj

[benvillalobos]

Issue Description

We recently added BaseOutputPath to common targets which added error checks in Microsoft.Common.CurrentVersion.targets like so:

<Error Condition="'$(OutputPath)' != '' and !HasTrailingSlash('$(OutputPath)')" Text="The OutputPath must end with a trailing slash." /><Error Condition="'$(BaseOutputPath)' != '' and !HasTrailingSlash('$(BaseOutputPath)')" Text="The BaseOutputPath must end with a trailing slash." />

These errors were actually thrown in DDRITs test project EndToEnd.csproj here: https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/292082

[04:05:54.601] Failed build step "msbuild ..\EndToEnd.csproj" with exit code 1
--
[04:05:54.601] Failed build step "msbuild  ..\EndToEnd.csproj" with exit code 1
[04:05:54.601]
[04:05:54.601] Microsoft (R) Build Engine version 16.9.0-preview-20607-04+1ff34e830 for .NET Framework
[04:05:54.601] Copyright (C) Microsoft Corporation. All rights reserved.
[04:05:54.601] Build started 12/9/2020 4:05:54 AM.
[04:05:54.601] Project "C:\Test\Containers\VC.Tests.IDE\Tests\Integration\DDRITs\EndToEnd.csproj" on node 1 (default targets).
[04:05:54.601] C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\Current\Bin\Microsoft.Common.CurrentVersion.targets(824,5): error : The OutputPath must end with a trailing slash. [C:\Test\Containers\VC.Tests.IDE\Tests\Integration\DDRITs\EndToEnd.csproj]
[04:05:54.601] Done Building Project "C:\Test\Containers\VC.Tests.IDE\Tests\Integration\DDRITs\EndToEnd.csproj" (default targets) -- FAILED.
[04:05:54.601] Build FAILED.
[04:05:54.601] "C:\Test\Containers\VC.Tests.IDE\Tests\Integration\DDRITs\EndToEnd.csproj" (default target) (1) ->
[04:05:54.601] (_CheckForInvalidConfigurationAndPlatform target) ->
[04:05:54.601]   C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\Current\Bin\Microsoft.Common.CurrentVersion.targets(824,5): error : The OutputPath must end with a trailing slash. [C:\Test\Containers\VC.Tests.IDE\Tests\Integration\DDRITs\EndToEnd.csproj]
[04:05:54.601]     0 Warning(s)
[04:05:54.601]     1 Error(s)
[04:05:54.601] Time Elapsed 00:00:00.18
[04:05:54.601]

To Do:

• Download the EndToEnd.csproj project from the VS repo.
• Repro the failure.
• Figure out why EndToEnd.csproj has OutputPath set to some value that doesn't end in a slash when it hits that target.

[rainersigwald]

cc @Nirmal4G -- looks like some of the checks in #5238 may have been overzealous.

[Nirmal4G]

The use of both BaseOutputPath and OutputPath properties elsewhere suggested that we should've added this check a long time ago. Both of these properties needed to be checked in the same way as the *IntermediateOutputPath properties as it holds a similar function to those as well.

I can see the removal is temporary but please do revert #5958 when you guys fix this in the VS Repo. It could've been inserting OutputPath from the command line that it skips the appending of the trailing slash.

[rainersigwald]

@Nirmal4G Sorry, but I disagree with you strongly here. The checks must remain off if they break user projects that were successfully building.

The use of both BaseOutputPath and OutputPath properties elsewhere suggested that we should've added this check a long time ago.

I completely agree! But it wasn't there. So some projects may have been successfully building with slightly broken paths for a very long time. It's not a good user experience to break those projects to fix the paths (if the paths bothered the users sufficiently to consider them a break, they could have already fixed them).

I can see the removal is temporary

It might be. The idea behind this issue is to root-cause the discovered problems and figure out the right course of action.

when you guys fix this in the VS Repo

Even if we could do this, I doubt it's the right thing to do. The test that is failing is just "build a project". That's exactly the kind of thing MSBuild users expect to keep working! So until we know otherwise I'm treating the failure as a signal that "the pain of the well-intentioned, reasonable change we made is excessive".

[Nirmal4G]

@rainersigwald I do get that. But I personally, always look into correctness of the code/logic rather than mass appeal.

Anyone can fix the mess coming from a patch that could introduce regressions or even new bugs.

Then again, its your call, after all! 😉

[Nirmal4G]

I'm just happy that the Common targets has BaseOutputPath support.