Use MSBuild to find target framework #348

gcbeattyAWS · 2024-11-18T18:17:46Z

Issue #, if available: #211

Description of changes:

When packaging a lambda using dotnet lambda package, if the target project does not include a node (because it is defined in a Directory.build.props at a higher location) the packaging tool fails saying it does not know what framework to use.

Missing required parameter: --framework

Expected Behavior

The tool will understand that MSBuild can walk directories searching for Directory.build.props files to define these values.

Current Behavior

The tool fails to walk the folder hierarchy to find the values necessary for the build.

Changes

Creates a generic helper function to get properties for a project using MSbuild and then XML parsing of the csproj as a fallback, https://learn.microsoft.com/en-us/visualstudio/msbuild/customize-your-build?view=vs-2022
Updates find framework function to use above function
Updates other existing functions to use the generic function

Testing

Created new lambda function which has the TargetFramework defined in Directory.Build.props instead of the .csproject file. I then added this to the existing unit test case to verify the target framework is read correctly.

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

src/Amazon.Common.DotNetCli.Tools/Assets/AmazonCommonDotNetCliTools.targets

normj · 2024-11-19T08:17:07Z

src/Amazon.Common.DotNetCli.Tools/Utilities.cs

+            string error = process.StandardError.ReadToEnd();
+            process.WaitForExit();
+
+            if (process.ExitCode != 0)


If the process failed we should fallback to the original XML parsing logic. There could be CI systems that are still deploying .NET 6 and only have .NET 6 SDK installed. The -getoutput command was added in .NET 8.

ah good point! i will update accordingly.

updated in latest commit

normj · 2024-11-19T08:22:04Z

src/Amazon.Common.DotNetCli.Tools/Utilities.cs


-            var xdoc = XDocument.Load(projectFile);
+            var arguments = new[]


We look up TargetFramework, OutputType, and PublishAot from the project file in this class. I would like to see this logic generalized so that we can have a method that takes in the project and a params of property names and returns back a dictionary of values. I don't really want 3 different code paths starting the dotnet process which will end up drifting.

The shared method should have the fallback logic I describe below that if the process fails parse the project xml like it does today so we don't risk any breaking behavior.

sure will update!

updated in latest commit

normj · 2024-11-19T08:23:37Z

testapps/TestFunctionBuildProps/TestFunctionBuildProps/Readme.md

@@ -0,0 +1,49 @@
+# AWS Lambda Empty Function Project


We don't really need the readme for checked in for the test apps.

updated in latest commit

normj · 2024-11-19T18:03:32Z

src/Amazon.Common.DotNetCli.Tools/Utilities.cs

-                var hasExited = process.WaitForExit(5000);
+                string output = process.StandardOutput.ReadToEnd();
+                string error = process.StandardError.ReadToEnd();
+                process.WaitForExit();


Should we keep the 5 second timeout or at least some timeout? I'm thinking of @philasmar recent PR that at least added int.MaxValue to avoid deadlocks in this API. #342

sounds good i will update to 5 seconds

src/Amazon.Common.DotNetCli.Tools/Utilities.cs

normj · 2024-11-20T01:18:38Z

