Add BaseOutputPath to common targets #5238

Nirmal4G · 2020-04-05T20:07:04Z

While this is not officially supported on non-sdk projects, NuGet Pack targets need a common output path to put '.nupkg' across both type of projects.

Required for NuGet/Home#9234 in PR NuGet/NuGet.Client#3270

Also Fixes #1664

Nirmal4G · 2020-04-12T11:56:17Z

I don't know who to tag. So, @rainersigwald NuGet Pack target needs this. Can you or anyone from the team take a look at this?

rainersigwald

Team triage: we're amenable in theory to taking this in 16.7/SDK 3.1.400.

src/Tasks/Microsoft.Common.CurrentVersion.targets

rainersigwald

I appear to have written up comments but not published them. Sorry!

src/Tasks/Microsoft.Common.CurrentVersion.targets

Nirmal4G · 2020-06-13T15:09:36Z

Did a little bit of refactoring to place all the output logic in one place. Other than that, only changes include what you have suggested: Using *OutputPath properties to fallback to legacy Configuration/Platfrom check. That's the last commit.

Should I add tests for the *OutputPath behaviors? If so, then, Where are the tests for the OutputPath?

src/Tasks/Microsoft.Common.CurrentVersion.targets

rainersigwald · 2020-07-17T16:02:00Z

I think this looks good to go (with the minor changes that are outstanding). Would you mind resolving conflicts?

Nirmal4G · 2020-07-18T15:58:29Z

When I tested the above logic with and without Common targets, there's a noticeable difference of around 700ms-1s delay when using property functions. This is observed with running the test project again and again in Command Prompt and WSL manually. Is there any guide/tool to properly profile MSBuild builds?

I have tested the above logic with

\, /
Path.Join
Path.Combine
NormalizeDirectory

The NormalizeDirectory/NormalizePath doesn't always generate paths under MSBuildProjectDirectory. Is this Intentional?

Behaviour of NormalizeDirectory

D:\Projects\Work\Repros>msbuild Test.proj -p:Configuration=C:Debug

Microsoft (R) Build Engine version 16.6.0+5ff7b0c9e for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 18-07-2020 08:30:16 PM.
Project "D:\Projects\Work\Repros\Test.proj" on node 1 (default targets).
Test:
  MSBuildToolsVersion: Current
  MSBuildToolsPath:    C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Current\Bin
  MSBuildProjectDirectory: D:\Projects\Work\Repros
  BaseOutputPath: bin\
  BaseIntermediateOutputPath: obj\
  OutputPath: C:\Program Files (x86)\Microsoft Visual Studio\Installer\Debug\
  IntermediateOutputPath: C:\Program Files (x86)\Microsoft Visual Studio\Installer\Debug\
Done Building Project "D:\Projects\Work\Repros\Test.proj" (default targets).


Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.61

Behaviour of Path.Combine

D:\Projects\Work\Repros>msbuild Test.proj -p:Configuration=C:Debug

Microsoft (R) Build Engine version 16.6.0+5ff7b0c9e for .NET Framework
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 18-07-2020 08:33:25 PM.
Project "D:\Projects\Work\Repros\Test.proj" on node 1 (default targets).
Test:
  MSBuildToolsVersion: Current
  MSBuildToolsPath:    C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Current\Bin
  MSBuildProjectDirectory: D:\Projects\Work\Repros
  BaseOutputPath: bin\
  BaseIntermediateOutputPath: obj\
  OutputPath: C:Debug\
  IntermediateOutputPath: C:Debug\
Done Building Project "D:\Projects\Work\Repros\Test.proj" (default targets).


Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.63

Behaviour of Path.Join

D:\Projects\Work\Repros>dotnet msbuild Test.proj -p:Configuration=C:Debug -v:n

Microsoft (R) Build Engine version 16.6.0+5ff7b0c9e for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

Build started 18-07-2020 08:36:31 PM.
     1>Project "D:\Projects\Work\Repros\Test.proj" on node 1 (default targets).
     1>Test:
         MSBuildToolsVersion: Current
         MSBuildToolsPath:    C:\Program Files\dotnet\sdk\3.1.302
         MSBuildProjectDirectory: D:\Projects\Work\Repros
         BaseOutputPath: bin\
         BaseIntermediateOutputPath: obj\
         OutputPath: bin\C:Debug\
         IntermediateOutputPath: obj\C:Debug\
     1>Done Building Project "D:\Projects\Work\Repros\Test.proj" (default targets).

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.21

Out of all the 3, which one do we want to have?

So far Path.Join is my favourite but it's not available to MSBuild for .NET Framework!

rainersigwald · 2020-07-21T20:27:53Z

there's a noticeable difference of around 700ms-1s delay when using property functions

That's a shockingly large difference. Can you share exactly how you were testing?

Is there any guide/tool to properly profile MSBuild builds?

We have a fairly detailed evaluation-time profiler, and can also use our ETW events.

The NormalizeDirectory/NormalizePath doesn't always generate paths under MSBuildProjectDirectory. Is this Intentional?

The results you shared don't make any sense to me, no. Again, can you describe exactly how you got them?

