search

AArnott / CodeGeneration.Roslyn

Assists in performing Roslyn-based code generation during a build.
Microsoft Public License
408 stars 59 forks source link

Build Target/Task simplification #83

Closed amis92 closed 6 years ago

amis92 commented 6 years ago

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:

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 calls dotnet-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.

AArnott commented 6 years ago

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. In the meantime, it looks like one of the two PRs I completed this morning created a conflict with your change.

AArnott commented 6 years ago

Thanks for responding to my review. Can you resolve the conflicts that remain?

amis92 commented 6 years ago

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?

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.

amis92 commented 6 years ago

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?

image

amis92 commented 6 years ago

More fixes:

Fix logging levels after c# task removal

amis92 commented 6 years ago

And portable targets for BuildTime package are in. Also, upgraded MSBuildSdkExtras usage in Attributes.

I think that'd be all. :)

amis92 commented 6 years ago

@AArnott so what's your final decision? Does it go in, or do we stay with the C# task?

amis92 commented 6 years ago

@AArnott pinging again :)

amis92 commented 5 years ago

For reference: https://github.com/AArnott/CodeGeneration.Roslyn/issues/102#issuecomment-426664994