-
Notifications
You must be signed in to change notification settings - Fork 475
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
#1571 Add annotations support for executable assemblies #1573
#1571 Add annotations support for executable assemblies #1573
Conversation
One other area I'd like some feedback on is the way the serializer is set within the executable assembly template. Currently it's hardcoded, in the same way it is in the LambdaFunctionModelBuilder class. It looks like there is an active TODO which would solve both points. I've added a custom serlalizer property to the LambdaFunction attribute. Could do a similar thing with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in the Writers
namespace needs to be updated to sync the CloudFormation template with the handler being the assembly name and setting the ANNOTATIONS_HANDLER
environment variable when using executable.
/// <summary> | ||
/// Indicates this the Lambda function is going to target an executable instead of a class based handler. | ||
/// </summary> | ||
[AttributeUsage(AttributeTargets.Class)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I envision this being an assembly attribute. No reason it would be tied to a startup class and there might not even be a startup class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add it as an assembly attribute, but I didn't think it was possible to access an assembly attribute in a source generator because the assembly doesn't exist yet.
EDIT: Ignore, found it.
/// Indicates this the Lambda function is going to target an executable instead of a class based handler. | ||
/// </summary> | ||
[AttributeUsage(AttributeTargets.Class)] | ||
public class LambdaOutputExecutableAttribute : Attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think LambdaGenerateMain
would be clearer? I'm on the fence so welcome your input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, agreed
if (this._lambdaFunctions.Count == 1) | ||
{ | ||
#> | ||
Func<<#= string.Join(", ", this._lambdaFunctions[0].GeneratedMethod.Parameters.Select(p => $"{p.Type.FullName}")) #>, <#= this._lambdaFunctions[0].GeneratedMethod.ReturnType.FullName #>> <#=this._lambdaFunctions[0].LambdaMethod.Name.ToLower()#> = <#=this._lambdaFunctions[0].LambdaMethod.ContainingType.Name#>_<#=this._lambdaFunctions[0].LambdaMethod.Name#>_Generated.<#=this._lambdaFunctions[0].LambdaMethod.Name#>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The LambdaFunction
might not return anything, in which case this would be an Action<>
instead of a Func<>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's handled above with the check if the return type is void.
annotationReport.LambdaFunctions.Add(model); | ||
} | ||
|
||
if (receiver.IsExecutable) | ||
{ | ||
var executableAssembly = new ExecutableAssembly(lambdaModels, lambdaModels[0].LambdaMethod.ContainingNamespace); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a diagnostic error if the user already has a Program
class. Otherwise they will get confusing error messages for duplicate Program
classes after the generator generates one. I'm wondering if we should actually use a different name then Program
to something more unique. And then we should check to see if there is already a Main
method and if so give an error for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need to add a diagnostic error if the project is missing a reference to Amazon.Lambda.RuntimeSupport
. Otherwise users will get confused when they add the new executable attribute and they start getting weird compiler errors. You can find some examples in this library where we check to make sure their is dependency to the API Gateway event package if they are creating an API Gateway based function.
{ | ||
#> | ||
|
||
switch (Environment.GetEnvironmentVariable("HANDLER")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of calling the environment variable ANNOTATIONS_HANDLER
to give the variable name more scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, agreed.
@@ -19,14 +20,14 @@ using <#= ns #>; | |||
|
|||
namespace <#= _model.LambdaMethod.ContainingNamespace #> | |||
{ | |||
public class <#= _model.GeneratedMethod.ContainingType.Name #> | |||
public <#= _model.IsExecutable ? "static " : "" #>class <#= _model.GeneratedMethod.ContainingType.Name #> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does executable mean the wrapper classes should be static
? It is worth a discussion if we should always use static classes or not but we should be consistent. I would rather we always use instance classes. It makes the testing more predictable and repeatable if we use instance classes. For example the mock lambda test tool always runs the constructor for every invoke so users can easily debug the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a good point. Although if you were using mock lambda test tool for this implementation you'd be using the executable assembly feature anyway right?
/// <summary> | ||
/// The serializer to use. | ||
/// </summary> | ||
public string Serializer { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a mix of approaches here. For class libraries user's don't set this value but instead use the assembly attribute. But for executable they would need to set this property. This will cause class library users to think they can set this property and that will work. I think we need to resolve what is the serializer via the attributes so we have a consistent experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood and updated to use the assembly OR the method attribute. I think this also fixes another issue that was opened yesterday #1572
private static async Task Main(string[] args) | ||
{ | ||
<# | ||
if (this._lambdaFunctions.Count == 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to special case the situation where there is just one function? The CloudFormation template should be updated to set the ANNOTATIONS_HANDLER
environment variable no matter if there is 1 or a 100 functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this to remove the need for the environment variable when you've only got one function, but that does then have the knock on effect of complicating the cloud formation writer code. Updated to always use environment variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we are getting close. Minor issues, some generated compilation errors address and some left over static logic that needs to be removed. Direction wise I don't have any major comments.
@@ -45,6 +45,13 @@ public static class DiagnosticDescriptors | |||
DiagnosticSeverity.Error, | |||
isEnabledByDefault: true); | |||
|
|||
public static readonly DiagnosticDescriptor MainMethodExists = new DiagnosticDescriptor(id: "AWSLambda0104", | |||
title: "static Main method exists", | |||
messageFormat: "Your project already defines a static Main method.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about for error message
Failed to generate Main method for LambdaGenerateMainAttribute because project already contains Main method. Existing Main methods must be removed when using LambdaGenerateMainAttribute attribute.
var model = context.Compilation.GetSemanticModel(methodDeclaration.SyntaxTree); | ||
var symbol = model.GetDeclaredSymbol(methodDeclaration); | ||
|
||
if (symbol.Name == "Main" && symbol.IsStatic && symbol.ContainingNamespace.Name == lambdaModels[0].LambdaMethod.ContainingAssembly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to check if the method has either 0 parameters or 1 parameter of type string[]
to be sure it is an entry point Main
.
@@ -61,12 +67,26 @@ public void OnVisitSyntaxNode(GeneratorSyntaxContext context) | |||
if (context.Node is ClassDeclarationSyntax classDeclarationSyntax && classDeclarationSyntax.AttributeLists.Count > 0) | |||
{ | |||
// Get the symbol being declared by the class, and keep it if its annotated | |||
var methodSymbol = context.SemanticModel.GetDeclaredSymbol(classDeclarationSyntax); | |||
var methodSymbol = ModelExtensions.GetDeclaredSymbol( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ModelExtensions.GetDeclaredSymbol
is a C# extension method for SemanticModel
. Is the reason you switch this code to call the extension method directly through the extension method type holder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, no good reason. Updated.
|
||
if (context.Node is MethodDeclarationSyntax methodDeclaration) | ||
{ | ||
var model = ModelExtensions.GetDeclaredSymbol( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment as above. Just call the method from context.SemanticModel.GetDeclaredSymbol
.
if (model.GeneratedMethod.ReturnType.FullName == "void") | ||
{ | ||
#> | ||
Action<<#= string.Join(", ", model.GeneratedMethod.Parameters.Select(p => $"{p.Type.FullName}")) #>> <#= model.LambdaMethod.Name.ToLower() #> = <#= model.LambdaMethod.ContainingType.Name #>_<#= model.LambdaMethod.Name #>_Generated.<#= model.LambdaMethod.Name #>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are missing creating the instance of the generated type. For example in the test I see
Func<string, string> toupper = Functions_ToUpper_Generated.ToUpper;
and expected to see
Func<string, string> toupper = new Functions_ToUpper_Generated().ToUpper;
<# | ||
} | ||
else | ||
{ | ||
#> | ||
private readonly <#= _model.LambdaMethod.ContainingType.Name #> <#= _model.LambdaMethod.ContainingType.Name.ToCamelCase() #>; | ||
private <#= _model.IsExecutable ? "static " : "" #>readonly <#= _model.LambdaMethod.ContainingType.Name #> <#= _model.LambdaMethod.ContainingType.Name.ToCamelCase() #>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ternary operator for static should be removed since we aligned on the wrapping generated class always being an instance class.
<# | ||
} | ||
else | ||
{ | ||
#> | ||
private readonly <#= _model.LambdaMethod.ContainingType.Name #> <#= _model.LambdaMethod.ContainingType.Name.ToCamelCase() #>; | ||
private <#= _model.IsExecutable ? "static " : "" #>readonly <#= _model.LambdaMethod.ContainingType.Name #> <#= _model.LambdaMethod.ContainingType.Name.ToCamelCase() #>; | ||
private <#= _model.IsExecutable ? "static " : "" #>readonly <#= _model.Serializer #> serializer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ternary operator for static should be removed since we aligned on the wrapping generated class always being an instance class.
<# | ||
} | ||
#> | ||
|
||
<# | ||
if (_model.IsExecutable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the static condition and static constructor. We are always using the instance constructor.
@@ -26,7 +27,7 @@ this.Write(new FieldsAndConstructor(_model).TransformText()); | |||
#> | |||
|
|||
|
|||
public <#= _model.LambdaMethod.ReturnsVoidOrGenericTask ? "async " : "" #><#= _model.GeneratedMethod.ReturnType.FullName #> <#= _model.LambdaMethod.Name #>(<#= string.Join(", ", _model.GeneratedMethod.Parameters.Select(p => $"{p.Type.FullName} {p.Name}")) #>) | |||
public <#= _model.IsExecutable ? "static " : "" #><#= _model.LambdaMethod.ReturnsVoidOrGenericTask ? "async " : "" #><#= _model.GeneratedMethod.ReturnType.FullName #> <#= _model.LambdaMethod.Name #>(<#= string.Join(", ", _model.GeneratedMethod.Parameters.Select(p => $"{p.Type.FullName} {p.Name}")) #>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the static logic.
namespace Amazon.Lambda.Annotations | ||
{ | ||
/// <summary> | ||
/// Indicates this the Lambda function is going to target an executable instead of a class based handler. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc suggestion:
Used when deploying Lambda functions as a .NET executable instead of a class library. Ensure the .NET project's output type is configured for `Console Application`. In the project file the `OutputType` property will be set to `Exe`.
Deploying as an executable versus a class library is required when compiling functions with Native AOT. It can also be useful to deploy as an executable to include specific versions of `Amazon.Lambda.RuntimeSupport` the .NET Lambda runtime client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The integ tests are failing for me till I update this assert from 25 to 26 for the new test function you added. Can you update the value as part of your PR?
I also build the package locally and did some adhoc testing which mostly went well but when I tried AOT I ran into problems. The problem is the library is setting the runtime
in the CloudFormation template to dotnet6
but for AOT we need it set for provided.al2
. There is logic in the .NET deploy tooling preventing deploying an AOT executable to .NET 6.
What I'm wondering if we should repurpose the LambdaGenerateMain
attribute into a general global properties attribute. Kind of mapping to the CloudFormation template global section. Something like:
[assembly:LambdaGlobalProperties(GenerateMain = true, Runtime = "provided.al2");
What do you think?
@@ -45,6 +45,13 @@ public static class DiagnosticDescriptors | |||
DiagnosticSeverity.Error, | |||
isEnabledByDefault: true); | |||
|
|||
public static readonly DiagnosticDescriptor MainMethodExists = new DiagnosticDescriptor(id: "AWSLambda0104", | |||
title: "static Main method exists", | |||
messageFormat: "Failed to generate Main method for LambdaGenerateMainAttribute because project already contains Main method. Existing Main methods must be removed when using LambdaGenerateMainAttribute attribute.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: There are 2 spaces in between LambdaGenerateMainAttribute
and attribute
at the end.
if (model.GeneratedMethod.ReturnType.FullName == "void") | ||
{ | ||
#> | ||
Action<<#= string.Join(", ", model.GeneratedMethod.Parameters.Select(p => $"{p.Type.FullName}")) #>> <#= model.LambdaMethod.Name.ToLower() #> = new <#= model.LambdaMethod.ContainingNamespace #>.<#= model.LambdaMethod.ContainingType.Name #>_<#= model.LambdaMethod.Name #>_Generated().<#= model.LambdaMethod.Name #>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird corner case when testing this in action but since you are using <#= model.LambdaMethod.Name.ToLower() #>
as the name of the variable and the Blueprint in Visual studio contains a method called Default
it cases the variable name to be default
which is a reserved .NET word trigger compilation errors. Maybe change this to <#= model.LambdaMethod.Name.ToLower() #>_handler
to avoid reserved word collision.
else | ||
{ | ||
#> | ||
Func<<#= string.Join(", ", model.GeneratedMethod.Parameters.Select(p => $"{p.Type.FullName}")) #>, <#= model.GeneratedMethod.ReturnType.FullName #>> <#= model.LambdaMethod.Name.ToLower() #> = new <#= model.LambdaMethod.ContainingNamespace #>.<#= model.LambdaMethod.ContainingType.Name #>_<#= model.LambdaMethod.Name #>_Generated().<#= model.LambdaMethod.Name #>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above about adding _handler
suffix to the variable name.
|
||
if (generateMainAttribute != null) | ||
{ | ||
isExecutable = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the context.Compilation.CommonOptions.OutputKind
property to see if it is OutputKind.ConsoleApplication. Then write a diagnostic error. I forget to change this when I was converting the blueprint in Visual Studio to use this feature and had to figure out after deployment why I was getting an Internal Error
which is not a great experience.
@normj I like your idea of having Lambda Global properties. It also keeps it flexible for any future changes we might want to make. Updated and addressed all the comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been playing with this PR and for the most part things have worked well. The one issue I have come across is if I change the runtime
property to provided.al2
all of the new functions get provided.al2
as the runtime. But all of the existing functions didn't get updated. That makes it hard/confusing switching from .NET 6 managed to a .NET 7 Executable for a project.
@normj all feedback actioned, apologies for the delay. |
|
||
return new ExecutableAssembly( | ||
lambdaModels, | ||
lambdaModels[0].LambdaMethod.ContainingNamespace); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this is the code that is causing the bug that if there are no Lambda functions we give back the "AWSLambda001 This is a bug. Please run the build ..." compilation error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a specific Diagnostic error to handle this.
if (model.GeneratedMethod.ReturnType.FullName == "void") | ||
{ | ||
#> | ||
Action<<#= string.Join(", ", model.GeneratedMethod.Parameters.Select(p => $"{p.Type.FullName}")) #>> <#= model.LambdaMethod.ExecutableAssemblyHandlerName #> = new <#= model.LambdaMethod.ContainingNamespace #>.<#= model.LambdaMethod.ContainingType.Name #>_<#= model.LambdaMethod.Name #>_Generated().<#= model.LambdaMethod.Name #>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are no parameters like the following function
[LambdaFunction]
public void Post()
{
}
then the Action is setup with an empty generic parameter instead of no generic parameter
case "Post":
Action<> post_handler = new JamesAnnotationTest.Functions_Post_Generated().Post;
await Amazon.Lambda.RuntimeSupport.LambdaBootstrapBuilder.Create(post_handler, new Amazon.Lambda.Serialization.SystemTextJson.DefaultLambdaJsonSerializer()).Build().RunAsync();
break;
else | ||
{ | ||
#> | ||
Func<<#= string.Join(", ", model.GeneratedMethod.Parameters.Select(p => $"{p.Type.FullName}")) #>, <#= model.GeneratedMethod.ReturnType.FullName #>> <#= model.LambdaMethod.ExecutableAssemblyHandlerName #> = new <#= model.LambdaMethod.ContainingNamespace #>.<#= model.LambdaMethod.ContainingType.Name #>_<#= model.LambdaMethod.Name #>_Generated().<#= model.LambdaMethod.Name #>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same issue as the Action above. This function
[LambdaFunction]
public string Default()
{
}
Creates a func with invalid comma at the start
Func<, string> default_handler = new JamesAnnotationTest.Functions_Default_Generated().Default;
await Amazon.Lambda.RuntimeSupport.LambdaBootstrapBuilder.Create(default_handler, new Amazon.Lambda.Serialization.SystemTextJson.DefaultLambdaJsonSerializer()).Build().RunAsync();
break;
@@ -0,0 +1,40 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "bak" file should be removed from the PR.
Fix typo 'consided' --> 'considered'.
Fix typo 'inteface' --> 'interface'
Still working my way through the code review, but could you add a header and section to https://github.com/aws/aws-lambda-dotnet/blob/master/Libraries/src/Amazon.Lambda.Annotations/README.md covering the new assembly attributes? We have more content in there than the developer guides for annotations. Also okay if in a separate PR also targeting |
Libraries/src/Amazon.Lambda.Annotations.SourceGenerator/Diagnostics/DiagnosticDescriptors.cs
Outdated
Show resolved
Hide resolved
Libraries/test/Amazon.Lambda.Annotations.SourceGenerators.Tests/SourceGeneratorTests.cs
Outdated
Show resolved
Hide resolved
Libraries/src/Amazon.Lambda.Annotations.SourceGenerator/Diagnostics/DiagnosticDescriptors.cs
Outdated
Show resolved
Hide resolved
@ashovlin all comments addressed |
Merged into |
8fea610
into
aws:feature/annotations-executable
Issue #, if available:
#1571
Description of changes:
Add functionality for executable assemblies for annotations framework. This will auto generate the
static Main
method.I'm particularly interested in feedback on the use of a new
LambdaOutputExecutable
annotation, I couldn't find a better way to establish if it was targetting an output assembly.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.