Drizin / CodegenCS

C# Toolkit for Code Generation (T4 alternative!)
MIT License
223 stars 30 forks source link

Dependency on local copies of CommandLine libs #18

Open xorets opened 1 year ago

xorets commented 1 year ago

I tried to make my own code-gen project by following "SimplePocos" example, but the project cannot be compiled because the library depends on two local copies of the CommandLine project libraries:

    <PackageReference Include="System.CommandLine" Version="2.0.0-codegencs" />
    <PackageReference Include="System.CommandLine.NamingConventionBinder" Version="2.0.0-codegencs" />

Is it possible maybe to update it to the latest published beta4 version?

Drizin commented 1 year ago

I'm sorry for the poor/outdated documentation.

The latest version of the sample template SimplePocos is here - and it doesn't require you to rebuild our forked version of System.CommandLine. All nuget dependencies are published. (To run templates like SimplePoco you don't need to rebuild the library and toolkit).

If you need to rebuild all libraries and tools, there are PowerShell scripts in the src folder. The right order would be:

xorets commented 1 year ago

Thank you for fast response! I was able to run the template using dotnet-codegencs CLI, but also would also have intellisense when I author the template. This forces me to reference your DLLs and it doesn't seem to be possible because of those dependencies.

Unless I do something wrong.

xorets commented 1 year ago

I also stumbled upon a confusion around ILogger interface. Judging by name I expected the standard .NET logger, while it turned out to be your custom one. Another confusion is that the interface have only async methods, but is used synchronously everywhere across the template.

Drizin commented 1 year ago

Thank you for fast response! I was able to run the template using dotnet-codegencs CLI, but also would also have intellisense when I author the template. This forces me to reference your DLLs and it doesn't seem to be possible because of those dependencies.

Did you open SimplePocos.csproj in Visual Studio? Did it restore the nuget references? And yet you don't get intellisense?

Drizin commented 1 year ago

I also stumbled upon a confusion around ILogger interface. Judging by name I expected the standard .NET logger, while it turned out to be your custom one.

Microsoft ILogger takes only a regular string, which doesn't allow messages to use colors, that's why I've used my own. But yeah, probably I should add support for injecting a regular MS ILogger.

Another confusion is that the interface have only async methods, but is used synchronously everywhere across the template.

Despite having async definition (I forgot the reason), the methods are all sync, so it doesn't matter if you await the task or not. And in case it makes it easier for users who are not familiar with async/await (template entry point can be sync or async).

xorets commented 1 year ago

Did you open SimplePocos.csproj in Visual Studio? Did it restore the nuget references? And yet you don't get intellisense?

In fact I created my own project with the same dependencies as yours and it doesn't compile. Now tried the same with your solution and see the same build error. (Just in case I use Rider, but I don't expect any difference in build behavior).

image

xorets commented 1 year ago

Microsoft ILogger takes only a regular string, which doesn't allow messages to use colors, that's why I've used my own. But yeah, probably I should add support for injecting a regular MS ILogger.

Yes, I know that it doesn't support colors. I use Spectre.Console myself to get the nice output. But it may be for a reason - logging is not the same as console output, in general case logs should not contain ANSI-sequences.

In my opinion, it is better to rename your class somehow. Otherwise it would be always a confusion.

The same is valid for *Async methods. If they are sync, they must not follow async naming rules and must not return Task. Otherwise it is confusing and may repel users.

Sorry for being picky! :) I think it's a great project, and would like to see it go further.

Drizin commented 1 year ago

I forgot the reason

I just remembered: logs can go to console (which doesn't need async) but they can also go to Visual Studio Output Window (which needs to be async). So different loggers (one being async) implement the same interface - that's why the interface is async. And as I've explained before, the examples are not awaiting because they don't need to. When the template is invoked using the CLI the logger implementation returns immediately, and if the template is invoked through VS Extension then the unawaited call also work fine.

To sum, while templates are running they can update their progress status - this is what is called logging here. Even though it's currently not logging into text files, it's logging into other sinks (console or VS output). Hope it makes sense.

Thank you for your suggestions and feedback.

Drizin commented 1 year ago

Now tried the same with your solution and see the same build error

In the past I've published a nuget package which was refering to this private nuget dependency (System.CommandLine* v codegencs-2.0.0) (I think I've removed and marked as deprecated version), but I don't think that's the case here (assuming you're using latest CodegenCS.Runtime - it's using this System.CommandLine* 2.0.0-beta4.22272.1 that you've mentioned). If you check YourProject.deps.json you might be able to find who is refering to this private nuget. And maybe updating references might fix this bad-dependency.

Drizin commented 1 year ago

I created my own project with the same dependencies as yours

I've missed that. So.. probably you're pointing to the CSPROJ instead of using the NUGET, right? In this case, CSPROJ will need those local versions. So.. getting back to my first reply - did you try the build-external.ps1? It should build all forked versions that you need.

xorets commented 1 year ago

Rick, I tried to open your solution where SimplePoco is located. Got the same result. The screenshot I provided us from an attempt to build your solution.

Custom build scripts won't help here because the IDE builds sources itself in background. It cannot get all necessary dependencies (your private build of Command Line library is transitively referenced by your Runtime library) and falls.

If you cannot link to the officially released command line package, then maybe you can bundle your forked version to your library? Likely it will solve the problem.

On Tue, Jun 6, 2023, 19:25 Rick Drizin @.***> wrote:

I created my own project with the same dependencies as yours

I've missed that. So.. probably you're pointing to the CSPROJ instead of using the NUGET, right? In this case, CSPROJ will need those local versions. So.. getting back to my first reply - did you try the build-external.ps1? It should build all forked versions that you need.

— Reply to this email directly, view it on GitHub https://github.com/CodegenCS/CodegenCS/issues/18#issuecomment-1579176656, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA2ZVO2OV7BXFUGESQ4PSLXJ5RZNANCNFSM6AAAAAAY3IBPS4 . You are receiving this because you authored the thread.Message ID: @.***>

Drizin commented 1 year ago

Ok, I found out what happened:

CodegenCS.Models 3.0.0 was published with those bad references (System.CommandLine private version). After a few days, I noticed the problem and republished 3.0.1 fixed, then I "unlisted" package 3.0.0. But it's still there (not sure if I can totally delete it).

Since CodegenCS.Models.DbSchema 3.0.0 (which is still the latest) was built using CodegenCS.Models 3.0.0, it still has this transitive dependency of the private version.

Quick fix: add <PackageReference Include="CodegenCS.Models" Version="3.0.1" />. Then build should work (in obj/project.assets.json you won't find the bad reference anymore).

xorets commented 1 year ago

Yes, it helped. Thank you very much!

But please consider my opinion around ILogger confusion and the correct usage of "*Async" pattern. It will make the library much easier to use in other apps.

Drizin commented 1 year ago

Sure thing. How would you call that class to avoid this confusion? I'll also investigate if I can replace my colorful console by the aforementioned Spectre.Console.

xorets commented 1 year ago

Maybe something around IConsole?

Drizin commented 1 week ago

FWIW now templates can be written using async Tasks.