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

Add .NET client generator #5201

Closed

Conversation

joemphilips
Copy link
Contributor

@joemphilips joemphilips commented Apr 20, 2022

This PR adds a new file msggen/fsharp.py which generates a F# files which can be consumed by .NET projects.

It has no external dependency. It can be more user-friendly if I can add deps to NBitcoin and DotNetLightning but I chose simplicity over usability.

I have tested in my own project (both F#, C#). It seems working fine.

Questions:

  • Should I publish it to https://www.nuget.org/ from CI? If so, I can create another PR.
  • Do I have to add documentation somewhere else?
  • The commit 89f76ba is independent of my work and it contains a (possible) bugfix for cln-rpc . Should I create another PR for it?

@joemphilips joemphilips force-pushed the add_fsharp_client branch 4 times, most recently from c223b8b to e68fd95 Compare April 20, 2022 09:14
@cdecker
Copy link
Member

cdecker commented Apr 20, 2022

Awesome work @joemphilips, my F# is pretty much inexistent, so I won't be able to verify this, but it seems to be doing what it's supposed to. The grpc.py file is slightly nicer to read as it does way less string concatenations and uses f-strings more extensively, but using rust.py as a basis is perfectly fine.

I'll cherry-pick 89f76ba into its own PR since I'd like to have that in v0.11.0 whose rc4 gets tagged today. Otherwise this PR just needs some minor python lint fixes and it's perfect to go in.

Regarding nuget, that'd require this repo to get the publication secrets, which might not be something that you want to do. Shouldn't be hard to set up a separate cron job that just takes the master, compiles and publishes it on your end if you don't want to give us access to that repo (which I'd totally understand btw) :-)

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the same proposal of this PR #2223 should this F# version stay in the lightningd/plugin repository that it is more a collection of tools built around core lightning?

@joemphilips joemphilips force-pushed the add_fsharp_client branch 6 times, most recently from 0ee2696 to 2a5ec30 Compare April 21, 2022 04:16
@joemphilips
Copy link
Contributor Author

This looks like the same proposal of this PR #2223 should this F# version stay in the lightningd/plugin repository that it is more a collection of tools built around core lightning?

It's a bit different. since it is a complete analog of cln-rpc, and it is a extension of msggen/ which is intending to generate more than one type of bindings, I think it is more natural to fit in this repository.

Regarding nuget, that'd require this repo to get the publication secrets, which might not be something that you want to do. Shouldn't be hard to set up a separate cron job that just takes the master, compiles and publishes it on your end if you don't want to give us access to that repo (which I'd totally understand btw) :-)

I prefer this repo to have a publication secrets instead of running cron in someplace else. Since it is more maintainable.
I will try create another PR. I will leave comment here if it doesn't work out well.

@vincenzopalazzo
Copy link
Collaborator

It's a bit different. since it is a complete analog of cln-rpc, and it is a extension of msggen/ which is intending to generate more than one type of bindings, I think it is more natural to fit in this repository.

I don't see any difference, in this case, this repository should contain all the models generated in a different language? how I see the problem is that we should give the possibility to the user to use msggen with generator like https://github.com/lnspec-tools/lnspec-codegen.py or https://github.com/vincenzopalazzo/monkeyc

If the autogeneration model is useful we should extract the generator and not keep adding languages that from now on two hears can be unmaintained because the maintainer is no longer interested in it.

since it is a complete analog of cln-rpc

cln-rpc is an official one, like pylightning for the PR #2223

@cdecker
Copy link
Member

cdecker commented Apr 21, 2022

I don't mind having more bindings in the contrib/ directory, as long as they are maintained (yes, we'll do a sweep eventually and kick out unmaintained stuff, I promise), and if it helps people find them easier, that's perfect.

The reason it belongs in the contrib directory is that everything outside that directory is used to build CLN itself or the builtin plugins, which now is the case for cln-rpc and cln-grpc.

@joemphilips likely we'll require an API key to push releases to the nuget repo, please don't include that in a PR but contact us directly so we can set it up as a secret to be used by Github Actions.

@vincenzopalazzo
Copy link
Collaborator

vincenzopalazzo commented Apr 21, 2022

if it helps people find them easier, that's perfect.

Do you think that this can be a little bit confusing for people? because part of the binding are in https://github.com/lightningd/plugins and part here?

