-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Using MSBuild task to run the XmlSerializer directly #117406
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
Conversation
96cab01
to
dc54a42
Compare
<Delete Condition="Exists('$(_SerializerCsIntermediateFolder)') == 'true'" Files="$(_SerializerCsIntermediateFolder)" ContinueOnError="true"/> | ||
<Message Text="Running Serialization Tool" Importance="normal" /> | ||
<Exec Command="dotnet Microsoft.XmlSerializer.Generator "$(IntermediateOutputPath)$(AssemblyName)$(TargetExt)" --force --quiet $(_SgenRspFilePath)" ContinueOnError="true"/> | ||
<UsingTask TaskName="GenerateXmlSerializers" AssemblyFile="$(MSBuildThisFileDirectory)/../lib/netstandard2.0/dotnet-Microsoft.XmlSerializer.Generator.dll" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UsingTask
must be moved outside a Target
node.
public static TextWriter InfoWriter { get; internal set; } = Console.Out; | ||
public static TextWriter WarningWriter { get; internal set; } = Console.Out; | ||
public static TextWriter ErrorWriter { get; internal set; } = Console.Error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be instance fields? Also, an Action<string>
looks like a better abstraction. Even better, you can use a TaskLoggingHelper
here.
|
||
namespace Microsoft.XmlSerializer.Generator | ||
{ | ||
internal sealed class InfoTextWriter : TextWriter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can combine the three TextWriters
with a single one like this:
internal sealed class DelegatedTextWriter(Action<string> fWrite) : TextWriter
{
public override Encoding Encoding => Encoding.UTF8;
public override void Write(string? value)
{
if (!string.IsNullOrEmpty(value))
{
fWrite(value)
}
}
public override void WriteLine(string? value) => Write(value);
}
And then create them in the task like Sgen.InfoWriter = new DelegatedTextWriter(Log.LogError);
.
{ | ||
if (!string.IsNullOrEmpty(value)) | ||
{ | ||
_log.LogMessage(MessageImportance.High, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should decide if the messages should have high importance, or which of them should.
Pack="true" /> | ||
<None Include="build\prefercliruntime" PackagePath="" Pack="true" /> | ||
<None Include="build\Microsoft.XmlSerializer.Generator.targets" PackagePath="build" Pack="true" /> | ||
<None Include="build\dotnet-Microsoft.XmlSerializer.Generator.runtimeconfig.json" PackagePath="lib\netstandard2.0" Pack="true" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make sure a .deps.json
file is included in the package. You can see this guide for the best practices when creating MSBuild task packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've had to abandon this approach because the Task was being run in msbuild running on .NET Framework when building in Visual Studio. We can't do that because we depend on implementation that lives in the .NET runtime to do some of the work. Would including a .deps.json file allow us to run the Task using .NET when building on Visual Studio?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's not yet supported and is tracked in dotnet/msbuild#4834.
private static void WriteHeader() | ||
{ | ||
// do not localize Copyright header | ||
Console.WriteLine($".NET Xml Serialization Generation Utility, Version {ThisAssembly.InformationalVersion}]"); | ||
InfoWriter.WriteLine($".NET Xml Serialization Generation Utility, Version {ThisAssembly.InformationalVersion}]"); | ||
} | ||
|
||
private void WriteHelp() | ||
{ | ||
Console.Out.WriteLine(SR.HelpDescription); | ||
Console.Out.WriteLine(SR.Format(SR.HelpUsage, this.GetType().Assembly.GetName().Name.Substring("dotnet-".Length))); | ||
Console.Out.WriteLine(SR.HelpDevOptions); | ||
Console.Out.WriteLine(SR.Format(SR.HelpAssembly, "-a", "--assembly")); | ||
Console.Out.WriteLine(SR.Format(SR.HelpType, "--type")); | ||
Console.Out.WriteLine(SR.Format(SR.HelpProxy, "--proxytypes")); | ||
Console.Out.WriteLine(SR.Format(SR.HelpForce, "--force")); | ||
Console.Out.WriteLine(SR.Format(SR.HelpOut, "-o", "--out")); | ||
|
||
Console.Out.WriteLine(SR.HelpMiscOptions); | ||
Console.Out.WriteLine(SR.Format(SR.HelpHelp, "-h", "--help")); | ||
InfoWriter.WriteLine(SR.HelpDescription); | ||
InfoWriter.WriteLine(SR.Format(SR.HelpUsage, this.GetType().Assembly.GetName().Name.Substring("dotnet-".Length))); | ||
InfoWriter.WriteLine(SR.HelpDevOptions); | ||
InfoWriter.WriteLine(SR.Format(SR.HelpAssembly, "-a", "--assembly")); | ||
InfoWriter.WriteLine(SR.Format(SR.HelpType, "--type")); | ||
InfoWriter.WriteLine(SR.Format(SR.HelpProxy, "--proxytypes")); | ||
InfoWriter.WriteLine(SR.Format(SR.HelpForce, "--force")); | ||
InfoWriter.WriteLine(SR.Format(SR.HelpOut, "-o", "--out")); | ||
|
||
InfoWriter.WriteLine(SR.HelpMiscOptions); | ||
InfoWriter.WriteLine(SR.Format(SR.HelpHelp, "-h", "--help")); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not required, now that Sgen is no longer a CLI tool.
private static void WriteHeader() | |
{ | |
// do not localize Copyright header | |
Console.WriteLine($".NET Xml Serialization Generation Utility, Version {ThisAssembly.InformationalVersion}]"); | |
InfoWriter.WriteLine($".NET Xml Serialization Generation Utility, Version {ThisAssembly.InformationalVersion}]"); | |
} | |
private void WriteHelp() | |
{ | |
Console.Out.WriteLine(SR.HelpDescription); | |
Console.Out.WriteLine(SR.Format(SR.HelpUsage, this.GetType().Assembly.GetName().Name.Substring("dotnet-".Length))); | |
Console.Out.WriteLine(SR.HelpDevOptions); | |
Console.Out.WriteLine(SR.Format(SR.HelpAssembly, "-a", "--assembly")); | |
Console.Out.WriteLine(SR.Format(SR.HelpType, "--type")); | |
Console.Out.WriteLine(SR.Format(SR.HelpProxy, "--proxytypes")); | |
Console.Out.WriteLine(SR.Format(SR.HelpForce, "--force")); | |
Console.Out.WriteLine(SR.Format(SR.HelpOut, "-o", "--out")); | |
Console.Out.WriteLine(SR.HelpMiscOptions); | |
Console.Out.WriteLine(SR.Format(SR.HelpHelp, "-h", "--help")); | |
InfoWriter.WriteLine(SR.HelpDescription); | |
InfoWriter.WriteLine(SR.Format(SR.HelpUsage, this.GetType().Assembly.GetName().Name.Substring("dotnet-".Length))); | |
InfoWriter.WriteLine(SR.HelpDevOptions); | |
InfoWriter.WriteLine(SR.Format(SR.HelpAssembly, "-a", "--assembly")); | |
InfoWriter.WriteLine(SR.Format(SR.HelpType, "--type")); | |
InfoWriter.WriteLine(SR.Format(SR.HelpProxy, "--proxytypes")); | |
InfoWriter.WriteLine(SR.Format(SR.HelpForce, "--force")); | |
InfoWriter.WriteLine(SR.Format(SR.HelpOut, "-o", "--out")); | |
InfoWriter.WriteLine(SR.HelpMiscOptions); | |
InfoWriter.WriteLine(SR.Format(SR.HelpHelp, "-h", "--help")); | |
} |
Sgen sgen = new Sgen(); | ||
Sgen.InfoWriter = new InfoTextWriter(Log); | ||
Sgen.WarningWriter = new WarningTextWriter(Log); | ||
Sgen.ErrorWriter = new ErrorTextWriter(Log); | ||
|
||
int exitCode = sgen.Run(args.ToArray()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that Sgen is no longer a CLI tool, you should add properties to the MSBuild task and to the Sgen
class, and not construct command-line arguments.
if (Force) | ||
args.Add("--force"); | ||
|
||
if (Quiet) | ||
args.Add("--quiet"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two are always true and we should update Sgen to remove all code that would run if they were false.
<Delete Condition="Exists('$(_SerializerDllIntermediateFolder)') == 'true'" Files="$(_SerializerDllIntermediateFolder)" ContinueOnError="true"/> | ||
<Delete Condition="Exists('$(_SerializerPdbIntermediateFolder)') == 'true'" Files="$(_SerializerPdbIntermediateFolder)" ContinueOnError="true"/> | ||
<Delete Condition="Exists('$(_SerializerCsIntermediateFolder)') == 'true'" Files="$(_SerializerCsIntermediateFolder)" ContinueOnError="true"/> | ||
<Message Text="Running Serialization Tool" Importance="normal" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<Message Text="Running Serialization Tool" Importance="normal" /> |
Not strictly needed, MSBuild binary logs keep track of when tasks start.
No description provided.