-
Notifications
You must be signed in to change notification settings - Fork 59
[RFC] Redesign (UX+extensibility oriented) #162
Comments
Wow. The goals look great. Thanks for writing this up, @amis92! The pipeline plugin mechanism you describe looks... complicated. If this is all based on real requirements folks have, I may be ok with it once I see a sample plugin that fills it and docs to help convince me it won't scare everyone away. Can we still support incremental build and minimize code generation costs? |
Let's fall back a little and try to list out all the requirements that we want from code generation framework. My thoughts:
|
About the redesign, simplicity by default would be a prerequisite. Furthermore, I would try to put as much effort in to join forces with Uno (@jeromelaban and his colleguaes). It might be worth (I think I would prefer that), to start a separate Github organisation for this and some satellite repositories. |
Anyone? |
@Corniel let's first try to define the shape of this abstract new project. If we have something nice to show, it'll have more chance to gain interest of others. So let me ask: how would you expand on this simplicity you mentioned? As in, first thing that comes to your mind, what currently complicated thing would you like to see simplified. |
public enum CompilationStep
{
PreCompile,
PostComile,
}
[AttributeUsage(AttributeTargets.Class, Inherited = false, AllowMultiple = false)]
public abstract class CodeGenerationAttributeAttribute<TGenerator> : Attribute
where TGenerator : ICodeGenerator
{
protected CodeGenerationAttributeAttribute(CompilationStep compilationStep);
}
public interface ICodeGenerator
{
Task<GeneratedCode> GenerateAsync(CodeGenerationContext context);
} The context should have direct access to the attribute (values) that triggered the code generation, and the GeneratedCode class should be flexible on where to write the generated code to. The current contract of It might be possible to privide hook on points on the build pipeline too, but I think that @AArnott hinted already that in most (90%+ of the) cases, you do not need that. |
I like the idea of GeneratedCode result class, that could open up possibilities. As per the attribute, we can't implement it like you've shown, because assembly where attribute is defined doesn't need to reference the generator assembly. This'd introduce dependency on the generator DLL, and thus limit consuming code's TargetFramework to be compatible with generator's. That's why current approach is needed (typeof or string). The attribute is available to generators currently via constructor argument. It could be part of the context, but it's on a cosmetic level, I'd say. I'm also not sure what CompilationStep would impact. |
@amis92 It is (or could be) cosmetic, but we're talking about simplicity, and this would help getting stuff right. The depedency on the TargetFramework is a big downside. Uno has |
Yes I think it would be a nice simplification. (attribute in context instead of in ctor) Uno's GenerateBefore/After is actually a bit more advanced, actually. It requires multiple compilation generations/tiers. Example: GeneratorA is annotated as GenerateBefore(GeneratorB). Thus, first compilation is done with the non-generated files, GeneratorA is run, and then second compilation with both original and first-generation generated files is created, and GeneratorB is run on that one, effectively allowing generators to consume outputs of other generators. I don't know if you know MSBuild well, but it's a bit like Before/AfterTargets on Target elements, albeit a bit less complicated. |
@amis92 Claiming that i'm an expert on |
I mean, I've never needed it, but it can be a future enhancement anyway. I'd not say this should be a part of the MVP we'd start with. |
Hello, @amis92 , @Corniel I'm monitoring this project for a while and looking forward for improvements. I think this kind of project could be breaking change in many scenarios. I don't have time to contribute now (maybe in couple of months) but I would like only to point you to this approach https://cezarypiatek.github.io/post/generate-mappings-on-build/. Cezary spotted some drawbacks is project and maybe it would be some interesting for you in on your planning process. For me the most important this to make project more popular apart from architecture is live documentation with all functionality listed and explained – now it is searching in issues discussion and PR’s. Please not give up on this project! |
@bigdnf Thanks for the link. I was aware of Cezary's blog but didn't read that post before. For brevity, I'll comment on listed changes:
That's a no-go. We're (contrary to Uno) definitely not following that path. It's a conscious design decision to reuse existing MSBuild pipeline, instead of re-evaluating it all. For @AArnott's explanation, see #100 (comment)
Well, we're using McMaster's package for the same result, I'd consider it done by PR #156 .
I'd agree, caching/incremental support is not the best it could be.
I really don't think it's complicated?
That might be doable but it's not a high priority issue.
I'd consider this done as well by PR #156 . |
@bigdnf re: documentation, have you seen wiki? I've "recently" put considerable effort there. Also new Readme should be more helpful. |
Thanks @amis92 for brief response. I will not argue about MSBuild because I don’t know internals of the project but I wanted you to be aware of things pointed by author of that blog. As far as documentation is concern it is better now but I still have a filling not everything is covered (for example info on how to log is in some issue – or maybe I’m not searching properly). I will try to follow docs next time I will be using this library and maybe add something if it will be missing. Ps. I don’t know it is connected with MSBuild mentioned on the beginning but recently I had some issue but I was not sure it is an Issue or it is not doable and now it came to my mind again. I had a class that was decorated with attribute to generate other class basing on it – I could get method names, parameters and parameters types but when I tried to get type symbol of parameter predecessor (base class) I get null so it looks like I can’t go deep all class hierarchy. If this looks like an issue I can create one if it is not doable please give me some info. |
Hi. The roslyn repo has a relatively new Source Generators Proposal, different from the original. I'm not sure if any of their thoughts would be useful, but I thought I'd chime in in case you hadn't noticed the new proposal. /goes back to lurking |
The link to the current source generators proposal is here: https://github.com/dotnet/roslyn/blob/features/source-generators/docs/features/source-generators.md |
Concerning the |
Well, the current proposal linked by @AArnott just above your post has a bit more details; it doesn't have much to do with In parallel however, the LDT is discussing allowing All very interesting stuff, definitely out of scope for this project here. |
Riding on the back of #137 through some discussions with @hypervtechnics this is what we came up with.
Terms
First let's define some terms we'll use across this document:
dotnet-codegen
tool invoked from MSBuild targets of this frameworkUX
Let's start with UX because that's a really important part, and we've had many issues with users struggling to get value from this project, because of many factors - unclear usage, shady error messages, complicated installation, etc. etc.
Our goals:
For achieving goal 1 we create an MSBuild Sdk
CodeGeneration.Roslyn.Sdk
. This Sdk will:dotnet-codegen
For achieving goal 2:
CodeGeneration.Roslyn.Plugin.Sdk
which:TargetFramework
used is compatible withdotnet-codegen
.props
and.targets
to the nuget packaging (if needed)CodeGeneration.Roslyn.Abstractions
packageNew framework design
Let's start with the MSBuild target that invokes the CLI tool. No changes there are required. The CLI tool's interface will stay the same or very similar.
Then the CLI tool will pass the arguments into a new component, codenamed "PipelineManager". This component takes responsibility of the rest of the process.
This pipeline process consists of three stages:
Plugin discovery
Plugins are discovered from the
CodeGenerationRoslynPlugin
MSBuild ItemGroup passed into CLI tool. This list contains assembly paths. These assemblies are loaded, and a special assembly attribute is looked-up:[assembly: CodeGenerationRoslynPluginManifest(typeof(FooManifest))]
This new attribute provided in
CodeGeneration.Roslyn.Attributes
package would look like that:Pipeline building
The
CodeGenerationRoslynPluginManifestAttribute.ManifestType
is a type that can implement some contracts. For now we propose a new interfaceIConfigurePipeline
, with more/new added in future (likeIConfigureServices
).The manifest is used to configure the pipeline builder, which then gets built and invoked with the context.
The pipeline is pre-filled with some built-in middleware that:
The last part looks a bit like a service, which we could also implement as ASP.NET Core does, via ServiceProvider and IConfigureServices extension point on PluginManifest. To be considered in future.
Pipeline execution
The pipeline context is instantiated and filled with inputs. The pipeline is built using
builder.Build()
and the delegate returned is invoked with the pipeline context.The delegate is awaited and the process is finished.
Work plan
We'll track the work at https://github.com/hypervtechnics/CodeGeneration.Roslyn/projects/1
and in issues here. The prioritization follows:
dotnet new
template for pluginPlease share your thoughts, concerns, ask questions - we'd like to discuss this before getting dirty in implementation.
The text was updated successfully, but these errors were encountered: