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

Use MSBuild OM for what it's good for #656

Merged
merged 1 commit into from
Jul 21, 2020

Conversation

AArnott
Copy link
Collaborator

@AArnott AArnott commented Nov 21, 2019

To do:

  • Consider replacing use of the evaluation model with the execution model after running a design-time build for more accurate/complete results and even simpler code.
  • Never load an msbuild project within the msbuild task
  • Test that mpc.exe works
  • Test that the MSBuild task works
  • See what else MSBuild can help with
  • Consider what can be fixed from Universal Code Generator references not always correct #583

Fixes #944

@neuecc
Copy link
Member

neuecc commented Nov 22, 2019

when I import Microsoft.Build, refered .NET Framework so I should change GeneratorCore .NET Standard 2.0 to .NET Core 2.1 ?

パッケージ 'Microsoft.Build 16.3.0' はプロジェクトのターゲット フレームワーク '.NETStandard,Version=v2.0' ではなく '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8' を使用して復元されました。このパッケージは、使用しているプロジェクトとの完全な互換性がない可能性があります。

Platform support .NETCoreApp 2.1, .NETFramework 4.7.2
https://www.nuget.org/packages/Microsoft.Build/


There is still a lot of my manual code at this stage,
so I don't see the benefit of replacing Microsoft.Build
(I can't evaluate cross-platform new/legacy csproj reliability of Microsoft.Build)

Ultimately, if my lot of manual collection is no longer needed, I think it's good.

@AArnott
Copy link
Collaborator Author

AArnott commented Nov 22, 2019

Yes, sadly Microsoft.Build itself is not netstandard. But targeting netcoreapp2.1 (or 3.0) for this library may work well. Or you can multitarget netcoreapp2.1 and net472 easily too. I plan to look into this to find a good option.

(I can't evaluate cross-platform new/legacy csproj reliability of Microsoft.Build)

I'm not sure what you mean. Do you mean that Microsoft.Build can't reliably tell you the compile items and references etc of a csproj when its new vs. legacy? It absolutely can. This is the same Microsoft.Build.dll that Visual Studio itself uses and what we use to build at the command line. It's the authority -- it doesn't get any more reliable/accurate than MSBuild itself.

@neuecc
Copy link
Member

neuecc commented Nov 22, 2019

What is used in Visual Studio has no guarantee regarding the safety of Mac/Linux behavior.
It's okay because the contents are just reading the Xml and creating the structure, maybe.
However, due to the confusion such as Workspace, I don't have much trust in these projects.

At present, XmlReader has been changed to a structured Reader, and there are no significant advantages.
Of course, I am glad that the interpretation of Glob and Macro is correct!

@AArnott
Copy link
Collaborator Author

AArnott commented Nov 22, 2019

no guarantee regarding the safety of Mac/Linux behavior

Yes there is. Microsoft.Build.dll is also used with dotnet build on mac/linux. Basically everywhere a csproj exists, MSBuild (and this dll) is what's reading and executing it. So if this Microsoft.Build.dll misinterprets it, the csproj itself is wrong by definition.

As for "no significant advantages", you may feel differently once I make changes to this PR to run a design-time build. It will support a much wider variety of projects automatically than you could ever hope to support by hand-parsing XML.

Copy link
Member

@neuecc neuecc left a comment

Choose a reason for hiding this comment

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

I'm interested in the development, so I want to see the code that follows.

@github-actions
Copy link

github-actions bot commented Apr 9, 2020

This pull request is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@AArnott
Copy link
Collaborator Author

AArnott commented Jun 18, 2020

The line count diff says it all: this is way simpler than what was happening before. And since we're using proper MSBuild/Roslyn APIs the fidelity is super high.

I still need to implement the msbuild task version. And I suppose I need to bring back directory support to mpc.exe. Is that for Unity, @neuecc? Also, what is this support I saw for a comma-delimited list of paths to csproj files for? What's the scenario? Is that roughly equivalent to just running mpc.exe separately for each project file?
If a directory is passed to mpc.exe, is it assumed to be a unity scenario or do we look for csproj files just in case they exist? Do we need to run on all of them or can we assume at most one csproj file per directory?

@AArnott AArnott force-pushed the msbuildOM branch 2 times, most recently from 61b4c04 to 75a184a Compare June 18, 2020 13:55
@AArnott
Copy link
Collaborator Author

AArnott commented Jun 18, 2020

OK, I figured that mpc most likely does support directory scans to support Unity, so that's still supported.
The only real thing that's missing at this point is the new MSBuild task implementation. At the moment, this is documented as expected to be invoked directly from the user's own project. But I think it would be much simpler for the user and allow us greater flexibility in the future if we instead provide our own MSBuild Target that executes the task on the currently building project with all the appropriate inputs either automatically determined or set by msbuild properties in the user's project. Any objection to this design change, @neuecc?

Also: Are the user-supplied preprocessor symbols that mpc allows you to supply only there to support unity scenarios? I'd like to drop support for them where an msbuild project file is given since it's not easy to support and I doubt is relevant except in Unity where no project file exists. Thoughts?

@AArnott AArnott force-pushed the msbuildOM branch 2 times, most recently from fdcdd22 to 9737979 Compare June 21, 2020 23:50
@AArnott
Copy link
Collaborator Author

AArnott commented Jun 21, 2020

I went ahead and implemented the MSBuild task and redesigned the msbuild task nuget package to be self-executing. So now a project may include just this:

  <ItemGroup>
    <PackageReference Include="messagepack" Version="2.1.143" />
    <PackageReference Include="MessagePack.MSBuild.Tasks" Version="2.1.144-g9737979c90" PrivateAssets="all" />
  </ItemGroup>

And it automatically gets mpc run during the build and a file produced in the intermediate directory, consumable in the normal way:

using System;
using MessagePack;
using MessagePack.Resolvers;

class Program
{
    static void Main(string[] args)
    {
        var o = new SomeObject { SomeMember = "hi" };

        var options = MessagePackSerializerOptions.Standard.WithResolver(
            CompositeResolver.Create(
                StandardResolver.Instance,
                GeneratedResolver.Instance
            ));
        byte[] b = MessagePackSerializer.Serialize(o, options);
        var o2 = MessagePackSerializer.Deserialize<SomeObject>(b, options);
        Console.WriteLine(o2.SomeMember);
    }
}

[MessagePackObject]
public class SomeObject
{
    [Key(0)]
    public string SomeMember { get; set; }
}

@AArnott AArnott marked this pull request as ready for review June 21, 2020 23:52
@AArnott AArnott added this to the v2.2 milestone Jun 21, 2020
@AArnott AArnott changed the base branch from master to v2.2 June 21, 2020 23:55
{
// Add this to your C# console app's Main method to give yourself
// a CancellationToken that is canceled when the user hits Ctrl+C.
var cts = new CancellationTokenSource();
Copy link
Member

Choose a reason for hiding this comment

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

We can use this.Context.CancellationToken that provided by ConsoleAppFramework(and GenericHost's Console lifetime)

@neuecc
Copy link
Member

neuecc commented Jul 13, 2020

@AArnott

I think it's very good!
The MSBuildWorkspace.Create, which works with Multiplatform,
is the what I've been wanted for all this time.

Directory scan is, yes, for Unity.
This is because Unity projects often don't have csproj on CI.
(Typically, Unity projects will ignore csproj.)

As for the MSBuild task, I don't know how it will be used (configured).
I'd like to get a sample.

By the way, what is the minimum code of use MSBuildWorkspace?
I can't execute this following code...

<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>netcoreapp3.1</TargetFramework>
    </PropertyGroup>

    <ItemGroup>
        <PackageReference Include="Microsoft.Build.Locator" Version="1.2.6" ExcludeAssets="runtime" />
        <PackageReference Include="Microsoft.Build.Framework" Version="16.0.461" ExcludeAssets="runtime"  />
        <PackageReference Include="Microsoft.Build" Version="16.0.461" ExcludeAssets="runtime"  />
        <PackageReference Include="Microsoft.CodeAnalysis.Workspaces.MSBuild" Version="3.6.0" />
        <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="3.6.0" />
    </ItemGroup>
</Project>
using Microsoft.Build.Locator;
using Microsoft.Build.Logging;
using Microsoft.CodeAnalysis.MSBuild;
using System.IO;
using System.Runtime.Loader;
using System.Threading;
using System.Threading.Tasks;

class Program
{
    static async Task Main(string[] args)
    {
        var instance = MSBuildLocator.RegisterDefaults();
        AssemblyLoadContext.Default.Resolving += (assemblyLoadContext, assemblyName) =>
        {
            var path = Path.Combine(instance.MSBuildPath, assemblyName.Name + ".dll");
            if (File.Exists(path))
            {
                return assemblyLoadContext.LoadFromAssemblyPath(path);
            }

            return null;
        };
        
        // my sample console app
        var projectPath = @"C:\Users\neuecc\Source\Repos\ConsoleApp34\ConsoleApp34\ConsoleApp34.csproj";
        
        var workspace = MSBuildWorkspace.Create();
        var logger = new ConsoleLogger(Microsoft.Build.Framework.LoggerVerbosity.Quiet);
        var project = await workspace.OpenProjectAsync(projectPath, logger, null, CancellationToken.None);
    }
}
Unhandled exception. System.IO.FileNotFoundException: Could not load file or assembly 'Microsoft.Build.Locator, Version=1.0.0.0, Culture=neutral, PublicKeyToken=9dff12846e04bfbd'.

@AArnott
Copy link
Collaborator Author

AArnott commented Jul 14, 2020

Thanks for the review, @neuecc.
Regarding your minimal use sample that doesn't work, the exception you see is because Microsoft.Build.Locator wasn't copied to your output directory, because you included ExcludeAssets="runtime" in the line below:

 <PackageReference Include="Microsoft.Build.Locator" Version="1.2.6" ExcludeAssets="runtime" />

MSBuild and Roslyn has APIs that work xplat, on .NET Framework and .NET Core, that give a high fidelity representation of a CSharpCompilation (and even a VB one) so that we don't have to execute our own design-time builds or parse XML ourselves.
@AArnott
Copy link
Collaborator Author

AArnott commented Jul 18, 2020

I've added a sample in the docs as you requested, @neuecc.
I think this is ready to go.
That said, I'd love to know what multipleIfDirectiveOutputSymbols is useful for so we can drop it if it's no longer useful.

@AArnott AArnott merged commit c99a57c into MessagePack-CSharp:v2.2 Jul 21, 2020
@AArnott AArnott deleted the msbuildOM branch July 21, 2020 22:17
@neuecc
Copy link
Member

neuecc commented Aug 5, 2020

To be honest, I don't understand it at all, but well, I'll think about it after I use it.
(I'm using it in my product, so if there's a problem, I'll give you feedback right away.

@neuecc
Copy link
Member

neuecc commented Aug 5, 2020

In addition, I don't think anyone can use it in this ReadMe.
Most people does not know MSBuild details,
and this document lacks that works only copy-and-paste.

@AArnott
Copy link
Collaborator Author

AArnott commented Aug 5, 2020

@neuecc Thanks for the feedback. I do want this to be easy. The way I've designed it there won't typically be any need at all to modify msbuild files (as opposed to before where they had to add targets and tasks).
But the one small msbuild change they might need to make is define a few properties. Although I document the properties, you're right that I don't give an xml snippet. Do you think #995 resolves your concerns?

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

Successfully merging this pull request may close these issues.

None yet

2 participants