BTW, I'm fine too, I'm just trying to find a guideline in my mind to keep this division between languages! A binding that uses msg gen can use the GitHub action to check if the model generation is updated with a cron job, so I think leaving this makes more hard also the in-flight fix from the maintainer because a PR can take time to enter in the master branch.

and also if the maintainer change after a while of the unmaintained job ? or if the repository will kick out before a new maintainer came out?

This is a common case that can happen, like me on https://github.com/laanwj/rust-clightning-rpc

@joemphilips
Copy link
Contributor Author

Here's my opinion about @vincenzopalazzo 's concern...

I think what we should make clear here is which is ideal

  1. Include all json-schema-generated clients into contrib/
  2. keep it loosely coupled by having each client’s own repositories.

2 seems ideal in terms of the maintainability issue that @vincenzopalazzo has mentioned.
Probably the only problem is that msggen is not open for extension, i.e. the client library authors have to fork this repo, edit msggen for their own needs, and keep rebasing it against the master branch.

So probably these are the right way to go?

  1. pack msggen itself as a library and other projects such as cln-rpc, cln-grpc, ClnSharp to work as a consumer of the library. So that adding another generator is a matter of ordinary API consumption.
    or
  2. create intermediate data representation for “msggen model” and other generator parses it as their own needs (I think this is unnecessary overhead, so probably 1 is better. )

In this way I think we can enable consumers to generate wire spec from csv files in a similar way, that means including the functionality of lnspec-codegen into msggen.

I'm not in hurry so if this seems the right way, I'll wait until the msggen gets ready. (I'm not a python specialist so it is hard for me to make that change by myself.)

@vincenzopalazzo
Copy link
Collaborator

vincenzopalazzo commented Apr 22, 2022

Probably the only problem is that msggen is not open for extension

It is easy to abstract an extension for the msggen, just use a Chain Of Responsibility pattern that keeps all the generators in a list with the possibility to generate one or all the code! Like the spec generation mentioned before does.

So probably these are the right way to go?

  • pack msggen itself as a library and other projects such as cln-rpc, cln-grpc, ClnSharp to work as a consumer of the library. So that adding another generator is a matter of ordinary API consumption.
  • create intermediate data representation for “msggen model” and other generator parses it as their own needs (I think this is unnecessary overhead, so probably 1 is better. )

In my view, these points are related, and it is what I'm doing with https://github.com/vincenzopalazzo/monkeyc however the pack message in relative easy comparing the work required to do the work that a monkeyc is born for.

i.e. the client library authors have to fork this repo, edit msggen for their own needs, and keep rebasing it against the master branch.

No! you can have your python implementation here of the generation code, and your F# model in your own repository, you need just to have a scrip in any language you like that clones lightning, generate the F# code and copy this code inside your model directory.

However, if you use the chain of responsibility pattern, you could have the python implementation also in your own repository, and release the extension like the python package.

A very good example of this "extension" model is serde in rust, which provides the interface for decoding and encoding a data model. each serde extension is released like a rust package. i.e: serde_json, serde_toml

Considering that we are relying on code generation to generate the types, we could spend a couple of hours making an abstraction of msggen that work for all

msggen now produces F# request/response from schema.json
files in a similar way to rust.
`importlib.metadata` is only usable in python 3.8+
And using backport described in python-poetry/poetry#273 (comment)
will cause following error in CI.
> "importlib_metadata.PackageNotFoundError: No package metadata was found for msggen"

So lets just keep it simple by not having version information in genrated code.
@joemphilips
Copy link
Contributor Author

No! you can have your python implementation here of the generation code, and your F# model in your own repository, you need just to have a scrip in any language you like that clones lightning, generate the F# code and copy this code inside your model directory.

I finally got what you mean by this paragraph.
So then I suppose I should just drop the commit adding generated F# client and leave it to you?
(I'm not 100% sure how you implement the extension model you've mentioned)

Then this PR is ready, when CI passes.

@vincenzopalazzo
Copy link
Collaborator

I can try to make a draft of it! I put it in my todo list

joemphilips added a commit to joemphilips/lightning that referenced this pull request Apr 23, 2022
According to the conversation in ElementsProject#5201,
generated code for 3rd-party generater should not reside in this repository.
Instead, only generators under `msggen` should be included.

This commit adjusts F# generator to follow the following rule of thumb.

1. no native F# code, only generators, so every primitives are now reside in
   `msggen/fsharp.py` with plain text.
2. no object equivalent to `RPCClient`, only DTOs must be generated.
   Implementing client is now the consumers duty.
joemphilips added a commit to joemphilips/lightning that referenced this pull request Apr 23, 2022
According to the conversation in ElementsProject#5201,
generated code for 3rd-party generater should not reside in this repository.
Instead, only generators under `msggen` should be included.

This commit adjusts F# generator to follow the following rule of thumb.

1. no native F# code, only generators, so every primitives are now reside in `msggen/fsharp.py` with plain text.
2. no object equivalent to `RPCClient`, only DTOs must be generated. Implementing client is now the consumers duty.
@joemphilips
Copy link
Contributor Author

joemphilips commented Apr 23, 2022

I've adjusted my F# generator a bit according to the rule that @vincenzopalazzo has mentioned, see the commit message for 6376342

I'm expecting @vincenzopalazzo to update msggen/__main__.py so that it takes a command line argument to

  1. specify which generator to use. (In a chain of responsibility pattern he mentioned?)
  2. output path for the generated code. (currently it always outputs to contrib/Model.fs)

joemphilips added a commit to joemphilips/lightning that referenced this pull request Apr 23, 2022
According to the conversation in ElementsProject#5201,
generated code for 3rd-party generater should not reside in this repository.
Instead, only generators under `msggen` should be included.

This commit adjusts F# generator to follow the following rule of thumb.

1. no native F# code, only generators, so every primitives are now reside in `msggen/fsharp.py` with plain text.
2. no object equivalent to `RPCClient`, only DTOs must be generated. Implementing client is now the consumers duty.
joemphilips added a commit to joemphilips/lightning that referenced this pull request Apr 23, 2022
According to the conversation in ElementsProject#5201,
generated code for 3rd-party generater should not reside in this repository.
Instead, only generators under `msggen` should be included.

This commit adjusts F# generator to follow the following rule of thumb.

1. no native F# code, only generators, so every primitives are now reside in `msggen/fsharp.py` with plain text.
2. no object equivalent to `RPCClient`, only DTOs must be generated. Implementing client is now the consumers duty.
According to the conversation in ElementsProject#5201,
generated code for 3rd-party generater should not reside in this repository.
Instead, only generators under `msggen` should be included.

This commit adjusts F# generator to follow the following rule of thumb.

1. no native F# code, only generators, so every primitives are now reside in `msggen/fsharp.py` with plain text.
2. no object equivalent to `RPCClient`, only DTOs must be generated. Implementing client is now the consumers duty.
@vincenzopalazzo
Copy link
Collaborator

vincenzopalazzo commented Apr 23, 2022

Much simpler when msggen will be released as a library

def gen_dotnet(generator_chain: GeneratorChain):
    fname = "<your path>"
    dest = open(fname, "w")
    generator_chain.add_generator(DotnetGenerator(dest))

if __name__ == "__main__":
  generator_chain = GeneratorChain()
  gen_dotnet(generator_chain, meta)
  generator_chain.generate(service)

And more important you can ship your DotnetGenerator as python package too, so every one that want use the dot net generator can pull your package with a simple pip3 installed msggen-dotnet

An on flight draft of the solution #5216

joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request Apr 24, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request Apr 26, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request Apr 28, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request Apr 28, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request Apr 28, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request Apr 28, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request Apr 28, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
@joemphilips
Copy link
Contributor Author

Feel free to close this issue since #5216 seems to be more promising approach.

joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request May 2, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request May 5, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request May 6, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request May 18, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
joemphilips added a commit to joemphilips/DotNetLightning that referenced this pull request May 19, 2022
This commit adds a new project for calling c-lightning from .NET
in a typesafe way.
Most of the codes are auto-generated from client generator built in
ref: ElementsProject/lightning#5201

It must be updated after ElementsProject/lightning#5216 gets merged.
So that all the generation logics is defined in this repo.
But this is good enough for temporary solution.
@vincenzopalazzo
Copy link
Collaborator

This PR can be close, right?

@joemphilips
Copy link
Contributor Author

This PR can be close, right?

yes. I will close.

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

Successfully merging this pull request may close these issues.

3 participants