[Regression in 10.0.101] Running `dotnet test` with `Configuration`/`Platform` environment variables isn't considered

[Youssef1313]

See microsoft/testfx#7121

[Youssef1313]

I know what's going on here. This is a regression from the fix we did in .NET 10.0.101 for correct parsing of solution files (#51794).

To mimic MSBuild behavior for solution vs project configuration, what we did on SDK is that we parse the solution file, get the solution configuration (if specified by user), or get the default solution configuration (usually Debug), and map that to the project configurations for the individual projects in the solution.

However, what we missed is that we need to read Configuration (and Platform) env variables if no explicit solution configuration is specified, before we fallback to the default.

When everything is fully MSBuild, this happens naturally because MSBuild property reads will consider env variables, and solution files are no different here (they generate a metaproj behind the scenes).

FYI @baronfel .

[baronfel]

I really would not suggest leaning more into environment variables for MSBuild properties - especially super important ones like Configuration and Platform. We generally regard this as a mistake in MSBuild these days and would not do this design if we were facing the same problem again. I'd be much more inclined to tell the user they are holding it wrong, and to have a MSbuild BuildCheck to flag any build that uses the Configuration/Platform/etc env vars. In fact, we have a buildcheck today that flags on any env var usage, that's how much we dislike the feature.

[Youssef1313]

@baronfel Hmm, so are we okay with having discrepancy between build and test commands when working on solutions? In addition, a discrepancy in test command itself when running on project (which will consider the env variable most likely) vs when running on solution?

[afscrome]

We have a number of pipelines where we heavily rely on env vars for passing msbuild properties like Configuration and Platform. Mainly because we can set these as job level env vars in CI, and then we ensures we pass the same properties through to dotnet restore, dotnet build, dotnet test, dotnet publish, dotnet pack etc. We've found doing this massively dropped the number of times people added properties to one command, leading to a later command rebuilding everything.

If this change is introduced in a newer .Net 10 sdk, that's going to be a massive break across our build pipelines, and certainly not one I'd expect in a minor sdk update.

[0xced]

This is exactly why I filed microsoft/testfx#7121 in the first place. All my .NET builds are setting the Configuration environment variable to Release to ensure that all the dotnet commands use the same configuration through the build pipeline.

https://github.com/0xced/0xced.Templates/blob/79e9cf98647d981be8db4861d7b47beff37e75b5/templates/nuget-classlib/.github/workflows/continuous-integration.yml#L5-L6

Having this behaviour change between the SDK version 10.0.100 and version 10.0.101 is very much unexpected.

We generally regard this (environment variables for MSBuild properties) as a mistake in MSBuild these days

I understand but such a breaking change should definitely not happen in a minor SDK update! Also, the Use environment variables in a build documentation should probably be updated. Nothing discourages users from using environment variables as MSBuild properties, it just explains how the current mechanism works.

we have a buildcheck today that flags on any env var usage, that's how much we dislike the feature

I don't know what a buildcheck is but it did not warn me about using environment variables. It took me quite some time to diagnose why I was getting a Win32Exception while running my tests which were running fine with the SDK 10.0.100.

Are we okay with having discrepancy between build and test commands when working on solutions?

I say no but I'm very aware that my voice in GitHub issues has extremely little weight. 😁

[Youssef1313]

Ping @baronfel to suggest how to move forward here.

FYI @nohwnd as well.