myriad icon indicating copy to clipboard operation
myriad copied to clipboard

Plugin activation

Open JordanMarr opened this issue 3 years ago • 23 comments

I think that the way I am activating my "ssdt" Myriad plugin for SqlHydra is a little different from your examples. For example, you usually activate your plugin by marking an F# object with an attribute (i.e. [<Generator.DuCases>]).

But in the case of SqlHydra "ssdt" plugin, my source <MyriadFile> is a .dacpac file.
So far this has worked fine because I only have one plugin in SqlHydra.dll. The issue is that, currently, my plugin will only work if it is the only plugin.

Last night I had some problems when I started creating a second plugin. For starters, the new plugin doesn't even require an input file at all. Really just needs to get some stuff out of the .toml config and then run on build. But there is no workflow currently to handle this kind of plugin since the <MyriadFile> usually points an fs file containing an attribute that will activate a named plugin. In my scenario, I had two plugins, one that points to a .dacpac file, and another that doesn't need an input file at all, and Myriad seemed to be just using the first plugin it could find. (I was able to comment one out which would then allow me to use the other).

So I'm thinking that perhaps both my plugin workflows could be accommodated with an additions to the MSBuild configuration.

Plugin Activation: non-fsharp MyriadFile

This style of plugin relies on a non-fsharp file (i.e. dacpac), and so it needs a way to specify the name of the plugin to run from the MSBuild config. Maybe something like this:

    <ItemGroup>
         <!-- Specify the .fs output file (to be generated by Myriad) -->
        <Compile Include="AdventureWorks.fs">
            <!-- Specify the .dacpac input  file -->
            <MyriadFile>../AdventureWorks/bin/Debug/AdventureWorks.dacpac</MyriadFile>
            <MyriadPlugin>ssdt<MyriadPlugin>
        </Compile>
    </ItemGroup>

Plugin Activation: no MyriadFile

This style of plugin can get all the input information it needs via a toml config section; it does not need an input MyriadFile at all. Maybe this case would just need:

    <ItemGroup>
         <!-- Specify the .fs output file (to be generated by Myriad) -->
        <Compile Include="Generated.fs">
            <!-- No input file needed - just specify the plugin only -->
            <MyriadPlugin>newplugin<MyriadPlugin>
        </Compile>
    </ItemGroup>

JordanMarr avatar Apr 15 '21 13:04 JordanMarr

Currently the CLI input has mandatory input so that would have to be addressed first: https://github.com/MoiraeSoftware/myriad/blob/master/src/Myriad/Program.fs#L14

Actual the MSBuild target property is also mandatory too.

plugins are included via the msbuild injection of props from the nuget generator.props etc.

So its the plugin author who decides if a plugin is applied or not.

7sharp9 avatar Apr 15 '21 15:04 7sharp9

Interesting.. So now I see how you are binding the Plugin arg from MSBuild to CLI here:

<MyriadSdk_Args Include='--plugin %(MyriadSdkGenerator.FullPath)' Condition=" '@(MyriadSdkGenerator)' != '' "/>

That is passing the plugin .dll path to the Myriad CLI -- which I am doing that here in SqlHydra.props.

But I have multiple generators in my plugin dll, so then how can it know which to use? Is MSBuild going to try to execute all of the generators it finds in my dll? Can more than one ItemGroup be configured within a single project? (like if I wanted to use two different plugins in the same project)?

EDIT: Sorry, I guess I can answer that question for myself by looking at your very clean CLI code!

JordanMarr avatar Apr 15 '21 16:04 JordanMarr

All plugins in a dll will get called, fields/dus/lend all share the same dll

7sharp9 avatar Apr 15 '21 17:04 7sharp9

Gotcha. Sorry, I was referring to generators as plugins which was adding to the confusion. All generators get called on build.

JordanMarr avatar Apr 15 '21 17:04 JordanMarr

So then let me try again: Would it make sense to be able to specify a MyriadGenerator since a plugin dll can have multiple generators?

    <ItemGroup>
         <!-- Specify the .fs output file (to be generated by Myriad) -->
        <Compile Include="AdventureWorks.fs">
            <!-- Specify the .dacpac input  file -->
            <MyriadFile>../AdventureWorks/bin/Debug/AdventureWorks.dacpac</MyriadFile>
            <MyriadGenerator>ssdt<MyriadPlugin>
        </Compile>
    </ItemGroup>

JordanMarr avatar Apr 15 '21 17:04 JordanMarr

But I have multiple generators in my plugin dll, so then how can it know which to use?

Thats a good question when theres no input file, the addition of validExtension was added as part of that filter for input files that didn't have a way of attributing what needed generating, I think input file could be empty is the CLI argument was non mandatory and the msbuild condition was removed. I think some sort of modulation of the props might be needed, I did once thing that having key/values in the msbuild could be useful but hoped the toml config might be better.

Compile.MyriadFile
Compile.Myriad.File

Having Myriad as the basic element might make more sense now that fs/any/none could be valid input types

7sharp9 avatar Apr 15 '21 17:04 7sharp9

What I wanted to make sure of was that complexity didn't explode, once you add two ways of doing things the logic starts to twist, we would have 3 ways of invoking a plugin, and three ways of specifying the properties which all have to end up being CLI params.

7sharp9 avatar Apr 15 '21 17:04 7sharp9

    <ItemGroup>
         <!-- Specify the .fs output file (to be generated by Myriad) -->
        <Compile Include="AdventureWorks.fs">
            <!-- Specify the .dacpac input  file -->
            <Myriad>
                <InputFile>../AdventureWorks/bin/Debug/AdventureWorks.dacpac</MyriadFile>
                <Generator>
                    <Name>ssdt</Name>
                    <Namespace>test</Namespace>
                </Generator>
                <!-- Or just text element -->
                <Generator>other</Generator>
            <Myriad>
        </Compile>
    </ItemGroup>```
    
Theres also using the toml file to add all these extra pieces of info:
    

<ItemGroup>
     <!-- Specify the .fs output file (to be generated by Myriad) -->
    <Compile Include="AdventureWorks.fs">
        <!-- Specify the .dacpac input  file -->
        <Myriad>
            <Generator>
                <Config>myGeneratorSection</Config>
            </Generator>
        <Myriad>
    </Compile>
</ItemGroup>

7sharp9 avatar Apr 15 '21 17:04 7sharp9

I think maybe extending the interface so that IsValid is passed some properties and then the plugin author can decide whether its applied of not

IsValid(inputFile: string option, extension: string option, generators: string list) : bool

generators would probably be exclusive if present I guess its up to the plugin to decide based on the inputs passed

7sharp9 avatar Apr 15 '21 17:04 7sharp9

It could also be possible to specify a glob as part of an filename to tell the generator what to do like [id].fs which would give it a one to many output. Im thinking too far ahead anyway, need to reign it in :-)

7sharp9 avatar Apr 15 '21 18:04 7sharp9

The clarity of intent is really high in this example you posted above. I like it!

    <ItemGroup>
        <Compile Include="AdventureWorks.fs">
            <Myriad>
                <InputFile>../AdventureWorks/bin/Debug/AdventureWorks.dacpac</InputFile>
                <Generator>ssdt</Generator>
            <Myriad>
        </Compile>
    </ItemGroup>

Things I like about it:

  • It doesn't need my comments anymore because it's very obvious that Myriad will be generating the file, and it's obvious that all the elements within <Myriad> are configuration settings.
  • <InputFile> has more clear intent than <MyriadFile> (which isn't immediately obvious whether it is for input or output). This is less an issue in my scenario since the input is a .dacpac, but worse if both input and output are .fs files.
  • It would allow specifying a named generator in the case where no input file is needed
  • Moving Namespace configuration into here seems pretty convenient. Although, the toml is already pretty straight forward and flexible.

JordanMarr avatar Apr 16 '21 05:04 JordanMarr

I think maybe extending the interface so that IsValid is passed some properties and then the plugin author can decide whether its applied of not

IsValid(inputFile: string option, extension: string option, generators: string list) : bool

generators would probably be exclusive if present I guess its up to the plugin to decide based on the inputs passed

The IsValid filter is also a very nice idea. It seems like maybe it could even replace the ValidInputExtensions member altogether since it has the extension passed to it. I like that inputFile is optional. I'm not sure how generators would work so I have opinion on it.

JordanMarr avatar Apr 16 '21 05:04 JordanMarr

<InputFile> has more clear intent than <MyriadFile> (which isn't immediately obvious whether it is for input or output). This is less an issue in my scenario since the input is a .dacpac, but worse if both input and output are .fs files.

Its to do with how msbuild work, output file is Compile Include, it has to be that way to get any integration

Technically it could just be <Input>{ ... }</Input> with inline data which is output, I think the file suffix is a bit wordy. Having Myriad as the parent makes it obvious that the siblings are just for it.

7sharp9 avatar Apr 16 '21 08:04 7sharp9

I think making the input optional and adding explicit Generators after this: https://github.com/MoiraeSoftware/myriad/blob/master/src/Myriad.Sdk/build/Myriad.Sdk.targets#L8

Is probably the way to proceed.

7sharp9 avatar Apr 16 '21 08:04 7sharp9

At some point I'll draft a spec, or someone else could if they feel inclined, and start a PR. Im not sure when I'll have time just yet.

7sharp9 avatar Apr 16 '21 20:04 7sharp9

@JordanMarr Im not entirely sure what the conclusion of all this was?

7sharp9 avatar May 11 '22 15:05 7sharp9

Maybe its: There needs to be a way of specifying which generators do or don't run on arbitrary files, so some type of filter, and as it cant be in the file itself it needs to be in the fsproj, hence the need for a generator and maybe an exludeGenerator element?

    <Compile Include="ArbitaryFile.fs">
      <MyriadFile>Test.txt</MyriadFile>
      <MyriadConfigKey>example1</MyriadConfigKey>
      <MyriadParams>
        <MyriadParam1>5</MyriadParam1>
        <MyriadParam2>6</MyriadParam2>
      </MyriadParams>
    </Compile>

Thats what an arbitrary input file currently supports

7sharp9 avatar May 11 '22 16:05 7sharp9

I ended up shelving the .dacpac approach entirely because:

  • .dacpac is limited to only SQL Server and I opted to support multiple DB providers
  • I already implemented .dacpac support for SQLProvider anyway

So you can probably close the issue... that is unless you still want to add support for specifying a generator for a given Compile output file (which does seem like it would be useful). I think a filter is exactly what is needed: run all by default unless one or more <Generator>ssdt</Generator> elements are specified, in which case it would only use those.

Or if you wanted to go all out, you could even make <IncludeGenerator>ssdt</IncludeGenerator> and <ExcludeGenerator>ssdt<ExcludeGenerator> which would make it a little more explicitly additive or subtractive, but that may also be overkill.

JordanMarr avatar May 11 '22 16:05 JordanMarr

It does feel like a feature hole where everything just runs, especially if you like to cram multiple plugins in one dll.

7sharp9 avatar May 11 '22 17:05 7sharp9

What makes it hard is this needs to be done in msbuild otherwise every plugin always gets activated and has to return an empty ast so nothing is added but thats very wasteful, the plugin should just not get invoked by myriad if it is excluded.

7sharp9 avatar May 11 '22 17:05 7sharp9

Thinking more I don't think its even possible at the MSBuild level as Myriad uses reflection to find all plugins inside the passed dll, so it has to be done at that point which is annoying.

7sharp9 avatar May 11 '22 17:05 7sharp9

This is done, just need docs 😢

7sharp9 avatar May 13 '22 17:05 7sharp9

Or you could just throw a few lines into the readme and be done with it.

JordanMarr avatar May 13 '22 20:05 JordanMarr

CAnt remember anything about this, so closing...

7sharp9 avatar Jun 28 '23 11:06 7sharp9