Disable AmmySidekick

[distantcam]

I was thinking about a scenario where I wouldn't want to include the AmmySidekick.dll. For development it's great, but for shipping code I wouldn't want to include it. And I also wouldn't want someone being able to add it to a directory and suddenly have an open port that can be used to change the UI of the app. Security nightmare.

So, first I tried removing AmmySidekick.dll from the output folder. This causes the app to crash trying to look for the assembly.

So then I removed the reference from the project. This caused the Ammy compiler to crash.

1>------ Build started: Project: AmmyQuickstart, Configuration: Debug Any CPU ------
1><Default>(1,1,1,1): error : Ammy compilation error: System.NullReferenceException: Object reference not set to an instance of an object.
1><Default>(1,1,1,1): error :    at Ammy.Language.AstTopExtensions.BuildXaml(TopWithNode top, TypeSymbol nodeType, ImmutableArray`1 members, String rootSymbolId, DependentPropertyEvalContext context)
1><Default>(1,1,1,1): error :    at Ammy.Language.TopWithNode.EvalProperties(DependentPropertyEvalContext context)
1><Default>(1,1,1,1): error :    at Ammy.Language.Start.EvalProperties(DependentPropertyEvalContext context)
1><Default>(1,1,1,1): error :    at Nitra.Declarations.EvalPropertiesHost.EvalProperties(DependentPropertyEvalContext context, IDependentPropertyContainer obj, Single statistics)
1><Default>(1,1,1,1): error :    at Nitra.Declarations.ProjectEvalPropertiesHost.ExecutePass(DependentPropertyEvalContext context, String passName)
1><Default>(1,1,1,1): error :    at Nitra.Declarations.EvalPropertiesHost.EvalProperties(DependentPropertyEvalContext context, String passName, Int32 stage)
1><Default>(1,1,1,1): error :    at Ammy.Language.Start.RefreshProject(CancellationToken _cancellationToken, ImmutableArray`1 files, Object data)
1><Default>(1,1,1,1): error :    at Ammy.Build.AmmyProject.RefreshProject(String outputPath, String rootNamespace, String assemblyName)
1><Default>(1,1,1,1): error :    at Ammy.Build.AmmyCompiler.Compile(AmmyProject project, Boolean generateMetaFile, Boolean needBamlGeneration, Boolean needUpdate)
1>I:\src\other\AmmyQuickstart\packages\Ammy.1.1.35\build\Ammy.targets(16,5): error : Ammy compilation failed
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========

It would be nice to have an Ammy compile only option that doesn't include the runtime UI updating stuff, and doesn't need the AmmySidekick.dll.

[aienabled]

+1, we don't need this is in the release builds for WPF applications. So it would be great if Ammy doesn't include AmmySidekick if there are no DEBUG symbol defined. It should be not hard as AmmyBackend already includes bool parameter determining whether it should include AmmySidekick code into the produced XAML code. And AmmySidekick.dll perhaps should not be referenced by the project at all. But if it should be referenced for some reason, it could be excluded from the release build by configuration checking in the csproj file, like this:

<ItemGroup Condition="'$(Configuration)' != 'Release'">
    <Reference Include="AmmySidekick" />
</ItemGroup>

I don't know if it possible to add a condition by symbol, but configuration condition like in this example should be good enough for most cases.

[ionoy]

Conditions for adding Register are following:

1. DEBUG compilation symbol defined
2. NO_AMMY_UPDATE compilation symbol is not defined

Although, they both don't work in the most recent version of Ammy since I've added SupportsRuntimeUpdate option to platform configuration. This bug is already fixed and everything should work properly in the next version.

As for condition for including AmmySidekick.dll, I'm not sure it is possible to add it with NuGet. If it's not possible, then the easiest way would be to just add documentation article on how to exclude AmmySidekick.dll conditionally.

[distantcam]

@ionoy Awesome!

I ❤️ this project.

[ionoy]

Thanks, man!

Just remembered. AmmySidekick.dll contains both runtime update logic and ExpressionConverter that is used for binding expressions. So, to exclude this dependency would mean binding expression not working anymore. Is referencing AmmySidekick.dll in non-debug scenarios a problem?

[aienabled]

@ionoy, I think it would be better to have separate assemblies for ExpressionConverter (AmmyHelper.dll?) and AmmySidekick.

[distantcam]

Yeah that's a problem, given AmmySidekick.dll also has that open port that requires users to open their firewall. This blocks me from using Ammy in commercial projects.

Apart from @aienabled's suggestion of splitting the 2 libraries, could Ammy provide the code for the bits it needs to the compiler so it's compiled directly into the assembly, rather than being a reference? Given Ammy is performing compiler magic anyway.

[ionoy]

Ammy wouldn't require an open port in non-debug or NO_AMMY_UPDATE configuration. Port is opened as soon as the first element is registered with Ammy.Register.

could Ammy provide the code for the bits it needs to the compiler so it's compiled directly into the assembly

What do you mean by that?

[aienabled]

@distantcam, have you seen the generated XAML code when the inline binding converters are used? Ammy serializes C# code to the binding expression and uses ExpressionConverter as the binding converter, which is included into AmmySidekick.dll currently. Ammy doesn't generate any C# code, it provides only MSBuild targets for converting Ammy to XAML files. So no magic, but really genius use of the already available features of XAML!

So I propose to move ExpressionConverter into the separate assembly.

[distantcam]

@aienabled No I haven't played with binding in Ammy yet. So I'm not sure how it works under the hood.

@ionoy I've just noticed the Ammy task already outputs compiled files as AmmyGeneratedItems. That's what I meant. You could also add the ExpressionConverter that way.

[ionoy]

Yeah, separating binding logic into its own assembly seems like a good idea. Runtime update code is very platform specific, so bundling it with code that is not platform specific seems wrong. There is also an SRP argument there.

@distantcam This is actually an interesting idea. Including code behind the scene instead of referencing platform dependent assembly. What would be the disadvantages?

[aienabled]

Even if ExpressionConverter code is compact and doesn't have any dependencies it will be generated per project. So each project will have it's own ExpressionConverter code. Might be not a really good idea.

[ionoy]

There is more code than just ExpressionConverter, so it isn't exactly compact. I can see a problem if there are multiple XAML-based projects inside the same solution that reference each other. Nothing major would happen (probably), but if there would be different versions of Ammy for each project it could lead to some interesting bugs.

[distantcam]

What would be the disadvantages?

Code collisions because someone has an Ammy class in their project for some reason. This can happen sometimes. It's best to use a name like ___COMPILED___AMMY, which hopefully would never be used. The harder solution would be to pre-compile the source using Roslyn, detect if there is a collision, and change the code output, but that's a lot of work and the simpler solution should work.

Also the project not having the right references for the code to compile. Again, just add to the references during the MSBuild task.

if there would be different versions of Ammy for each project it could lead to interesting bugs.

Another reason to compile it into the assembly. Make the class internal and it's not a problem. :)

The ExpressionConverter source would be revealed this way, if that's a concern.

[ionoy]

Revealing ExpressionConverter code is not an issue. It's kinda open at the moment since you can decompile AmmySidekick.

I like the idea of ExpressionConverter being included as an internal code. This means we don't need a separate assembly, which means no problems with conflicting dlls. It makes Ammy compilation a little bit more complicated since there is no actual reference with ExpressionConverter inside. I would have to create this TypeSymbol manually before resolving namespaces.

So, my current understanding of a solution is:

1. Include ExpressionConverter as code inside ammy.targets
2. Include conditional reference to AmmyRuntimeUpdate.dll inside ammy.targets