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 support for .NET 8 to Lambda Annotations #1658

Merged
merged 8 commits into from
Feb 14, 2024
Merged

Conversation

ashovlin
Copy link
Member

@ashovlin ashovlin commented Jan 18, 2024

Issue #, if available: DOTNET-7321

Description of changes: This adds support for the upcoming .NET 8 managed runtime to Lambda Annotations.

  • Previously: Since dotnet6 was the only managed runtime when annotations launched we hardcoded dotnet6 when generating the CloudFormation for a zip package type
  • Now: We support dotnet8 in either the LambdaGlobalProperties attribute or by detecting it in the user's csproj.

On Detection

  1. From internal discussions, we didn't want to simply parse the .csproj's XML for <TargetFramework> since that leaves us susceptible to issues like Directory.build.props heirarchy is not respected aws-extensions-for-dotnet-cli#211
  2. I could not find a way to detect the TFM from the context available to the source generator (GeneratorExecutionContext)
  3. I explored using Micorosft.Build and Microsoft.Build.Locator, but those don't target .NET Standard so I don't think we can use them from a source generator.
  4. Norm suggested the new dotnet msbuild -getProperty which is available with 17.8 (8.0.1) Edit: see Add support for .NET 8 to Lambda Annotations #1658

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ashovlin ashovlin requested review from normj and philasmar January 18, 2024 15:18
"test\\TestExecutableServerlessApp\\TestExecutableServerlessApp.csproj",
"test\\TestServerlessApp.IntegrationTests\\TestServerlessApp.IntegrationTests.csproj",
"test\\TestServerlessApp.NET8\\TestServerlessApp.NET8.csproj",
Copy link
Member Author

@ashovlin ashovlin Jan 18, 2024

Choose a reason for hiding this comment

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

fyi: I was only adding line 14, my VS sorted the list.

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@@ -1,6 +1,16 @@
; Shipped analyzer releases
; https://github.com/dotnet/roslyn-analyzers/blob/master/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md

## Release 1.1.0
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated to this change, just moving the errors we added in 1.1.0 to the shipped file.

envValue.Append($"{Environment.GetEnvironmentVariable(envName)}_");
}

envValue.Append("amazon-lambda-annotations_1.1.0.0");
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just increment the version to the one we intend to release this PR in? (and do it for all the other snaphsot files)

Copy link
Member Author

Choose a reason for hiding this comment

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

Given the churn we've had with .NET 8, I didn't want to tie this to a version yet so would just do the separate versioning PR.

@ashovlin ashovlin requested a review from philasmar February 8, 2024 18:19
Copy link
Member

@normj normj left a comment

Choose a reason for hiding this comment

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

@ashovlin
Copy link
Member Author

Summarizing today's commits from internal discussion: I had seen prior art for the following:

context.AnalyzerConfigOptions.GlobalOptions.TryGetValue("build_property.TargetFramework", out var targetFramework);

But when I initially tried it out, targetFramework was null so I gave up. However I was testing this through our CSharpSourceGeneratorTest which wasn't setting the TFM appropriately. When manually testing the full package in a test project targetFramework was set appropriately.

e6ec280 now pivots to this approach and removes the dotnet msbuild -getProperty which should be more performant.

@ashovlin
Copy link
Member Author

Can you update the following comment block to include dotnet8.

https://github.com/aws/aws-lambda-dotnet/blob/master/Libraries/src/Amazon.Lambda.Annotations/LambdaGlobalPropertiesAttribute.cs#L22

Fixed in a1aac5a, thanks.

@ashovlin ashovlin requested a review from normj February 14, 2024 04:16
@ashovlin ashovlin merged commit 5d26ecd into dev Feb 14, 2024
4 checks passed
@ashovlin ashovlin deleted the shovlia/annotations-net8 branch February 14, 2024 18:32
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