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

Amazon.Lambda.Tools 5.12.1 breaks application publishing #351

Open
1 task done
martincostello opened this issue Nov 27, 2024 · 7 comments
Open
1 task done

Amazon.Lambda.Tools 5.12.1 breaks application publishing #351

martincostello opened this issue Nov 27, 2024 · 7 comments
Labels
bug This issue is a bug. module/cli-ext p2 This is a standard priority issue potential-regression Marking this issue as a potential regression to be checked by team member s Effort estimation: small

Comments

@martincostello
Copy link

Describe the bug

I have a toy application targeting .NET 9 that is deployed to both AWS Lambda as a custom runtime function and Azure App Service as a container.

A dependabot PR (martincostello/adventofcode#1980) updating Amazon.Lambda.Tools from 5.12.0 to 5.12.1 breaks publishing the application on Linux and Windows with one error and on macOS with a different error.

Linux and Windows

Amazon Lambda Tools for .NET Core applications (5.12.1)
Project Home: https://github.com/aws/aws-extensions-for-dotnet-cli, https://github.com/aws/aws-lambda-dotnet
	
Host machine architecture (X64) differs from Lambda architecture (Arm64). Building Native AOT Lambda functions require the host and lambda architectures to match.

macOS

Amazon Lambda Tools for .NET Core applications (5.12.1)
Project Home: https://github.com/aws/aws-extensions-for-dotnet-cli, https://github.com/aws/aws-lambda-dotnet
	
No container build image available for targetFramework net9.0 and architecture arm64.

The tooling should not block the publish as the application is authored in a way that a working native AoT application is produced that works for an ARM64 custom runtime function.

Regression Issue

  • Select this option if this issue appears to be a regression.

Expected Behavior

The lambda ZIP archive is produced for deployment.

Current Behavior

Publishing the application fails.

Reproduction Steps

  1. Clone martincostello/adventofcode@d3e9a00
  2. Run build.ps1 -SkipTests from the root of the repository

Possible Solution

No response

Additional Information/Context

No response

Targeted .NET platform

.NET 9

CLI extension version

amazon.lambda.tools 5.12.1

Environment details (OS name and version, etc.)

Any

@martincostello martincostello added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 27, 2024
@github-actions github-actions bot added the potential-regression Marking this issue as a potential regression to be checked by team member label Nov 27, 2024
@normj
Copy link
Member

normj commented Nov 27, 2024

Version 5.12.1 corrected how the tool evaluated MSBuild properties like PublishAot to use the dotnet msbuild -getproperty command instead of the brute force approach of parsing the csproj file for the XML element PublishAot. This was a long time asked feature to be able to handle properties being set in places like Directory.Build.props.

In your case you have a Directory.Build.targets file that was ignored before 5.12.1 but is not being used in the evaluation of the PublishAot property. In that file you are setting PublishAot to true which is causing the tool to think it should do a Native AOT compilation.

  <PropertyGroup Condition=" ('$(MSBuildProjectName)' == 'AdventOfCode.Console' OR '$(MSBuildProjectName)' == 'AdventOfCode.Site') AND '$(PublishForAWSLambda)' != 'true' ">
    <PublishAot>true</PublishAot>
  </PropertyGroup>

This is a change in behavior that I understand the argument is breaking behavior. I would prefer to keep using dotnet msbuild -getproperty to evaluate msbuild properties since it is the correct value and people have been asking for this for a long time. Do you think you can change your project around to not attempt PublishAot? If you don't want to change your Directory.Build.targets file you could add --msbuild-parameters "/p:PublishAot=false" in your build.ps1 where you invoke the dotnet lambda package command.

@ashishdhingra ashishdhingra added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. p2 This is a standard priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Nov 27, 2024
@martincostello
Copy link
Author

martincostello commented Nov 28, 2024

I'll try explicitly turning it off, but it should be off by default for publishing the Lambda anyway. The property shouldn't be evaluating as true as the condition guarding it should be false.

martincostello added a commit to martincostello/adventofcode that referenced this issue Nov 28, 2024
@martincostello
Copy link
Author

martincostello commented Nov 28, 2024

Nope, that didn't work either: martincostello/adventofcode@c8dbd01

The tool is incorrectly determining AoT usage and blocking the publishing.

I already worked around a previous incorrect determination with some of the changes you can see in the diff here where I publish for AoT for Azure App Service, but not for Lambda: martincostello/adventofcode@930d5a5

@martincostello
Copy link
Author

martincostello commented Nov 28, 2024

I think what's missing is that you're not passing any of the msbuild-parameters values provided through to dotnet msbuild:

var arguments = new List<string>
{
"msbuild",
projectFile,
"-nologo",
$"--getProperty:{string.Join(',', propertyNames)}"
};

This is what I get running it manually:

dotnet msbuild -getProperty:PublishAot .\src\AdventOfCode.Site\ -p:PublishForAWSLambda=false
truedotnet msbuild -getProperty:PublishAot .\src\AdventOfCode.Site\ -p:PublishForAWSLambda=truedotnet msbuild -getProperty:PublishAot .\src\AdventOfCode.Site\
true

Not adding in the properties causes the MSBuild evaluation to get the wrong result, causing a false-positive error.

@normj
Copy link
Member

normj commented Nov 29, 2024

@martincostello That is a good catch that we should include the current msbuild parameters specified to dotnet lambda. We'll get that fixed. How much of a blocker is this for you? With the US holidays and re:Invent going on next week we are under strict release conditions for non re:Invent related releases. So by default I wouldn't be able to get a fix till after re:Invent.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Nov 29, 2024
@martincostello
Copy link
Author

That's fine - I can easily just ignore the dependabot PR and not take the change. No rush, so post-re:Invent to test any fix is absolutely fine.

@normj
Copy link
Member

normj commented Nov 30, 2024

Here is the PR to include the --msbuild-parameters value when evaluating MSBuild properties. When we get past our restricted change control for re:Invent I'll get this released.

@ashishdhingra ashishdhingra added the s Effort estimation: small label Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. module/cli-ext p2 This is a standard priority issue potential-regression Marking this issue as a potential regression to be checked by team member s Effort estimation: small
Projects
None yet
Development

No branches or pull requests

3 participants