src/Amazon.Common.DotNetCli.Tools/Utilities.cs

                {
-                    msbuildProcessFailed = true;
+                    Console.WriteLine($"MSBuild process exited with code {process.ExitCode}. Error: {error}");


No Console.WriteLine calls are used. This same code is used inside Visual Studio and there is no console there. If you want to log something you need to pass in an instance if IToolLogger to handle logging.

I'm not too sure we should be logging anything visible to the user here. We are exposing an implementation detail that would be more confusing if anything.

i just removed the logging all together

normj · 2024-11-20T01:18:42Z

src/Amazon.Common.DotNetCli.Tools/Utilities.cs

            {
-                // swallow any exceptions related to `dotnet msbuild`
-                msbuildProcessFailed = true;
+                Console.WriteLine($"Error executing MSBuild or parsing output: {ex.Message}");


No Console.WriteLine calls are used. This same code is used inside Visual Studio and there is no console there. If you want to log something you need to pass in an instance if IToolLogger to handle logging.

Same comment about logging

normj · 2024-11-20T01:18:47Z

src/Amazon.Common.DotNetCli.Tools/Utilities.cs

            }
+            catch (Exception ex)
+            {
+                Console.WriteLine($"Error parsing project file XML: {ex.Message}");


No Console.WriteLine calls are used. This same code is used inside Visual Studio and there is no console there. If you want to log something you need to pass in an instance if IToolLogger to handle logging.

Same comment about logging

normj · 2024-11-20T01:23:41Z

src/Amazon.Common.DotNetCli.Tools/Utilities.cs

-                    process.Kill();
-                    msbuildProcessFailed = true;
-                }
+                    using JsonDocument doc = JsonDocument.Parse(output);


I might be missing something but if propertyNames is a single value isn't the output of the command just the value and not the JSON document?

PS C:\dev\repos\aws-extensions-for-dotnet-cli> dotnet msbuild .\testapps\TestFunction\TestFunction.csproj --getProperty:TargetFramework,TargetFrameworks { "Properties": { "TargetFramework": "net6.0", "TargetFrameworks": "" } }

is what the commmand returns, so it is a json document

We need to check if the count of propertyNames is 1 or greater since the result varies. If you run:

dotnet msbuild --nologo --getProperty:TargetFramework

The result will be

net8.0

If you run

dotnet msbuild --nologo --getProperty:TargetFramework,TargetFrameworks

The result will be:

{ "Properties": { "TargetFramework": "net8.0", "TargetFrameworks": "" } }

You need to handle both cases

ah yes you are right. i will update that!

updated to handle case for when only one property is provided. Thanks for catching this!

philasmar · 2024-11-20T16:35:25Z

.autover/changes/8a5204e4-83ec-45ac-8c75-27512ae9cdf8.json

+      "Name": "Amazon.Lambda.Tools",
+      "Type": "Patch",
+      "ChangelogMessages": [
+        "Use MSBuild to find target framework"


Add more explanation here for the significance of this for customers. Not sure everyone would know what this implies.

philasmar · 2024-11-20T16:38:40Z

src/Amazon.Common.DotNetCli.Tools/Utilities.cs

-        /// <summary>
-        /// Retrieve the `OutputType` property of a given project
+        // <summary>
+        /// Looks up specified properties from a project file.


nit: drop "file" since MSBUILD is not looking at the project file (.csproj) alone

philasmar · 2024-11-20T16:44:38Z

src/Amazon.Common.DotNetCli.Tools/Utilities.cs

-                    process.Kill();
-                    msbuildProcessFailed = true;
-                }
+                    using JsonDocument doc = JsonDocument.Parse(output);


We need to check if the count of propertyNames is 1 or greater since the result varies. If you run:

dotnet msbuild --nologo --getProperty:TargetFramework

The result will be

net8.0

If you run

dotnet msbuild --nologo --getProperty:TargetFramework,TargetFrameworks

The result will be:

{ "Properties": { "TargetFramework": "net8.0", "TargetFrameworks": "" } }

You need to handle both cases

philasmar · 2024-11-20T16:53:01Z

src/Amazon.Common.DotNetCli.Tools/Utilities.cs

                {
-                    msbuildProcessFailed = true;
+                    Console.WriteLine($"MSBuild process exited with code {process.ExitCode}. Error: {error}");


I'm not too sure we should be logging anything visible to the user here. We are exposing an implementation detail that would be more confusing if anything.

philasmar · 2024-11-20T16:54:39Z

src/Amazon.Common.DotNetCli.Tools/Utilities.cs

            {
-                // swallow any exceptions related to `dotnet msbuild`
-                msbuildProcessFailed = true;
+                Console.WriteLine($"Error executing MSBuild or parsing output: {ex.Message}");


Same comment about logging

philasmar · 2024-11-20T16:54:46Z

src/Amazon.Common.DotNetCli.Tools/Utilities.cs

            }
+            catch (Exception ex)
+            {
+                Console.WriteLine($"Error parsing project file XML: {ex.Message}");


Same comment about logging

normj

One minor comment to address an existing issue but I don't need to review the PR again assuming you address the comment.

src/Amazon.Common.DotNetCli.Tools/Utilities.cs

gcbeattyAWS changed the base branch from master to dev November 18, 2024 18:17

normj reviewed Nov 18, 2024

View reviewed changes

src/Amazon.Common.DotNetCli.Tools/Assets/AmazonCommonDotNetCliTools.targets Outdated Show resolved Hide resolved

gcbeattyAWS force-pushed the gcbeatty/buildprops branch from c33ef14 to f3fe7a3 Compare November 18, 2024 19:34

Use MSBuild to find target framework

823e65d

gcbeattyAWS force-pushed the gcbeatty/buildprops branch from f3fe7a3 to 823e65d Compare November 18, 2024 19:46

gcbeattyAWS added 2 commits November 18, 2024 14:46

remove extra spacing

8883689

Add dev config

6f1429b

gcbeattyAWS marked this pull request as ready for review November 18, 2024 20:34

gcbeattyAWS requested a review from philasmar November 18, 2024 20:34

normj requested changes Nov 19, 2024

View reviewed changes

Create generic function

cb68604

normj requested changes Nov 19, 2024

View reviewed changes

add timeout and update targetframeworks logic

9245d43

normj requested changes Nov 20, 2024

View reviewed changes

philasmar requested changes Nov 20, 2024

View reviewed changes

PR comments

cb98714

normj approved these changes Nov 20, 2024

View reviewed changes

src/Amazon.Common.DotNetCli.Tools/Utilities.cs Outdated Show resolved Hide resolved

Fix self contained bug

af70e6c

boblodgett requested a review from philasmar November 21, 2024 18:31

normj approved these changes Nov 21, 2024

View reviewed changes

philasmar approved these changes Nov 26, 2024

View reviewed changes

gcbeattyAWS merged commit b8b53d1 into dev Nov 26, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use MSBuild to find target framework #348

Use MSBuild to find target framework #348

gcbeattyAWS commented Nov 18, 2024 •

edited

Loading

normj Nov 19, 2024

gcbeattyAWS Nov 19, 2024

gcbeattyAWS Nov 19, 2024

normj Nov 19, 2024

gcbeattyAWS Nov 19, 2024

gcbeattyAWS Nov 19, 2024

normj Nov 19, 2024

gcbeattyAWS Nov 19, 2024

normj Nov 19, 2024

gcbeattyAWS Nov 19, 2024

normj Nov 20, 2024

philasmar Nov 20, 2024

gcbeattyAWS Nov 20, 2024

normj Nov 20, 2024

philasmar Nov 20, 2024

normj Nov 20, 2024

philasmar Nov 20, 2024

normj Nov 20, 2024

gcbeattyAWS Nov 20, 2024 •

edited

Loading

philasmar Nov 20, 2024

gcbeattyAWS Nov 20, 2024

gcbeattyAWS Nov 20, 2024

philasmar Nov 20, 2024

philasmar Nov 20, 2024

philasmar Nov 20, 2024

philasmar Nov 20, 2024

philasmar Nov 20, 2024

philasmar Nov 20, 2024

normj left a comment


		var xdoc = XDocument.Load(projectFile);
		var arguments = new[]

Use MSBuild to find target framework #348

Use MSBuild to find target framework #348

Conversation

gcbeattyAWS commented Nov 18, 2024 • edited Loading

Expected Behavior

Current Behavior

Changes

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gcbeattyAWS Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normj left a comment

Choose a reason for hiding this comment

gcbeattyAWS commented Nov 18, 2024 •

edited

Loading

gcbeattyAWS Nov 20, 2024 •

edited

Loading