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

Closed
1 task done
martincostello opened this issue Nov 27, 2024 · 15 comments · Fixed by #355
Closed
1 task done

Amazon.Lambda.Tools 5.12.1 breaks application publishing #351

martincostello opened this issue Nov 27, 2024 · 15 comments · Fixed by #355
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
Contributor

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
Contributor 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
Contributor 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
Contributor 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
Contributor 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
@normj
Copy link
Member

normj commented Dec 16, 2024

Version 5.12.2 has been released with the change. Thanks for helping track the issue down.

@martincostello
Copy link
Contributor Author

Hmm - it still isn't working. I get the same errors as before.

I'll need to look into to see why the change doesn't seem to have made any difference.

martincostello added a commit to martincostello/aws-extensions-for-dotnet-cli that referenced this issue Dec 19, 2024
Fix MSBuild parameters specified in `aws-lambda-tools-defaults.json` not being used with `Utilities.LookPublishAotFlag()`.
Resolves aws#351.
@martincostello
Copy link
Contributor Author

I'll need to look into to see why the change doesn't seem to have made any difference.

Found the problem - the value passed through to check for native AoT doesn't include any MSBuild properties from aws-lambda-tools-defaults.json. #355 should resolve it.

@normj
Copy link
Member

normj commented Dec 19, 2024

Thanks @martincostello. We'll get the PR released.

@normj normj closed this as completed in 2c711df Dec 19, 2024
@normj normj reopened this Dec 19, 2024
@normj
Copy link
Member

normj commented Dec 19, 2024

Version 5.12.3 has been released. Can you let me know if that unblocks you?

@aws aws deleted a comment from github-actions bot Dec 19, 2024
@martincostello
Copy link
Contributor Author

All working again now 🚀: martincostello/adventofcode#2029

@normj
Copy link
Member

normj commented Dec 19, 2024

Thanks for the confirmation and the help addressing the issue.

@normj normj closed this as completed Dec 19, 2024
Copy link

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

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

Successfully merging a pull request may close this issue.

3 participants