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.

Sign up for GitHub

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

Switch --runtime value to linux-x64 #112

Merged
merged 3 commits into from
Mar 23, 2020
Merged

Switch --runtime value to linux-x64 #112

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
058a7d9
Change what the value set --runtime when calling dotnet CLI commands …
normj Mar 23, 2020
37ccdaa
Update comment for old RID constants
normj Mar 23, 2020
cfc3bdc
Respond to PR comments
normj Mar 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Amazon.Lambda.Tools/LambdaConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ public static class LambdaConstants
// https://github.com/dotnet/corefx/blob/release/1.0.0/pkg/Microsoft.NETCore.Platforms/runtime.json
internal const string RUNTIME_HIERARCHY = "netcore.runtime.hierarchy.json";

// The closest match to Amazon Linux
internal const string RUNTIME_HIERARCHY_STARTING_POINT = "rhel.7.2-x64";
// The runtime identifier used for older Lambda runtimes running on Amazon Linux 1.
internal const string LEGACY_RUNTIME_HIERARCHY_STARTING_POINT = "rhel.7.2-x64";


public const string AWS_LAMBDA_MANAGED_POLICY_PREFIX = "AWSLambda";
Expand Down
40 changes: 29 additions & 11 deletions src/Amazon.Lambda.Tools/LambdaDotNetCLIWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Diagnostics;
using System.Runtime.InteropServices;
using Amazon.Common.DotNetCli.Tools;
using System.Linq;