Nirmal4G · 2020-08-05T15:38:12Z

Here's the test projects with different variations of OutputPath definitions: MSBuild-5238-Repro.zip

With both Desktop and Core MSBuild run the test projects with Configuration=C:Debug or PlatformName=C:ARM (i.e. prefix them with Windows Drive notation) and observe the differences.
They are of course invalid Configuration and Platform but Except /,\ and Path.Join all others have different behaviors with relative path inserted into the above properties.
I don't know what sort of problems they will bring down the road but I observed it by accident and so, I post here for clarification.

As you have suggested, we can also profile them with or without Common targets using ImportCommonTargets property. I will also profile them (both the test projects and PR patch) and post them here soon.

Forgind · 2020-08-21T22:04:32Z

Without having looked too carefully, does this also resolve #2308?

Nirmal4G · 2020-10-13T14:19:44Z

@Forgind

Without having looked too carefully, does this also resolve #2308?

No, I don't think so. That requires changes in both C++ and .NET targets.

src/Tasks/Microsoft.Common.CurrentVersion.targets

rainersigwald

I need to look at the tests but other than the comments about "MPFProj" that I need clarification on this is looking good. Thanks for your patience!

src/Tasks/Microsoft.Common.CurrentVersion.targets

While this is not officially supported on non-sdk projects, NuGet Pack targets need a common output path to put '.nupkg' across both type of projects

Replace 'HasTrailingSlash' conditional function with '[MSBuild]::EnsureTrailingSlash' property function Replace '[System.IO.Path]::Combine' property function with '[MSBuild]::NormalizePath' and '[MSBuild]::NormalizeDirectory' property functions

when using OutputPath without BaseOutputPath

…bine paths Usually, either '[MSBuild]::NormalizePath' or '[MSBuild]::NormalizeDirectory' property functions would be preferred for these cases. But, there's a bug (on Windows) which occurs when a drive relative path like 'C:Projects' is specified and it fails to return to the project directory. Usually, '[System.IO.Path]::Join' would be most preferred in this context. But, it's not available in NETFX. So, '[System.IO.Path]::Combine' is used instead.

Making sure that existing 'OutputPath' scenarios work, while allowing the use of 'BaseOutputPath' property

Adjust the wording to make it simple, brief and clear. Co-Authored-By: Rainer Sigwald <raines@microsoft.com> > Fixup with original commit when this patch is approved.

Silly ME! There were a lot of build errors from the Fakes targets. I was banging my head on which change caused this but finally found it. Guess being sick has its perks.

rainersigwald

Awesome, thanks!

Nirmal4G · 2020-12-02T20:08:32Z

@rainersigwald Sorry for the late reply!

Went to celebrate Diwali and came back with a bad case of cold.

Forgind

Looks good to me! Thanks for this!

Went to celebrate Diwali and came back with a bad case of cold.

I'm sorry to hear that. Glad you're feeling better!

Nirmal4G · 2020-12-05T06:50:18Z

I'm sorry to hear that. Glad you're feeling better!

I'm fine now. Thanks!

Nirmal4G · 2020-12-09T13:44:43Z

Finally! 🥳

This was referenced Apr 5, 2020

OutputPath logic in Pack targets in a customized build doesn't work properly NuGet/Home#9234

Closed

Fix output path issues with customized builds NuGet/NuGet.Client#3270

Merged

Nirmal4G marked this pull request as ready for review April 6, 2020 12:39

marcpopMSFT added this to the MSBuild 16.7 Preview 1 milestone Apr 14, 2020

Forgind mentioned this pull request Apr 20, 2020

Paths starting with ./ in DefaultItemExcludes are not properly handled #5281

Open

rainersigwald self-assigned this Apr 22, 2020

rainersigwald requested changes Apr 24, 2020

View reviewed changes

src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved

src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved

src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved

Nirmal4G requested a review from rainersigwald April 29, 2020 12:15

marcpopMSFT modified the milestones: MSBuild 16.7 Preview 1, MSBuild 16.7 Preview 2 May 1, 2020

This was referenced May 2, 2020

Always use 'Configuration' property #5330

Closed

Fix Comment at target ResolveComReferences #5329

Merged

Nirmal4G commented May 2, 2020

View reviewed changes

src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved

rainersigwald reviewed Jun 5, 2020

View reviewed changes

src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved

src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved

Nirmal4G marked this pull request as draft June 10, 2020 18:55

Nirmal4G marked this pull request as ready for review June 13, 2020 12:10

rainersigwald requested changes Jun 18, 2020

View reviewed changes

rainersigwald modified the milestones: MSBuild 16.7 Preview 2, MSBuild 16.8 Jun 18, 2020

marcpopMSFT modified the milestones: MSBuild 16.8, MSBuild 16.9 Sep 23, 2020

Nirmal4G requested a review from Forgind September 28, 2020 19:16

Nirmal4G requested a review from rainersigwald October 13, 2020 14:10

rainersigwald reviewed Oct 23, 2020

View reviewed changes

src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved

rainersigwald reviewed Oct 23, 2020

View reviewed changes

src/Tasks/Microsoft.Common.CurrentVersion.targets Outdated Show resolved Hide resolved

Nirmal4G added 7 commits December 2, 2020 09:25

Add BaseOutputPath to common targets

2f335b0

While this is not officially supported on non-sdk projects, NuGet Pack targets need a common output path to put '.nupkg' across both type of projects

Fallback to legacy behavior for Configuration/Platform validation

9a7012e

when using OutputPath without BaseOutputPath

Add tests for checking 'OutputPath' properties

97bc2b5

Making sure that existing 'OutputPath' scenarios work, while allowing the use of 'BaseOutputPath' property

Update comments on legacy Configuration/Platform check

17c4944

Adjust the wording to make it simple, brief and clear. Co-Authored-By: Rainer Sigwald <raines@microsoft.com> > Fixup with original commit when this patch is approved.

Add a missing bracket

f7e97bd

Silly ME! There were a lot of build errors from the Fakes targets. I was banging my head on which change caused this but finally found it. Guess being sick has its perks.

rainersigwald approved these changes Dec 2, 2020

View reviewed changes

Forgind approved these changes Dec 4, 2020

View reviewed changes

rainersigwald merged commit 7528963 into dotnet:master Dec 7, 2020

Nirmal4G mentioned this pull request Dec 9, 2020

dotnet build with output folder causes assemblies to stomp on each other for Multiple TFMs dotnet/sdk#8964

Open

Nirmal4G deleted the patch-1 branch December 9, 2020 13:43

benvillalobos mentioned this pull request Dec 11, 2020

OutputPath Error Check Fails in EndToEnd.csproj #5965

Open

3 tasks

marcpopMSFT mentioned this pull request Dec 16, 2020

Support common output directory root in SDK out of the box dotnet/sdk#867

Closed

dsplaisted mentioned this pull request Dec 28, 2020

Move PackageOutputPath initialization to NuGet's Pack targets dotnet/sdk#10733

Closed

Nirmal4G mentioned this pull request Feb 1, 2021

Miscellaneous refactors leftover from #5238 #6104

Merged

JoeRobich mentioned this pull request Feb 4, 2021

It looks like Microsoft.CodeAnalysis.MSBuild.UnitTests.MSBuildWorkspaceTests.TestOpenProject_AssemblyNameIsPath unit-test is flaky dotnet/roslyn#50258

Closed

Nirmal4G mentioned this pull request Feb 5, 2021

Support Common output via BuildDir and PublishDir #6105

Closed

Nirmal4G mentioned this pull request Mar 21, 2021

Revert broken code cleanup #6245

Merged

Nirmal4G mentioned this pull request Jul 7, 2022

Prevent --output command-line option for multi-targeted projects dotnet/sdk#13369

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BaseOutputPath to common targets #5238

Add BaseOutputPath to common targets #5238

Nirmal4G commented Apr 5, 2020 •

edited

Loading

Nirmal4G commented Apr 12, 2020 •

edited

Loading

rainersigwald left a comment

rainersigwald left a comment

Nirmal4G commented Jun 13, 2020

rainersigwald commented Jul 17, 2020 •

edited

Loading

Nirmal4G commented Jul 18, 2020

rainersigwald commented Jul 21, 2020

Nirmal4G commented Aug 5, 2020

Forgind commented Aug 21, 2020

Nirmal4G commented Oct 13, 2020 •

edited

Loading

rainersigwald left a comment

rainersigwald left a comment

Nirmal4G commented Dec 2, 2020

Forgind left a comment •

edited

Loading

Nirmal4G commented Dec 5, 2020

Nirmal4G commented Dec 9, 2020

Add BaseOutputPath to common targets #5238

Add BaseOutputPath to common targets #5238

Conversation

Nirmal4G commented Apr 5, 2020 • edited Loading

Nirmal4G commented Apr 12, 2020 • edited Loading

rainersigwald left a comment

Choose a reason for hiding this comment

rainersigwald left a comment

Choose a reason for hiding this comment

Nirmal4G commented Jun 13, 2020

rainersigwald commented Jul 17, 2020 • edited Loading

Nirmal4G commented Jul 18, 2020

rainersigwald commented Jul 21, 2020

Nirmal4G commented Aug 5, 2020

Forgind commented Aug 21, 2020

Nirmal4G commented Oct 13, 2020 • edited Loading

rainersigwald left a comment

Choose a reason for hiding this comment

rainersigwald left a comment

Choose a reason for hiding this comment

Nirmal4G commented Dec 2, 2020

Forgind left a comment • edited Loading

Choose a reason for hiding this comment

Nirmal4G commented Dec 5, 2020

Nirmal4G commented Dec 9, 2020

Nirmal4G commented Apr 5, 2020 •

edited

Loading

Nirmal4G commented Apr 12, 2020 •

edited

Loading

rainersigwald commented Jul 17, 2020 •

edited

Loading

Nirmal4G commented Oct 13, 2020 •

edited

Loading

Forgind left a comment •

edited

Loading