Skip to content

Conversation

@Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Oct 22, 2025

Fixes #51398
Fixes #51481

@Youssef1313 Youssef1313 marked this pull request as ready for review October 23, 2025 12:53
var msbuildArgs = MSBuildArgs.AnalyzeMSBuildArguments(buildOptions.MSBuildArgs, CommonOptions.PropertiesOption, CommonOptions.RestorePropertiesOption, CommonOptions.MSBuildTargetOption(), CommonOptions.VerbosityOption());
if (string.IsNullOrEmpty(activeSolutionConfiguration))
{
activeSolutionConfiguration = solutionFile.GetDefaultConfigurationName();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rainersigwald @baronfel Can this be null or empty under any scenario?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsure, would need to reach out to the VS Solution team for clarification on specific behaviors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From comment from Richard below:

A valid solution file will always have at least one solution configuration and solution platform, it should be safe to not continue if this isn't true.

activeSolutionPlatform = solutionFile.GetDefaultPlatformName();
}

var solutionConfiguration = solutionFile.SolutionConfigurations.FirstOrDefault(c => c.ConfigurationName == activeSolutionConfiguration && c.PlatformName == activeSolutionPlatform)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rainersigwald @baronfel Is this the right way to get the solution configuration?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you may need to handle the case where the solution configuration accepts any platform? but am not certain, again a question for the VS Solution team about required solution behaviors.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Platform and configuration are case insensitive, so if they are coming from the user's command line, these checks should ignore case.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A valid solution file will always have at least one solution configuration and solution platform, it should be safe to not continue if this isn't true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed to use OrdinalIgnoreCase comparison.

var solutionConfiguration = solutionFile.SolutionConfigurations.FirstOrDefault(c => c.ConfigurationName == activeSolutionConfiguration && c.PlatformName == activeSolutionPlatform)
?? throw new InvalidOperationException($"The solution configuration '{activeSolutionConfiguration}|{activeSolutionPlatform}' is invalid.");

// TODO: What to do if the given key doesn't exist in the ProjectConfigurations?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rainersigwald @baronfel Few lines below, I'm filtering projects only where the project configurations contains the requested solution configuration. Should that be an error instead if the solution configuration isn't found in the dictionary? or is it a valid scenario? And if valid scenario, is skipping the project the right behavior?

using var collection = new ProjectCollection(globalProperties: CommonRunHelpers.GetGlobalPropertiesFromArgs(msbuildArgs), logger is null ? null : [logger], toolsetDefinitionLocations: ToolsetDefinitionLocations.Default);
var evaluationContext = EvaluationContext.Create(EvaluationContext.SharingPolicy.Shared);
IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModules> projects = SolutionAndProjectUtility.GetProjectProperties(projectFilePath, collection, evaluationContext, buildOptions);
IEnumerable<ParallelizableTestModuleGroupWithSequentialInnerModules> projects = SolutionAndProjectUtility.GetProjectProperties(projectFilePath, collection, evaluationContext, buildOptions, configuration: null, platform: null);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this code path we are dealing with a single project.

Comment on lines +40 to +41
globalProperties.TryGetValue("Configuration", out var activeSolutionConfiguration);
globalProperties.TryGetValue("Platform", out var activeSolutionPlatform);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Configuration/Platform here are "solution configuration". This maps then to "project configuration" on the individual project files.

Should we clear out those values from the dictionary at this point?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good line of thinking, I think you may be right. To do it right you'd use this to find the solutionconfiguration, and then use the solutionconfiguration to find per-project configuration/platform/etc arguments as we talked about today.

.Where(p => ProjectShouldBuild(solutionFile, p.RelativePath) && p.ProjectConfigurations.ContainsKey(solutionConfiguration.FullName))
.Select(p => (p.ProjectConfigurations[solutionConfiguration.FullName], p.AbsolutePath))
.Where(p => p.Item1.IncludeInBuild)
.Select(p => (p.AbsolutePath, p.Item1.ConfigurationName, p.Item1.PlatformName));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rainersigwald @baronfel Can ConfigurationName or PlatformName here be null or empty?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsure, to me these are all questions about solution behavior - and such the VS Solution team needs to eb brought in.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid projects require a configuration name, but platform is options. For example, reporting projects and setup projects don't have platforms. Also, there are many invalid solution files out there were configurations may be missing from the file. What to do in those cases is up to the tool, but it should probably be ignored, but not skip other valid projects.

