Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace homegrown sln parser with library #222

Merged
merged 1 commit into from
Jan 7, 2025
Merged

Conversation

baronfel
Copy link
Collaborator

@baronfel baronfel commented Jan 3, 2025

This PR removes our copied solution parser and instead uses Microsoft.VisualStudio.SolutionPersistence.

The API surface area is intended to remain the same here, I'm not trying to do a huge new refactoring here.

@baronfel baronfel requested a review from Copilot January 3, 2025 02:07

Choose a reason for hiding this comment

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

Copilot reviewed 21 out of 30 changed files in this pull request and generated no comments.

Files not reviewed (9)
  • Directory.Packages.props: Language not supported
  • ionide-proj-info.sln: Language not supported
  • src/Ionide.ProjInfo.ProjectSystem/WorkspacePeek.fs: Language not supported
  • src/Ionide.ProjInfo.Sln/Ionide.ProjInfo.Sln.csproj: Language not supported
  • src/Ionide.ProjInfo/InspectSln.fs: Language not supported
  • src/Ionide.ProjInfo/Ionide.ProjInfo.fsproj: Language not supported
  • test/Ionide.ProjInfo.Tests/Tests.fs: Language not supported
  • src/Ionide.ProjInfo.Sln/ExceptionHandling.cs: Evaluated as low risk
  • src/Ionide.ProjInfo.Sln/ProbablyUnusedNamespaces.cs: Evaluated as low risk
@baronfel
Copy link
Collaborator Author

baronfel commented Jan 3, 2025

@Krzysztof-Cieslak dude what the heck. Look at all that "language not supported" :(

@baronfel baronfel force-pushed the replace-sln-parser branch from 74ad76d to a663ecc Compare January 3, 2025 02:12
@baronfel baronfel requested a review from TheAngryByrd January 3, 2025 02:16
@baronfel
Copy link
Collaborator Author

baronfel commented Jan 3, 2025

Hey @YuliiaKovalova / @JanKrivanek - will the solution parser library need to be treated the same as MSBuild libraries for in-proc users (aka MSBuild.Locator-style users)? Meaning setting PrivateAssets/IncludeAssets to ensure the dll doesn't end up in the built output?

@JanKrivanek
Copy link

Hey @YuliiaKovalova / @JanKrivanek - will the solution parser library need to be treated the same as MSBuild libraries for in-proc users (aka MSBuild.Locator-style users)? Meaning setting PrivateAssets/IncludeAssets to ensure the dll doesn't end up in the built output?

It'd be preferable.
We should btw. make this usage explicit with the owning team to stress the need for compatibility guarantees.

@baronfel
Copy link
Collaborator Author

baronfel commented Jan 3, 2025

I was afraid of that :)

That means right now packaging a tool that wants to use the solution parser is very difficult. Today, you can bundle it no problem, but when 9.0.200 or a 9.0.100 with a backport is released you may get assembly binding confusion. I wonder what our official recommendation should be here? Maybe an AssemblyResolve hook that tries to load from the MSBuild location first and then falls back to the app-local bundle?

@JanKrivanek
Copy link

Lets sync on this.

I'm thinking that if we get good enough compatibility guarantees (and sane versioning), than the lowest version shipped with any supported msbuild can be referenced and bundled - so that it's available when (older version of) msbuild doesn't have it loaded, but as well can be satisfied by >= version of already loaded assembly, if (newer version of) msbuild already loaded it.
But I might be missing a lot - so let's discus deeper

@baronfel
Copy link
Collaborator Author

baronfel commented Jan 7, 2025

After discussing with the slnx team and @JanKrivanek, we should be safe to bundle the library for now. Both MSBuildLocator and us try to load dependencies from the MSBuild location first, and then from the app-local directory. As long as the SLNX library keeps binary compatibility (which the team has committed to), then shipping the library in proj-info shouldn't break. Once .NET 10 releases, or .NET 9 releases all contain the SLNX library, then we could mark the dependency as IncludeAssets="compile" and stop shipping it.

@baronfel baronfel merged commit ed11093 into main Jan 7, 2025
3 checks passed
@baronfel baronfel deleted the replace-sln-parser branch January 7, 2025 15:35
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.

2 participants