-
Notifications
You must be signed in to change notification settings - Fork 59
Conversation
Looks interesting. I didn't used to have a CLI tool at all, but as you say, going cross platform (or even just working with multiple versions of MSBuild and roslyn) necessitated it. So going "all the way" may be worthwhile. I look forward to reviewing this--hopefully soon. |
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.
I'm a bit on the fence about this change. I find C# easier to read than MSBuild syntax when we're doing a lot of variable manipulation.
Is the real win here that we can repeatedly build this repo without locked file errors causing the build to fail?
@@ -1,4 +0,0 @@ | |||
<?xml version="1.0" encoding="utf-8" ?> | |||
<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
<Import Project="..\$(MSBuildThisFileName)$(MSBuildThisFileExtension)" /> |
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.
Have you confirmed that with the removal of all these files that the resulting package still correctly installs into projects with all these target frameworks?
I added them because (at least at the time) nuget would consider a top-level build file as a net40 targeted file, so it wouldn't add the import to net20 or other target framework projects.
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.
I've confirmed that (after my incoming fixes) it works for net20, net40, net45, netstandard1.0 and netstandard 2.0. Portable targets aren't supported, but it can be probably done, if necessary.
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.
It'll probably require using MsBuildSdkExtras package for multi-targeting portables
ArgumentSyntax.Parse(args, syntax => | ||
{ | ||
syntax.DefineOption("version", ref version, "Show version of this tool."); |
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.
"...of this tool (and exits)."
That way it's clear it's not really an option, but a command that ignores anything else on the command line.
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.
Good point.
<PackagePath>build\</PackagePath> | ||
</None> | ||
</ItemGroup> | ||
<Message Text="None now includes: @(None->'%(Identity) Pack:%(Pack) PackagePath:%(PackagePath)')" Importance="high"/> |
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.
I'm guessing this <Message>
was for your own diagnostic purposes and shouldn't be retained in the PR.
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.
Indeed. Will be removed.
<XmlPoke XmlInputPath="$(PropsFilePath)" Value="$(PackageVersion)" Query="/dn:Project/dn:ItemGroup/dn:DotNetCliToolReference/@Version" Namespaces="$(_ProjectNamespaces)" /> | ||
<ItemGroup> | ||
<None Remove="build\$(PropsFileName)" /> | ||
<None Include="build\$(PropsFileName)"> |
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.
Why remove and include when you can Update?
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.
Updating doesn't work for me. When I do just Update, all items in group have Pack set to false, instead of only the one.
Also it's very restricted as per docs: https://docs.microsoft.com/pl-pl/visualstudio/msbuild/item-element-msbuild#attributes
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.
Hmmm.... it seems like it ought to have worked here. All the "restrictions" are satisfied in this case as far as I can tell. Oh well, thanks for trying.
</Compile> | ||
</ItemDefinitionGroup> | ||
<ItemGroup> | ||
<!--This item's Version metadata is updated to match the package's one in .csproj--> |
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.
Can you elaborate on this comment to specify that the substitution is done at build-time by the SetPackageVersionInProps
target?
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.
Will do.
Thanks for responding to my review. Can you resolve the conflicts that remain? |
* drop build output (empty dll) from package * add net20 target * clear up build contents handling * mark as development dependency * set MinClientVersion to 2.5 as it's the first handling build/ contents * add printing tool version at normal importance
I think it's the biggest practical win. I think it might be a little less readable, but since it follows a pattern found in MS XmlSerializer Generator, and drops the size of the package considerably (2MB->4kB), it's worth it. |
There's also an issue that I'm not sure how to resolve, namely the files that the build additionally produces (response file and txt file with generated files list) aren't deleted nor overwritten by future builds. Probably it'd be best to make these file names static, but also avoiding any conflicts... How would one do such thing? Taking inspiration from existing C# pipeline, probably suffixing project name would suffice? |
* Normalized .targets property names * Static .rsp and FileList.txt file names * normal importance of Exec console outputs * MSBuild-recognized format of logs from Logger * add actually empty files to CompilationGenerator.EmptyGeneratedFiles
More fixes: Fix logging levels after c# task removal
|
And portable targets for BuildTime package are in. Also, upgraded MSBuildSdkExtras usage in Attributes. I think that'd be all. :) |
@AArnott so what's your final decision? Does it go in, or do we stay with the C# task? |
@AArnott pinging again :) |
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.
Thanks!
For reference: #102 (comment) |
I've been browsing Microsoft's XML Serializer Generator tool: https://github.com/dotnet/corefx/tree/master/src/Microsoft.XmlSerializer.Generator
I've discovered it uses a similar approach of calling a dotnet CLI tool through MSBuild Target, although it does that using simple MSBuild tasks, instead of C# code DLLs, which seems to simplify greatly making it "cross-platform" (it's automatic).
So I've taken to modify this project to work in a similar manner.
One thing which requires explanation for sure:
Sanitize
method in CLI toolProgram.cs
is needed because of multiple empty lines written into response file by MSBuild task.How it works now
Instead of delivering multiple props and targets, only one, universal, is included in NuGet.
.targets
contains a Target that has "inlined" the work that the old DLL ToolTask did. It produces a response file and callsdotnet-codegen
with appropriate arguments.Due to that, whole C# task could be deleted, as well as all target-specific props and targets files.
Also, because there was no need to find DLL's path, plumbing for overriding the path was no longer needed (e.g. in Unit Tests).
I'm interested in your thoughts about that.
Disclaimer: I think it's not yet complete, I haven't tested yet the newly produced package.