namespace Amazon.Lambda.Tools
{
Expand Down Expand Up @@ -81,7 +82,9 @@ public int Store(LambdaToolsDefaults defaults, string projectLocation, string ou
}

arguments.Append($" --manifest \"{fullPackageManifest}\"");
arguments.Append($" --runtime {LambdaConstants.RUNTIME_HIERARCHY_STARTING_POINT}");


arguments.Append($" --runtime {LambdaUtilities.DetermineRuntimeParameter(targetFramework)}");

if(!enableOptimization)
{
Expand Down Expand Up @@ -207,23 +210,38 @@ public int Publish(LambdaToolsDefaults defaults, string projectLocation, string
{
arguments.Append(" /p:GenerateRuntimeConfigurationFiles=true");

// If you set the runtime to RUNTIME_HIERARCHY_STARTING_POINT it will trim out the Windows and Mac OS specific dependencies but Razor view precompilation
// will not run. So only do this packaging optimization if there are no Razor views.
Comment on lines -210 to -211
Copy link

Choose a reason for hiding this comment

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

Could you comment on why this is different for "netcoreapp2.0" and "netcoreapp2.1"? Why you don't need p:PreserveCompilationContext for other targets? Also why you don't need to check for Razor views for other targets?

Also this comment was useful, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added comment

if (Directory.GetFiles(fullProjectLocation, "*.cshtml", SearchOption.AllDirectories).Length == 0)
if (new string[] { "netcoreapp2.0", "netcoreapp2.1" }.Contains(targetFramework))
{
arguments.Append($" -r {LambdaConstants.RUNTIME_HIERARCHY_STARTING_POINT}");
if(Directory.GetFiles(fullProjectLocation, "*.cshtml", SearchOption.AllDirectories).Length == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we still have this, the comment reason that got deleted still seems important (With slight modifications).

Copy link
Member Author

Choose a reason for hiding this comment

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

Added comment

{
arguments.Append($" -r {LambdaConstants.LEGACY_RUNTIME_HIERARCHY_STARTING_POINT}");
Copy link

Choose a reason for hiding this comment

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

You could do arguments.Append($" -r {LambdaUtilities.DetermineRuntimeParameter(targetFramework)}"); instead and have the same code for both "netcoreapp2.0", "netcoreapp2.1" and other targets.

You can also consider moving the common configurations for both "netcoreapp2.0", "netcoreapp2.1" and other targets to a function or adjust the ifs to avoid duplicating the code.

You can disregard this if you think it is more readable like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Switch to call LambdaUtilities.DetermineRuntimeParameter and put the setting of runtime and self contained in a shared Action.


if (msbuildParameters == null ||
msbuildParameters.IndexOf("--self-contained", StringComparison.InvariantCultureIgnoreCase) == -1)
{
arguments.Append(" --self-contained false ");
}

if (string.IsNullOrEmpty(msbuildParameters) ||
!msbuildParameters.Contains("PreserveCompilationContext"))
{
_logger?.WriteLine("... Disabling compilation context to reduce package size. If compilation context is needed pass in the \"/p:PreserveCompilationContext=false\" switch.");
arguments.Append(" /p:PreserveCompilationContext=false");
}
}
}
else
{
if (msbuildParameters == null ||
msbuildParameters.IndexOf("--self-contained", StringComparison.InvariantCultureIgnoreCase) == -1)
msbuildParameters.IndexOf("--runtime", StringComparison.InvariantCultureIgnoreCase) == -1)
{
arguments.Append(" --self-contained false ");
arguments.Append($" -r {LambdaUtilities.DetermineRuntimeParameter(targetFramework)}");
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe --runtime and -r are the same. Should probably pick one. Also this seems like it could end up duplicating the same thing already added always on line 87.

Copy link
Member Author

Choose a reason for hiding this comment

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

Standardized on --runtime

}

if (string.IsNullOrEmpty(msbuildParameters) ||
!msbuildParameters.Contains("PreserveCompilationContext"))
if (msbuildParameters == null ||
msbuildParameters.IndexOf("--self-contained", StringComparison.InvariantCultureIgnoreCase) == -1)
{
_logger?.WriteLine("... Disabling compilation context to reduce package size. If compilation context is needed pass in the \"/p:PreserveCompilationContext=false\" switch.");
arguments.Append(" /p:PreserveCompilationContext=false");
arguments.Append(" --self-contained false ");
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/Amazon.Lambda.Tools/LambdaPackager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ private static IList<string> CalculateRuntimeHierarchy()

// Use a queue to do a breadth first search through the list of runtimes.
var queue = new Queue<string>();
queue.Enqueue(LambdaConstants.RUNTIME_HIERARCHY_STARTING_POINT);
queue.Enqueue(LambdaConstants.LEGACY_RUNTIME_HIERARCHY_STARTING_POINT);

while(queue.Count > 0)
{
Expand Down
23 changes: 22 additions & 1 deletion src/Amazon.Lambda.Tools/LambdaUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,27 @@ public static ConvertManifestContentToSdkManifestResult ConvertManifestContentTo
var updatedContent = updatedDoc.ToString();

return new ConvertManifestContentToSdkManifestResult(true, updatedContent);
}
}

/// <summary>
/// Determines what runtime identifier (RID) to use when running a dotnet CLI command. For the
/// older .NET Lambda runtimes that are not running on Amazon Linux 2 keep using the existing rhel.7.2-x64
/// RID. For all other runtimes that are running on Amazon Linux 2 use the newer linux-x64 RID which did
/// not exist when this tool was first created.
/// </summary>
/// <param name="targetFramework"></param>
/// <returns></returns>
public static string DetermineRuntimeParameter(string targetFramework)
{
switch (targetFramework)
{
case "netcoreapp1.0":
case "netcoreapp2.0":
case "netcoreapp2.1":
return LambdaConstants.LEGACY_RUNTIME_HIERARCHY_STARTING_POINT;
default:
return "linux-x64";
}
}
}
}
2 changes: 1 addition & 1 deletion test/Amazon.Common.DotNetCli.Tools.Test/UtilitesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace Amazon.Common.DotNetCli.Tools.Test
public class UtilitesTests
{
[Theory]
[InlineData("../../../../../testapps/TestFunction", "netcoreapp1.0")]
[InlineData("../../../../../testapps/TestFunction", "netcoreapp2.1")]
[InlineData("../../../../../testapps/ServerlessWithYamlFunction", "netcoreapp2.1")]
[InlineData("../../../../../testapps/TestBeanstalkWebApp", "netcoreapp2.1")]
public void CheckFramework(string projectPath, string expectedFramework)
Expand Down
15 changes: 15 additions & 0 deletions test/Amazon.Lambda.Tools.Test/UtilitiesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,20 @@ public void TestGettingLayerDescriptionForNonDotnetLayer(string fullLayer, strin
var value = LambdaUtilities.DetermineListDisplayLayerDescription(fullLayer, maxLength);
Assert.Equal(displayLayer, value);
}

[Theory]
[InlineData("netcoreapp1.0", "rhel.7.2-x64")]
[InlineData("netcoreapp1.1", "linux-x64")]
[InlineData("netcoreapp2.0", "rhel.7.2-x64")]
[InlineData("netcoreapp2.1", "rhel.7.2-x64")]
[InlineData("netcoreapp2.2", "linux-x64")]
[InlineData("netcoreapp3.0", "linux-x64")]
[InlineData("netcoreapp3.1", "linux-x64")]
[InlineData("netcoreapp6.0", "linux-x64")]
public void TestDetermineRuntimeParameter(string targetFramework, string expectedValue)
{
var runtime = LambdaUtilities.DetermineRuntimeParameter(targetFramework);
Assert.Equal(expectedValue, runtime);
}
}
}