GH4520: add slnx support to SolutionParser

[wgnf]

Closes #4520

As discussed in the above-mentioned issue I have added support for .slnx Solution files without any third-party library.

I have added the exact same tests as for the .sln files, using dotnet sln migrate to migrate the used .sln files to .slnx and keeping the assertions. That way, I could make sure that there aren't any regressions.

There are few things to point out:

• The new .slnx format does not provide any IDs anymore, as cross-references between things in the Solution are not necessary anymore. The SolutionProject still has a mandatory ID however (and I didnt't want to make breaking changes to that). So I just submitted string.Empty to the ID
• .sln and .slnx files support nested folders. In .slnx files these are modeled using the name of a solution-folder (i.e. Root is the root folder, and Root/Sub is the folder "Sub" underneath that Root-Folder). To not make things too complicated for now, I did not adhere to that consideration just yet (for the above-mentioned example, the "Root" SolutionFolder would not have an instance of the "Sub" SolutionFolder). If that is something that you want to see, I can make sure to respect that
• I have unfortunately no idea if there are any documentation hints that I would need to extend to make sure that users know that .slnx files are now also supported

If something is unclear here in the PR or in the code, please feel free to reach out :)

[wgnf]

@microsoft-github-policy-service agree

[wgnf]

Seems like the pipelines are failing however. Is that something I have to worry about?

[gep13]

@wgnf said...
Seems like the pipelines are failing however. Is that something I have to worry about?

Yes, this is absolutely something that we need to worry about 😄

Looks like your changes are failing one of the StyleCop rules that we have in place.

It could be that some newer syntax that you are using is causing a rule to be tripped.

[wgnf]

Ahh, I guess the [] is making an issue. I can go ahead and change that.

[kasperk81]

@wgnf, you can run it through this test suite https://github.com/microsoft/vs-solutionpersistence/tree/main/test/Microsoft.VisualStudio.SolutionPersistence.Tests/SlnAssets to iron out corner cases. i.e. serializeToXml(deserialize(<name>.sln.txt)) == <name>.slnx.xml

As someone who added slnx support in dotnet new sln -f slnx, on github linguist for ```slnx and .slnx syntax highlighting, then participated in whole msbuild and sdk integration process, I can tell you with confidence that there are many corner cases handled in their parser; which is why i was rooting for using official parser that was rejected by the maintainer

[wgnf]

@kasperk81

Maybe the full-fledged support using the third-party library you mentioned, supporting all the edge-cases there might be, is something that can be done in a separate AddIn, as already mentioned in the Issue.

I guess, but I might be alone on this, simple .sln (that is already there) and .slnx, that does not support all the edge-cases, is ok for the core-library, as it adds the functionality for most use-cases.

[wgnf]

The current pipeline error is the following:

/home/runner/work/cake/cake/src/Cake.Frosting.Template/Cake.Frosting.Template.csproj(25,4): error MSB3733: Input file "templates/cakefrosting/build/Build.csproj" cannot be opened. The process cannot access the file '/home/runner/work/cake/cake/src/Cake.Frosting.Template/templates/cakefrosting/build/Build.csproj' because it is being used by another process. [TargetFramework=net8.0]

Seems like some race-condition, but not something caused by my changes, to me?

[devlead]

The current pipeline error is the following:

/home/runner/work/cake/cake/src/Cake.Frosting.Template/Cake.Frosting.Template.csproj(25,4): error MSB3733: Input file "templates/cakefrosting/build/Build.csproj" cannot be opened. The process cannot access the file '/home/runner/work/cake/cake/src/Cake.Frosting.Template/templates/cakefrosting/build/Build.csproj' because it is being used by another process. [TargetFramework=net8.0]

Seems like some race-condition, but not something caused by my changes, to me?

Yes, likely a file in use; I've seen the .NET 10 SDK holding on to files a bit more at times.

[kasperk81]

that does not support all the edge-cases, is ok for the core-library, as it adds the functionality for most use-cases.

I'm not sure what you meant by "all the edge cases". Did you find concrete results from that test suite like x/N cases are not handled such that those x case are unlikely to happen in reality and hence considered "edge cases"?

[wgnf]

No, I have not used that suite yet.

[devlead]

No, I have not used that suite yet.

They're likely be edge cases, but add a couple integration tests here
https://github.com/cake-build/cake/blob/develop/tests/integration/Cake.Common/Solution/SolutionAliases.cake

Test solutions are added and copied from here
https://github.com/cake-build/cake/tree/develop/tests/integration/resources/Cake.Common/Solution

And we then we can always add more tests if a new edge case or actual issue should arise.
But if dotnet new, Cake slnx and the unit tests ones work, then we've got a good starting point.

[kasperk81]

Something not in Slnx.xsd schema is considered edge case? or what is the definition of edge case here?

We should at least gather the data what is not going to be supported and write it down in unit test in terms of: what is the expected behavior of cake when such content is provided that is understood by msbuild/vscode but rejected by cake.

[devlead]

Something not in Slnx.xsd schema is considered edge case? or what is the definition of edge case here?

We should at least gather the data what is not going to be supported and write it down in unit test in terms of: what is the expected behavior of cake when such content is provided that is understood by msbuild/vscode but rejected by cake.

To be clear, with edge cases, we mean things not needed, known or tested at this point.

We've not reviewed PR yet, but if it handles similar scenarios as existing sln parser, it'll likely be a good starting point. To fully handle everything string possible in format today or in the future is out of scope. For that we recommend creating an addin with official parser(or just reference it). 100% spec compliance is out of scope, if other common needs come up we can add more test scenarios and sort those.

[wgnf]

I just added two simple integration tests for .slnx files. I wanted to do something more complex too, but then I realized:

• The SolutionProjectEqualityComparer that is used for the solution-file integration-tests also checks for the parent of a project (see here], which I would expect and want
• but the Parent property of the SolutionProject is internal set; (see here), which makes it impossible to set up an "expected" SolutionProject in the integration test, when dealing with solution folders

And this is something I don't feel comfortable changing.

Or am I missing something that I could to, to be able to set up an integration-test considering these restrictions?

[devlead]

@wgnf you can add completely new tests that you see appropriate, important part is we test the alias works as expected with real files and cake script context.

[wgnf]

Yeah, I've already gone ahead and added two integration tests

[devlead]

Quick initial look, and this looks good to me, some minor comments. Will take a deeper look as soon as I can.

[wgnf]

Thanks for the fast feedback! I think I'll be able to adress the requests later this week or on the weekend.

[devlead]

Thanks for the fast feedback! I think I'll be able to adress the requests later this week or on the weekend.

Excellent, let us know if any questions arise.

[wgnf]

I integrated the requested changes. The Azure Pipeline has problems with a hanging process again however :/

[devlead]

LGMT 👍

[devlead]

@wgnf your changes have been merged, thanks for your contribution 👍