Skip to content
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

Dependency on local copies of CommandLine libs #18

Open
xorets opened this issue Jun 5, 2023 · 16 comments
Open

Dependency on local copies of CommandLine libs #18

xorets opened this issue Jun 5, 2023 · 16 comments

Comments

@xorets
Copy link

xorets commented Jun 5, 2023

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
Copy link
Owner

Drizin commented Jun 5, 2023

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:

  • build-external.ps1 is what initializes the required git submodules, including my fork of System.CommandLine.
  • build-core.ps1
  • build-models.ps1
  • build-tools.ps1
  • build-visualstudio.ps1

@xorets
Copy link
Author

xorets commented Jun 6, 2023

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
Copy link
Author

xorets commented Jun 6, 2023

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
Copy link
Owner

Drizin commented Jun 6, 2023

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
Copy link
Owner

Drizin commented Jun 6, 2023

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
Copy link
Author

xorets commented Jun 6, 2023

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
Copy link
Author

xorets commented Jun 6, 2023

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
Copy link
Owner

Drizin commented Jun 6, 2023

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
Copy link
Owner

Drizin commented Jun 6, 2023

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
Copy link
Owner

Drizin commented Jun 6, 2023

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
Copy link
Author

xorets commented Jun 6, 2023 via email

@Drizin
Copy link
Owner

Drizin commented Jun 7, 2023

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
Copy link
Author

xorets commented Jun 7, 2023

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
Copy link
Owner

Drizin commented Jun 7, 2023

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
Copy link
Author

xorets commented Jun 7, 2023

Maybe something around IConsole?

Drizin added a commit that referenced this issue Jun 8, 2023
…ed to 3.0.1:

3.0.0 versions will be deprecated, since they were using CodegenCS.Models 3.0.0 which was also deprecated because it was published with a bad dependency - private nuget
So basically the build was failing unless you build all submodules. Refs #18
Drizin added a commit to CodegenCS/Templates that referenced this issue Jun 8, 2023
… to 3.0.1 (3.0.0 nuget had a bad dependency - Refs Drizin/CodegenCS#18).

Updating csprojs  to net6.0 (but they should run in any version - from net472+, netcoreapp2.0+, and net5.0+)
@Drizin
Copy link
Owner

Drizin commented Jun 24, 2024

FWIW now templates can be written using async Tasks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants