Update Directory.Build.props

[captainsafia]

Fixes #32615.

The RepoRoot environment variable is set by the build scripts in each of the project areas. The value looks something like src/Components/../.. without a trailing slash.

This ends up breaking the build locally after the user has run the local build scripts since we no longer override it with one that continues the trailing slash.

We should always set the property in MSBuild and not rely on the global property set in the build scripts.

[SteveSandersonMS]

Looks great - thanks for resolving this!

[dougbu]

This won't fix anything because $(RepoRoot) is always either a global property that can only be overridden when invoking an inner build using the <MSBuild/> task (as Arcade does) or not set.

[captainsafia]

can only be overridden when invoking an inner build using the task

What do you mean by this?

[SteveSandersonMS]

This change does fix the issue on my machine.

I don't have an explanation for why this issue occurs for me and not for @captainsafia though. I have tried completely cleaning my local copy (git clean -xdf) so that all artifacts and the local .NET SDK were removed, and the problem still occurred after running build.cmd followed by dotnet build.

[captainsafia]

Yes, here's how I'm reasoning about why this fix makes sense.

Before this change

User runs build.cmd then dotnet build

The CMD script sets the RepoRoot property to a path without a trailing slash. When the user runs, dotnet build it loads the Directory.Build.props file which doesn't override the value with a property that contains the trailing slash.

User runs build.cmd
Everything works fine because the RepoRoot environment variable is overridden by the MSBuild in Build.proj.

After this change

User runs build.cmd then dotnet build

The CMD script sets the RepoRoot property to a path without a trailing slash. When the user runs, dotnet build it loads the Directory.Build.props file which overrides the value with a property that contains the trailing slash.

User runs build.cmd
Everything works fine because the RepoRoot environment variable is overridden by the MSBuild in Build.proj.

When building with the CMD scripts, Arcade will always override the value to the correct thing. However, when building with dotnet we need this change to make sure the trailing slash is set.

[dougbu]

What I'm not understanding is how your change helps in the dotnet msbuild case. That is, my <RepoRoot Condition=" '$(RepoRoot)' == '' ">$([MSBuild]::EnsureTrailingSlash('$(MSBuildThisFileDirectory)'))</RepoRoot> should do the Right Thing™️ in the dotnet msbuild case because $(RepoRoot) is not set then whether or not build.cmd ran before it. Starting to wonder if the root cause is actually https://github.com/dotnet/aspnetcore/blob/main/src/Testing/src/build/Microsoft.AspNetCore.Testing.props#L4 because $(RepositoryRoot) may be an odd value in a few cases. A fix to that line similar to what's in the root Directory.Build.props already might be warranted to cover our bases.

[captainsafia]

should do the Right Thing™️ in the dotnet msbuild case because $(RepoRoot) is not set then whether or not build.cmd ran before it.

RepoRoot will be set if build.cmd ran before it. In that case, the build script will set the RepoRoot environment variable to a path without a trailing slash. Then the '$(RepoRoot)' == '' will evaluate to false because $(RepoRoot) will resolve to the value defined as an environment var.

Since we're using dotnet msbuild the properties defines in the Build.proj won't take affect and we'll never override the incorrect value that we set.

[dougbu]

Separately, the current code here will result in extra noise in the normal (Arcade-ified) case because msbuild and dotnet msbuild warn when something tries to override a global properties.

can only be overridden when invoking an inner build using the task

What do you mean by this?

Ms. Build treats global properties (environment variables and /p:Foo=Bar on the command line) as read-only. The TreatAsLocalProperty attribute is an escape hatch from that.

In any case, I'm glad this works but

1. Want to understand how much change introduced this or what else caused the new "direct build" issues
2. Very much do not want to make our binary logs noisier in the cases that are currently working.

I'm working on (1) still because I keep doing dumb things trying to get a binary log for the SignalR pipeline failures.

For (2), use ! HasTrailingSlash(...) to reduce attempts to override a global property and a variant of the new line (perhaps using NormalizeDirectory($(MSBuildThisFileDirectory), '..', '..' , '..', '..')) in Microsoft.AspNetCore.Testing.props. May need to include an empty check in the Condition here because Ms. Build sometimes messes up when given a non-existent property.

[dougbu]

Since we're using dotnet msbuild the properties defines in the Build.proj won't take affect and we'll never override the incorrect value that we set.

In the dotnet msbuild case, nothing except this line (which should execute) and Microsoft.AspNetCore.Testing.props sets $(RepoRoot). You're correct Build.proj doesn't matter but there's also no global property to mess w/ unless you're talking about @SteveSandersonMS's /p:RepoRoot=C:\your\path\to\aspnetcore\ workaround.

Put another way, build.cmd doesn't do anything that should help or hinder the later dotnet msbuild command. (Well, except possibly for covering projects that wouldn't otherwise be ready for use.)

[captainsafia]

build.cmd sets the RepoRoot variable and MSBuild allows you to reference environment variables as properties (ref) so it does affect the dotnet msbuild invocation since it sets the RepoRoot property.

[dougbu]

build.cmd sets the RepoRoot variable

The scripts set the property on the command line, not in the environment. Those settings don't exist when you later run dotnet msbuild.

In any case, I now have a binary log to help me understand what's happening.

[dougbu]

One option might be to use TreatAsLocalProperty="RepoRoot" as Arcade does at https://github.com/dotnet/arcade/blob/e7ede87875f41a9b3df898ae08da5ebc96e24f56/src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj#L2

[dougbu]

Suggest we go forward w/ #32664 because that doesn't have the extra override issue and cleans up a bit more. Plus #32615 is clearly (now) my bad.

[captainsafia]

Sounds good!