Sdk: Support common output directory root in SDK out of the box

[tmat]

Currently, new projects using SDK build into per-project bin and obj directories. That's good for simple projects that don't participate in CI. For repos that build using CI servers it is often a requirement to produce all outputs under a single root directory with following (or similar) layout:

$(RootOutputPath)\$(Configuration)\bin\$(MSBuildProjectName)
$(RootOutputPath)\$(Configuration)\obj\$(MSBuildProjectName)
$(RootOutputPath)\$(Configuration)\packages

We have seen countless repos with build systems that are customized to do so, each in a different way. Such build customization is usually hard to get right.

I propose to add an out of the box option to the SDK that allows customers to create such repo layouts trivially.

The $(RootOutputPath) could perhaps be specified via implicit Directory.Build.props import feature: https://github.com/Microsoft/msbuild/issues/222.

Perhaps we could also consider $(RepositoryRootPath) a well-known property that has a documented meaning. It's a generally useful property to have, imo.

To summarize I propose the user has the option to set the following properties in Directory.Build.props:

<!-- Default value for configuration is set after Directory.Build.props is imported -->  
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>

<RepositoryRootPath>$([System.IO.Path]::GetFullPath('$(MSBuildThisFileDirectory)\'))</RepositoryRootPath>
<RootOutputPath>$(RepositoryRootPath)artifacts\$(Configuration)\bin\</RootOutputPath>
<RootIntermediateOutputPath>$(RepositoryRootPath)artifacts\$(Configuration)\obj\</RootIntermediateOutputPath>

And the SDK uses these variables to set the output paths like so:

<PropertyGroup Condition="'$(RootOutputPath)' != ''" >
  <BaseOutputPath Condition="'$(BaseOutputPath)' == ''">$(RootOutputPath)$(MSBuildProjectName)\</BaseOutputPath>
  <OutputPath>$(BaseOutputPath)</OutputPath>
</PropertyGroup>

<PropertyGroup Condition="'$(RootIntermediateOutputPath)' != ''" >
  <BaseIntermediateOutputPath Condition="'$(BaseIntermediateOutputPath)' == ''">$(RootIntermediateOutputPath)$(MSBuildProjectName)\</BaseIntermediateOutputPath>
  <IntermediateOutputPath>$(BaseIntermediateOutputPath)</IntermediateOutputPath>
</PropertyGroup>

[tmat]

@srivatsn @nguerrera
/cc @ManishJayaswal @jaredpar

[jaredpar]

Very much agree. This is pushing developers to have a correct build by default. By correct I mean a build where by a given output path is only written to exactly once. Should make this as easy as possible for developers to opt into.

Note: Why use $(RepoOutputPath) here instead of $(BaseOutputPath)?

[tmat]

@jaredpar $(BaseOutputPath) is a per-project setting (as are all the other output variables that are currently used). $(RootOutputPath) is per-repo setting. Also we need an indicator that the repo is using a common root output as opposed to per-project bin/obj dirs. $(RootOutputPath) would be such indicator -- if set the layout is as proposed above.

[jaredpar]

$(BaseOutputPath) is a per-project setting (as are all the other output variables that are currently used).

I don't really see a difference between project and solution level settings. The only real difference is whether MSBuild is targeting a solution or a project during a build.

Concrete example: Assuming Util.sln contains Util.csproj I would expect the following commands to produce the same output:

> msbuild Util.sln /p:BaseOutputPath=c:\test
> msbuild Util.csproj /p:BaseOutputPath=c:\test

[dsplaisted]

The difference is that the solution level path (RootOutputPath as described here) needs to have the project name appended to it, while the project setting (BaseOutputPath) does not get the project name added to it.

[jaredpar]

That doesnt line up with the recomendations we came away with when meeting with various teams last week. In that meeting $(BaseOutputPath) definitely gets the project name appended to it.

[dsplaisted]

BaseOutputPath currently just defaults to bin\, so by default it is in the project folder and doesn't get the project name appended to it. It's a new property that the SDK introduces, but it's patterned after BaseIntermediateOutputPath from MSBuild which defaults to obj\.

Are you saying that the project name currently does get automatically added to it, or that that's what should happen?

[jaredpar]

Are you saying that the project name currently does get automatically added to it, or that that's what should happen?

What should happen. The discussion we had centered around $(BaseOutputPath) being a value that needed to be respected by build systems. If it was pre-populated with a value then it should be the root of the normal output tree that you would generate.

Concretely: suppose my code exists at e:\code\roslyn and I executed the following command

> msbuild Roslyn.sln

If my output was all under e:\code\roslyn\binaries then executing this command

> msbuild Roslyn.sln /p:BaseOutputPath=e:\example

Should put all of my output under e:\example\binaries. The directory structure under binaries should not change in either example, just the location where binaries is located.

[nguerrera]

We need to pick a different name and I like Tomas' choice of RootOutputPath. BaseOutputPath is patterned after BaseIntermediateOutputPath and we can't change the latter's long-existing meaning. There should also be RootIntermediateOutputPath for parity.

The logic could be something like:

if BaseOutputPath is not set
   if RootOutputPath is set
       BaseOutputPath = RootOutputPath\ProjectName\bin\
  else
      BaseOutputPath = bin\

Ditto for s/Output/IntermediateOutput/;s/bin/obj/

So you can /p:BaseIntermediateOutputPath and /p:BaseOutputPath do what they do now, and /p:RootOutputPath and /p:RootIntermediateOutputPath exhibit the behavior desired here.

[jaredpar]

BaseOutputPath is patterned after BaseIntermediateOutputPath and we can't change the latter's long-existing meaning.

What is the long term existing meaning? When discussed with MSBuild they were receptive of the semantics I outlined.

[tmat]

@nguerrera Agreed. In the proposal above I actually took it one step further and used a single variable $(RootOutputPath) to define a root path for (Base)OutputPath, (Base)IntermediateOutputPath and PackageOutputPath. If we thing it'd be useful to allow these to point to different roots then we can have mutliple Root* variables.

I'd be fine with either.

[nguerrera]

What is the long term existing meaning?

BaseIntermediateOutputPath never gets a project name appended to it to produce IntermediateOutputPath

[tmat]

@jaredpar All the properties that are set currently are per-project: either they include the project name, or are relative to the project directory.

[jaredpar]

@tmat

All the properties that are set currently are per-project: either they include the project name, or are relative to the project directory.

Again I don't see a difference between per project and per solution. From the perspective of build it's impossible to distinguish between the two of them.

@nguerrera

BaseIntermediateOutputPath never gets a project name appended to it to produce IntermediateOutputPath

Again opposite of what we discussed the other day.

[nguerrera]

BaseIntermediateOutputPath never gets a project name appended to it to produce IntermediateOutputPath

Again opposite of what we discussed the other day.

I wasn't there. Regardless, existing behavior cannot be up for discussion ;)

https://github.com/Microsoft/msbuild/blob/e5bc7994348dba88d1f6fe2ebd6aa354355f0063/src/Tasks/Microsoft.Common.CurrentVersion.targets#L340-L343

[nguerrera]

Again I don't see a difference between per project and per solution. From the perspective of build it's impossible to distinguish between the two of them.

There is nothing to do with solutions here. The question is whether a property implies appending a project-name on the way to the final per-project output. BaseXxx has precedent to not do this so we can make RootXxx the alternative that does.

They would behave identically when invoked by building individual projects or traversing to them from sln.

[jaredpar]

There is nothing to do with solutions here.

Then I do not understand the significance of pointing out items are per project.

[nguerrera]

In this context:

Per project means: the path is the path, there is no requirement for the sdk or msbuild to append project name to it to prevent collisions. Either user arranges to have project name in it or to keep it relative to csproj as it is by default. (Also GIGO: if you don't arrange accordingly, you can get incorrect build that stomps on itself). This is BaseXxx.

Not per project means: I can safely specify this property with a shared value and msbuild or sdk will arrange to append a project disambiguator to it when it is used in the context of any given project. This is new and we can invent RootXxx for it.

[tmat]

This is how I would set per-project variables based on root variables:

Set by the user globally:

    <RepoRoot>$([System.IO.Path]::GetFullPath('$(MSBuildThisFileDirectory)'))</RepoRoot>
    <ArtifactsDir>$(RepoRoot)artifacts\</ArtifactsDir>
    <RootOutputPath>$(ArtifactsDir)$(Configuration)\bin\</RootOutputPath>
    <RootIntermediateOutputPath>$(ArtifactsDir)$(Configuration)\obj\</RootIntermediateOutputPath>

In the SDK:

<PropertyGroup Condition="'$(RootOutputPath)' != ''" >
    <BaseOutputPath Condition="'$(BaseOutputPath)' == ''">$(RootOutputPath)$(MSBuildProjectName)\</BaseOutputPath>
    <OutputPath>$(BaseOutputPath)</OutputPath>
</PropertyGroup>

<PropertyGroup Condition="'$(RootIntermediateOutputPath)' != ''" >
    <BaseIntermediateOutputPath Condition="'$(BaseIntermediateOutputPath)' == ''">$(RootIntermediateOutputPath)$(MSBuildProjectName)\</BaseIntermediateOutputPath>
    <IntermediateOutputPath>$(BaseIntermediateOutputPath)</IntermediateOutputPath>
</PropertyGroup>

[tmat]

I believe the above is consistent with the conclusions of our meeting. Note that the properties are set conditionally, so setting Base* paths on per-project basis overrides the default settings. This makes it compatible with LUT and other systems that would want to redirect output paths. Perhaps we could set RootOutputPath also conditionally if we want easy redirection on per-repo basis.

[jaredpar]

I still think we need to start by stepping back and ...

• Discussing what the core build variables are that we need to consider.
• Which ones we need to respect if they have a default value.
• What types of items should / shouldn't be appended to those values.

Need to come to an agreement here before I can really comment on this proposal.

[jaredpar]

I believe the above is consistent with the conclusions of our meeting.

Sorry I don't.

Let's take some time, write it down, then we have a shared place to go back to for understanding.

[jaredpar]

Here is a concrete example of what I don't understand:

https://github.com/dotnet/roslyn/pull/16942

This PR was created directly as a result of our previous meeting. Didn't need any new variables here to get this running. Everything is based off of $(BaseOutputPath).

I don't know how we got from that PR and meeting to needing new variables.

[nguerrera]

You can do this yourself using only BaseXxx, but it's tricky. Feature here is to make it easier. RootXxx would be a new feature that does the right thing on your behalf when you just specify the top-level dir. You would not need to ever say $(MSBuildProjectName) yourself in a csproj or targets file to get a sane build.

[tmat]

+1 for what @nguerrera says. In addition the goal is for users to not need to touch any of the (Base)(Intermediate)OutputPath. OutDir properties.

[tmat]

Roslyn currently defines the variables like so:

    <RepoRoot>$([System.IO.Path]::GetFullPath('$(MSBuildThisFileDirectory)..\..\'))</RepoRoot>
    <BaseOutputPath Condition="'$(BaseOutputPath)' == ''">$(RepoRoot)Binaries\</BaseOutputPath>
    <OutDir>$(BaseOutputPath)$(Configuration)\</OutDir>
    <BaseIntermediateOutputPath Condition="'$(BaseIntermediateOutputPath)' == ''">$(RepoRoot)Binaries\Obj\</BaseIntermediateOutputPath>
    <IntermediateOutputPath>$(BaseIntermediateOutputPath)$(Configuration)\$(MSBuildProjectName)\</IntermediateOutputPath>

It's slightly different from the proposal above which includes the project name in the Base* paths as well. It turns out this is needed to be compatible with dotnet SDK. Once we move Roslyn to the SDK we will need to include the project name not just in (Intermediate)OutputPath but also in the Base variables. Doing so brings it on par with this proposal.

[jaredpar]

It's slightly different from the proposal above which includes the project name in the Base* paths as well. It turns out this is needed to be compatible with dotnet SDK.

I think this is what is getting to me. This issue is jumping to a conclusion without fully explaining the problem. It may make sense to the others on this who are more familiar with the SDK. For others though I just don't see the justification.

I think it would help to step back and ...

1. Define the core MSBuild properties involved here.
2. Note which ones may have pre-populated values that need to be respected.
3. Note the properties which should not be combined together.

[Nirmal4G]

For me, I put these props/targets in my build toolset folder and I import them in Directory.Build.<props/targets>.

Here is the gist of my MSBuild Output Configurations.

Related Issues:

1. #803
2. Microsoft/msbuild#1603

[lennoncork]

@Nirmal4G just for information sake, out issue was that we were modifying the build output but also including the $(AssemblyName) and $(Configuration) to get a clean build structure. We are also targeting multiple frameworks so we wanted the additional separation. For now we're reverted back to the default structure until a suitable solution exists.

[Nirmal4G]

@lennoncork These are not the final solution. I saw people posting their workarounds and so I posted mine. This will help until common props and targets gets their own SDK story.

For now, workarounds are better, In my example, instead of MSBuildProjectName you can use AssemblyName and/or ProjectName. if those are not set, you can override BuildPath instead of OutputPath and IntermediateOutputPath and the BaseXxx ones.

But completely overhauling the Build Output structure requires a significant work that takes across all the props/targets and those of SDKs that ship with MSBuild and VS.

[Nirmal4G]

Please consider these names

• BuildPath
• BaseBuildPath
• BuildOutputPath
• BuildRootPath
• RootBuildPath

I personally like BuildPath and BuildOuputPath and I used the former one in my example

Also use PublishPath or PublishOuputPath and set it default to BuildPath itself so that user can override it with a local directory parallel to BuildPath or server share if they want to!

[marcpopMSFT]

@rainersigwald this relates to the PR you have in msbuild. Will believes this will break c++/CLI and so we may want to consider special casing there.

[Nirmal4G]

Here's my take on this via BuildDir property: Nirmal4G@ab012dcf481a25a8bdd9b5a8247c7baf9313a1b9

MSBuild's side: Nirmal4G/msbuild@a9563c04adb0255815acd3dbfd1f13896ac4d530

• It does introduce build^ via BuildDir and publish^ via PublishDir in the project root.
• Uses BuildDir for Path mismatch warning between props/targets.
• It moves MSBuildProjectExtensionsPath to BuildDir.

Thus, freeing up BaseIntermediateOutputPath from Common props. I believe this will serve up nicely in years to come.

^Note: we can prepend ~ in order to differentiate it from source folders. We could also have BuildDirName and use the existing PublishDirName to make the folder names overridable!

[marcpopMSFT]

https://github.com/dotnet/msbuild/pull/5238

[jaredpar]

@marcpopMSFT is there any work to do after that MSBuild change was merged?

[dsplaisted]

@marcpopMSFT is there any work to do after that MSBuild change was merged?

We'll have to look into it, but I think that the SDK copied some of MSBuilds output path calculation logic, because it needed the paths to be calculated earlier in the evaluation or something.