Skip to content

Conversation

@normj
Copy link
Member

@normj normj commented Nov 30, 2024

Issue #, if available:
#351

Description of changes:
In the recent release of Amazon.Lambda.Tools we switched to using dotnet msbuild getproperty command to evaluate MSBuild properties like PublishAot. It is possible for users to specify additional MSBuild parameters via the --msbuild-parameters switch that might affect the evaluation. That is the case of the linked issue.

This PR pass along the user specified value for --msbuild-parameters to the dotnet msbuild getproperty command so the evaluation has the full context for evaluation.

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

…arameters include user specified msbuild parameters.
@normj normj force-pushed the normj/use-msbuild-parameters branch from 47d76ee to cac10d2 Compare November 30, 2024 00:08
/// Looks up specified properties from a project.
/// </summary>
/// <param name="projectLocation">The location of the project file.</param>
/// <param name="msBuildParameters">Additonal MSBuild paramteres passed by the user from the commandline</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// <param name="msBuildParameters">Additonal MSBuild paramteres passed by the user from the commandline</param>
/// <param name="msBuildParameters">Additional MSBuild parameters passed by the user from the commandline</param>

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks fixed

[InlineData("TargetFramework", "", "net6.0")]
[InlineData("TargetFramework", "/p:NonExistence=net20.0", "net6.0")]
[InlineData("TargetFramework", "/p:TargetFramework=net20.0", "net20.0")]
public void TestPropertyEvaluationWithMSBuildParameters(string property, string msbuildparameters, string expectedValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth adding a test case for when more that one property is added by the user?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a couple more test scenarios.

@dscpinheiro dscpinheiro requested review from a user, 96malhar and philasmar December 3, 2024 18:32
@normj normj changed the base branch from master to dev December 16, 2024 17:28
@normj normj merged commit 5c1e258 into dev Dec 16, 2024
2 checks passed
@normj normj deleted the normj/use-msbuild-parameters branch December 16, 2024 17:29
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