Closed ThaDaVos closed 1 year ago
ThaDaVos
commented
1 year ago To be more precise - hooking into here: https://github.com/AaronRobinsonMSFT/DNNE/blob/570d39ef439d54eaa6c68ca423f17a3753eeb6b7/src/dnne-gen/Generator.cs#L311
That's all I need to do and then export my own files 🤣
AaronRobinsonMSFT
commented
1 year ago is it okay if I borrow/look into the generation part of this project?
@ThaDaVos DNNE is MIT, but honestly it could be public domain as far as I am concerned. Feel free to copy code if that is helpful or suggest extension points for dnne-gen. I wrote dnne-gen specifically to be used independently if desired so if there is an extension point that would help you suggest it and we can discuss the best way to move forward.
ThaDaVos
commented
1 year ago Thanks! I'll first try to add it straight into the source code of dnne-gen so I can check what kind of information I need to generate the files I need - based on that I can give some recommendations on how we can move forward and open up dnne-gen to also generate the files for consumption
ThaDaVos
commented
1 year ago I am still working on this - but to better support a scenario I made changes inside the MSBUILD part by adding a new PROP - but it seems it's not picked up - do I need to seperatly build something? (A contribution guide could help a lot)
ThaDaVos
commented
1 year ago Got the above working - was looking in the wrong place - needed to pass it to the generator, not the compiler so had to add some conditions etc to DNNE.targets file was enough
ThaDaVos
commented
1 year ago @AaronRobinsonMSFT - I am finally done with all the changes and extra
The things I did:
Generator.cs to separate files to get a clear overview of the files usedGenerator.cs class regarding reading of the Assembly to a seperate class called AssemblyReader and introduced a few new types to keep the hierarchy in syncC99Generator which prefixes certain stuff with the resolving classname to make it easier to create class based wrappers in other languages and prevent conflicts if multiple classes export methods with the same name (this way the library resolves this, instead of the user/developer needing to do this, thinking back to the ease-of-use discussion we participated in a few weeks ago)All can be found here: https://github.com/ThaDaVos/DNNE
If you want a PR to ease the comparison, please ask - but it shouldn't be merged as it contains code specifically for Clarion
AaronRobinsonMSFT
commented
1 year ago @ThaDaVos Thanks for the example. It looks reasonable to me. The idea of a plugin system is nice and it seems you've got a reasonable architecture. I think all plugins would need to exist in DNNE itself however. If not, a loading mechanism and SDK would need to be designed. If you want I would move all language specific code into respective sub folders (that is, C99/ and Clarion/) and we can figure out the right UX for triggering those generators via MSBuild.
I'm also happy to accept the work even if the Clarion support isn't included.
ThaDaVos
commented
1 year ago Something like in my latest commit?
I cleaned up the code, moved everything which seemed correct to their own namespaces and added support for selecting the generators one wants to use.
As you said, it's probably best to have this included in the base of DNNE or maybe eventually through some plugin like system like with nuget packages which register their own generators, like DNNE.Clarion for example.
Also, if this is included in the base code, other people can benefit from it also
ThaDaVos
commented
1 year ago Just thinking - currently I've got two C99 generators in there - as when you gonna generate Class wrappers (for example Clarion) you'll end up with conflicting Export names, so DNNE takes care of this for you - but maybe the functionality in which the Class generator version differs, would be a great default instead of having the possibility to generate conflicts and have compile errors due that? Helps with the ease of use of the package and feels like a more logical default - if you want I can generate a .g.c file for each scenario so you can compare
ThaDaVos
commented
1 year ago Here are the diffs:
ThaDaVos
commented
1 year ago Latest commits contain a new Generator which is help full for creating new generators as it writes the collected data to an XML file - sadly, a few properties had to be ignored as they gave errors regarding Hexadecimal values.
Maybe you can shed some light on something, but how can I get the following attribute value?
I can get the UnmanagedCallersOnly but I am interrested in the return: MarshalAs for my clarion code (this is just a test example) - as I want to change certain the used return type - or wrap it, based on that attribute (or I'll create a custom attribute for it) but I can't find any way to access it - the current GetCustomAttributes does not return it, it seems
ThaDaVos
commented
1 year ago I figured the return part out - it's a parameter on Index -1
AaronRobinsonMSFT
commented
1 year ago @ThaDaVos I am about to be on vacation for a few weeks. I'll try to check this out at some point. Overall I like the direction and I think your suggestion about adding the class to the export names is a good start, but I think it needs to include the namespace too. I agree it should be an option and not the default, you already have that. I would rename the new property, DnneWrapWithClasses, to DnneFullyQualifyExport. Please add an example in the comments. Something simple like MethodName if false, Namespace_Class_MethodName if true.
ThaDaVos
commented
1 year ago First of, have a nice vacation @AaronRobinsonMSFT - there are more changes coming - I'll commit them as soon as possible - but in short:
AttributorsDnneAdditionalGenerators supports * now to select all generatorsArgumentTypes and ArgumentNames into a single struct and listThaDaVos
commented
1 year ago Maybe we should convert this to a discussion?
ThaDaVos
commented
1 year ago Some more info - tried to get exporting of an xsd working - but cannot get the Assembly to load - eventually I figured out this was due the DNNE.dll being run as 64-bit and my project being 32-bit.
But do you know how I can extract a type from my DLL inside DNNE so it can be used for Type based contracting? In this example XSD generation (you can check my fork for what i've been doing)
ThaDaVos
commented
1 year ago @AaronRobinsonMSFT
Something cool I just figured out - you can remove the:
<Platforms>x86</Platforms>
<PlatformTarget>x86</PlatformTarget>
<RuntimeIdentifier>win-x86</RuntimeIdentifier>And use only
<DnneRuntimeIdentifier>win-x86</DnneRuntimeIdentifier>This will make your Library DLL be AnyCPU but the DNNE native library be targeted to win-x86 - it works for me in Clarion - also, it seems to be easier to deal with
ThaDaVos
commented
1 year ago I've got XSD generation working also - needed two changes, one is the above change so the DNNE.dll and project.dll is the same and second I had to increase the target of DNNE to dotnet7 - because I was getting System.Runtime not found errors
AaronRobinsonMSFT
commented
1 year ago second I had to increase the target of DNNE to dotnet7
Which part of DNNE? An important goal here is to ensure support for all currently supported .NET runtimes.
there are more changes coming - I'll commit them as soon as possible - but in short:
If you are thinking of adding this logic to DNNE officially it is probably a good idea to start a PR so we can review the changes. Even the first past of changes from some time ago contained a substantial number of changes that will be difficult to review if the PR gets too big.
ThaDaVos
commented
1 year ago Which part of DNNE? An important goal here is to ensure support for all currently supported .NET runtimes.
I increased the dotnet version for dnne-pkg and dnne-gen - this was needed else the Assembly of the compiled managed binary cannot be loaded for XSD (as you need to extract the types).
Maybe, if you want to support all currently supported .NET Runtimes - the package needs to be made for each version if you want to support stuff like loading the Assembly in a generator
If you are thinking of adding this logic to DNNE officially
Some of the logic would be great to have added to DNNE officially to ease the addition of generators and logic for other programming languages - my current version gave me a great insight in how it works and how we could restructure the project to allow this - but it may be better if I did it from a clean fork and not include language specific implementations - but maybe we first have to discuss which pattern we want to allow for extending DNNE.
We could take a look at how Fody is setup (the configuration file way maybe easier for people too)
AaronRobinsonMSFT
commented
1 year ago @ThaDaVos I'm going to close this for now. If you decide to integrate your changes we can discuss strategy in a PR.
ThaDaVos
commented
7 months ago @AaronRobinsonMSFT - sorry for commenting on a closed ticket, still playing around with a fork - but got a question, I see you used the low-level MetaData reader, the thing is, documentation on it isn't that highly available - would it be a good option to convert to something higher level, to ease extending? https://learn.microsoft.com/en-us/dotnet/standard/assembly/inspect-contents-using-metadataloadcontext
Or would I be better of creating my own new package for that?
My fork: https://github.com/ThaDaVos/DNNE
AaronRobinsonMSFT
commented
7 months ago the low-level MetaData reader, the thing is, documentation on it isn't that highly available
The Metadata is an officially documented specification and the API follows that specification precisely. I would avoid trying to abstract away the metadata details as they aren't always expressed precisely in the higher level APIs you're point out.
Is there some aspect of Metadata that is causing you grief? I'm happy to help elucidate any areas of confusion.
ThaDaVos
commented
7 months ago The thing which is causing me grief - is getting the values of const fields - the only thing I can find is something about a blobreader
ThaDaVos
commented
7 months ago Got the retrieval of values working, currently trying to figure out what the TGenericContext is used for and what it should be - can't find any documentation about it and only see it used in the System.Reflection.Metadata.ISignatureTypeProvider and second generic type
AaronRobinsonMSFT
commented
7 months ago The thing which is causing me grief - is getting the values of const fields - the only thing I can find is something about a blobreader
Yes, Blobs fully defined in ECMA-335. They are typically strings, byte arrays, primitives, or Guids. This requires using a Blob index into the Blob table and decoding the bytes appropriately.
currently trying to figure out what the TGenericContext is used for and what it should be
The ISignatureTypeProvider is used for participating in a callback mechanism for parsing. For DNNE and most other interop related scenarios this is likely unnecessary. You can look at the use of ISignatureTypeProvider in DNNE for converting .NET types into C99 types.
ThaDaVos
commented
7 months ago @AaronRobinsonMSFT - I checked your C99TypeProvider, just trying to understand how it works and what everything means, that's why i asked what the TGenericContext generic parameter is for, as you're using an empty class in there - and I was curious as to why?
Also, one thing I think is a bit stupid - is that the ISignatureTypeProvider doesn't allow one to specify the type to be returned separatly from the type provided - like you can only say string - so you get a string and have to return a string - you can't say, "Give me a string and I'll return a type or an enum value" or something like that
AaronRobinsonMSFT
commented
7 months ago you're using an empty class in there - and I was curious as to why?
Because I need to provide something to handle the generic context that may be passed around. Since I am not using it, it can be an empty class. I recommend debugging through it with some signatures to get a sense of the semantics.
ThaDaVos
commented
7 months ago I've got everything working except the Generic part - would really love to find a way to resolve the generic parameter - but the only parameters one seems to get are the Generic Context and the index of the parameter - but no single reference to the method and such - so I am guessing the Generic Context needs methods or something to make this work - but I cannot find anything about needing to implement an interface - you've got any idea @AaronRobinsonMSFT ?
jkoritzinsky
commented
7 months ago @ThaDaVos basically you need to define your own type that contains the generic context for your scenario (ie, generic arguments for the containing class and the method). You'd instantiate your type using the typeDef and methodDef objects available around here:
Then when parsing the signature, you pass your object in here:
Then, your object will be passed back to the methods as the signature is decoded, enabling you to look up the generic types.
System.Reflection.Metadata allows you to provide your own representation for the generic context as it has no requirements internally.
ThaDaVos
commented
7 months ago Thanks for the info @jkoritzinsky - but I don't see how to helps with resolving the Generic parameters, as the TypeResolver itself, in it's callbacks, doesn't supply the needed info - only the TGenericContext and an Index:
public string GetGenericMethodParameter(TGenericContext? genericContext, int index)
{
return "NOT_IMPLEMENTED";
}
public string GetGenericTypeParameter(TGenericContext? genericContext, int index)
{
return "NOT_IMPLEMENTED";
}Which results in public string GetGenericInstantiation(string genericType, ImmutableArray<string> typeArguments) having as typeArguments parameter containing a value of NOT_IMPLEMENTED - which means, to resolve the typeArguments i need to somehow figure them out in above mentioned methods.
For example, in my fork, I have the following debugs logs for the following type (Using VS Code Logpoints):
GetGenericMethodParameter | Requested a generic method parameter on index: 0
2
GetTypeFromReference | KnownType: "CLASS" | Type: 18
GetGenericMethodParameter | Requested a generic method parameter on index: 0
GetGenericInstantiation | KnownType: "CLASS" | Type: "CLASS" | KnownArgs: "UNKNOWN" | args: "NOT_IMPLEMENTED"
GetGenericInstantiation | KnownType: "CLASS" | Type: "CLASS" | KnownArgs: "CLASS" | args: "CLASS"
GetTypeFromReference | KnownType: "VALUETYPE" | Type: 17
Method[NOT_IMPLEMENTED]: RunThe type inspected:
public TResult Run<TResult>(Func<Task<TResult>> taskFunc, CancellationToken token = default(CancellationToken))
{
}You can update the AbstractTypeResolver type in your fork to require that TGenericContext implements some interface and then you can have those methods call the methods on the generic context object to determine how to build the string signature.
With System.Reflection.Metadata (which DNNE is built on) you need to do most of the work yourself.
DNNE here doesn't need the generic context as it doesn't support generics, so it doesn't need to add constraints or do anything here.
If you're adding generics support, then you need to wire up these pieces to work.
ThaDaVos
commented
7 months ago @jkoritzinsky - thanks for the insight, but I think my question/goal is not clear to you - I am trying to implement the Generic support, but I cannot find any guidance on how to do it - no explanations or spec regarding it - the only thing I have, is the order of the methods being called in, per method it's trying to decode - which doesn't help at all, as the order isn't logical.
But I'll add a test project to the repo, which can be used for testing the dnne-gen part on, this way I won't have many types to look through and it may be easier to see what's happening - as currently I was testing it against a library I already have written.
I love DNNE this far for my Clarion based projects - now I want to take it a step further and also generate the necassary files for my Clarion projects (mostly wrappers and stuff for interactions with the DLL) - is it okay if I borrow/look into the generation part of this project?
Or maybe, we could extract the generation part in a generic way, maybe make it hookable, so one can generate additional files based on the collected methods and signatures (as this is the part I really need to generate the consuming side).