@Youssef1313
Copy link
Member Author

@copilot Please add tests for #51481 and #51398. The tests should be in GivenDotnetTestBuildsAndRunsTestsWithDifferentOptions, and test assets go in test/TestAssets/TestProjects.

Copy link
Contributor

Copilot AI commented Nov 3, 2025

@Youssef1313 I've opened a new pull request, #51526, to work on those changes. Once the pull request is ready, I'll request review from you.

@Youssef1313
Copy link
Member Author

@copilot Fix this failure:

Expected string "C:\h\w\AC400931\t\dotnetSdkTests\1hijiz2p.5kp\e56198d1-1eca---9A84D171\MultiTestProjectSolutionWithSharedProject.slnx : error MSB4025: The project file could not be loaded. Microsoft.Build.Exceptions.InvalidProjectFileException: Solution filter file at "C:\h\w\AC400931\t\dotnetSdkTests\1hijiz2p.5kp\e56198d1-1eca---9A84D171\TestProjectsWithShared.slnf" includes project "TestProject/TestProject.csproj" that is not in the solution file at "C:\h\w\AC400931\t\dotnetSdkTests\1hijiz2p.5kp\e56198d1-1eca---9A84D171\MultiTestProjectSolutionWithSharedProject.slnx".  C:\h\w\AC400931\t\dotnetSdkTests\1hijiz2p.5kp\e56198d1-1eca---9A84D171\TestProject\TestProject.csproj
C:\h\w\AC400931\t\dotnetSdkTests\1hijiz2p.5kp\e56198d1-1eca---9A84D171\MultiTestProjectSolutionWithSharedProject.slnx : error MSB4025:    at Microsoft.Build.Shared.ProjectFileErrorUtilities.VerifyThrowInvalidProjectFile(Boolean condition, String errorSubCategoryResourceName, BuildEventFileInfo projectFile, Exception innerException, String resourceName, Object[] args)
C:\h\w\AC400931\t\dotnetSdkTests\1hijiz2p.5kp\e56198d1-1eca---9A84D171\MultiTestProjectSolutionWithSharedProject.slnx : error MSB4025:    at Microsoft.Build.Construction.SolutionFile.ValidateProjectsInSolutionFilter()
C:\h\w\AC400931\t\dotnetSdkTests\1hijiz2p.5kp\e56198d1-1eca---9A84D171\MultiTestProjectSolutionWithSharedProject.slnx : error MSB4025:    at Microsoft.Build.Construction.SolutionFile.ReadSolutionModel(SolutionModel solutionModel)
C:\h\w\AC400931\t\dotnetSdkTests\1hijiz2p.5kp\e56198d1-1eca---9A84D171\MultiTestProjectSolutionWithSharedProject.slnx : error MSB4025:    at Microsoft.Build.Construction.SolutionFile.ParseUsingNewParser()" to contain "TestProject.dll".

Copy link
Contributor

Copilot AI commented Nov 16, 2025

@Youssef1313 I've opened a new pull request, #51770, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: Youssef1313 <[email protected]>
@Youssef1313 Youssef1313 enabled auto-merge (squash) November 18, 2025 10:57
@Youssef1313 Youssef1313 merged commit 0a0a993 into release/10.0.2xx Nov 18, 2025
22 of 25 checks passed
@Youssef1313 Youssef1313 deleted the dev/ygerges/parse-sln branch November 18, 2025 12:11
@Youssef1313
Copy link
Member Author

/backport to release/10.0.1xx

@github-actions
Copy link
Contributor

Started backporting to release/10.0.1xx (link to workflow run)

@github-actions
Copy link
Contributor

@Youssef1313 backporting to release/10.0.1xx failed, the patch most likely resulted in conflicts. Please backport manually!

git am output
$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Patch format detection failed.
Error: The process '/usr/bin/git' failed with exit code 128

Link to workflow output

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants