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

Proposal: Use MSBuild's Microsoft.Build.* libraries #87

Closed
teo-tsirpanis opened this issue Sep 14, 2020 · 11 comments · Fixed by #88
Closed

Proposal: Use MSBuild's Microsoft.Build.* libraries #87

teo-tsirpanis opened this issue Sep 14, 2020 · 11 comments · Fixed by #88

Comments

@teo-tsirpanis
Copy link

teo-tsirpanis commented Sep 14, 2020

Currently, dotnet-proj-info works with project files by invoking the command-line edition of MSBuild. This approach becomes unwidely when retrieving information from a project: a temporary target file is generated that specifies the names of the proeprties it needs, then MSBuild is invoked, writing the values of these properties to another temporary file. And this approach isn't perfect, resulting in bugs like #82. Even worse, pieces of the source code are MSBuild is being selectively piggybacked in a pretty brittle and clearly unsupported way, via the internal Dotnet.ProjInfo.Helpers project.

What I propose is to use the official MSBuild API, as offered through packages like Microsoft.Build. The benefits would be enormous. The codebase will be simplified, the performance will be increased, and the project will follow established best practices.

Caveats

This is gonna be a breaking change, more or less, but acceptable for me since we are still in version 0.x:

  • Support for .NET Standard will have to be removed. Only .NET Framework and Core will be supported. Personally I don't consider it a problem; I cannot imagine a .NET project analyzer that needs the ubiquity of platforms that .NET Stnadard can offer but .NET Core 2.x cannot. Besides, .NET 5's time draws near, reducing the framework fragmantation.

  • We have to reconsider the way the project deals with MSBuild installations. Currently it looks for MSBuild executable files, but what we would need are directories with the MSBuild assemblies. We would use the Microsoft.Build.Locator package as recommended by Microsoft. It can find only MSBuild installations from .NET Core SDKs when run on .NET Core, and only from Visual Studio when run from .NET Framework.

    • What is more, the MSBuild locator's implementation relies on hooking assembly resolving events, meaning that we might not be able to use multiple versions of MSBuild per program invocation. Still it seems a pretty exotic scenario for me.
  • To read solution files, we use a package that was last updated three years ago that also piggybacks from MSBuild source code. Using the official solution parser API would also cause trouble because it cannot retrieve the files inside a solution folder, which means that until Solution file parser should parse and preserve ProjectSection(SolutionItems) dotnet/msbuild#1708 is fixed (preferably by Consider contributing the solution folder file reader to the mainstream MSBuild project. enricosada/sln#3), we have to keep using the package we are already using.


I am waiting for your feedback on these proposals. Once some ambiguities (like my second bullet point) are addressed, I will start working on an implementation.

@baronfel
Copy link
Collaborator

I'm entirely in favor of doing something very much like this. Now that these libraries are maintained and made openly available it seems odd to not use them. I understand a worry about not being able to use multiple versions, but this package is pretty core/foundational to f# tooling so it generally stays pretty well maintained. Meaning we shouldn't worry about code rot.

@teo-tsirpanis
Copy link
Author

@baronfel I have a question: should we keep supporting the legacy project files?

Things like getting the compiler arguments for them is complicated. Besides, the SDK-based project files can build .NET Framework assemblies too.

@Krzysztof-Cieslak
Copy link
Member

Support for .NET Standard will have to be removed

We can probably go even further with that and target only Core.

We have to reconsider the way the project deals with MSBuild installations.

Locator thing sounds good - TBF, all we care is finding the correct location from SDK

Breaking change

I'm planning to do a bunch of major releases soon (FSAC 1.0 and Ionide 5.0) so it's probably good idea to do those changes before that.

solution files

I'll message Enrico to transfer the sln repo to Ionide organization.

@teo-tsirpanis
Copy link
Author

should we keep supporting the legacy project files?

We can probably go even further with that and target only Core.

@Krzysztof-Cieslak, I take my last question's answer as a yes.

@Krzysztof-Cieslak
Copy link
Member

It's interesting question... I think it would be nice to support legacy project files but if it's too complex or too much work we can live without it - we've been telling users that legacy project files support is not developed any more, so i feel it may be ok to remove it with next major Ionide/FSAC

@teo-tsirpanis
Copy link
Author

Getting the compiler's command-line arguments from a legacy project is a pretty big deal, involving replacing an MSBuild target - that cannot be easily reproduced using library calls, and two piggybacked files from F#'s sources, so I will not port it.

Still it makes me curious, why we need to get the compiler's arguments in the first place?

@Krzysztof-Cieslak
Copy link
Member

why we need to get the compiler's arguments in the first place

To create correctly something called FSharpProjectOptions which is used by every FCS operation - this is true in case of both legacy and new project files.

@teo-tsirpanis
Copy link
Author

teo-tsirpanis commented Nov 6, 2020

Currently blocked by dotnet/msbuild#5856 microsoft/MSBuildLocator#86.

@Krzysztof-Cieslak
Copy link
Member

I've started the implementation of this in #88. PRs, suggestions and improvements are really welcomed - let's make DPI way better ;-)

@teo-tsirpanis
Copy link
Author

My implementation is on https://github.com/teo-tsirpanis/dotnet-proj-info/tree/msbuild-library. I am not currently working on it because of the blocking bugs.

Jusging from comments in #88, I will entirely remove verbose project files when I start working on it again.

@Krzysztof-Cieslak
Copy link
Member

@teo-tsirpanis, Oh I haven't realized you've started implementing things already, sorry. Can you focus your efforts on the branch I've started in #88?

Currently blocked by microsoft/MSBuildLocator#86.

I've worked around this with - microsoft/MSBuildLocator#86 (comment)

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 a pull request may close this issue.

3 participants