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

Fix Erroneous "No BuildArchitecture specified in Layer" Warning #6508

Merged
merged 12 commits into from
Jan 8, 2024
Merged

Fix Erroneous "No BuildArchitecture specified in Layer" Warning #6508

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c9d5c67
Move Missing BuildArchitecture warning to build_layer
Jan 3, 2024
d181877
Formatting
Jan 3, 2024
e302c4f
Fix typo
Jan 4, 2024
457edf0
Narrow warnings to mismatches between buildArch and Compatible Archit…
Jan 4, 2024
bc59f9d
Fix some tests
Jan 5, 2024
9d3e38b
Fix integ test
Jan 5, 2024
c8afda2
More integ tests
Jan 5, 2024
1eef3e6
Format fix
Jan 5, 2024
0720024
Fix integ test
Jan 5, 2024
e43f5f6
Fix typo
Jan 5, 2024
1029dcb
Merge branch 'develop' into bentvels-fix-6444
bentvelj Jan 5, 2024
812adb4
Parameterize test with use_container
Jan 6, 2024
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
7 changes: 7 additions & 0 deletions samcli/lib/build/app_builder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,13 @@ def _build_layer(
str
Path to the location where built artifacts are available
"""

if layer_metadata and "BuildArchitecture" not in layer_metadata:
LOG.warning(
"WARNING: No BuildArchitecture specified in Layer `%s`" + " Metadata. Defaulting to x86_64.",
layer_name,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to print this warning message if layer has arm64 architecture as CompatibleArchitectures but it doesn't have BuildArchitecture property

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is another warning in the LayerVersion constructor that should take care of this.

It would output

WARNING: Layer `Layer` has BuildArchitecture `x86_64`, which is not listed in CompatibleArchitectures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may be slightly confusing if the customer doesn't know that x86 is the default? Maybe we can adjust the messaging here

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern with this one is; if I have a layer which is supposed to be built with x86 I will keep getting these warning messages all the time, which can be annoying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. As discussed, going to restructure according to these rules:

There are two cases where we'd like to warn the customer

  1. Compatible Architectures is only x86 (or not present) but Build Architecture is arm64
  2. Build Architecture is x86 (or not present) but Compatible Architectures is only arm64


# Create the arguments to pass to the builder
# Code is always relative to the given base directory.
code_dir = str(pathlib.Path(self._base_dir, codeuri).resolve())
Expand Down
6 changes: 0 additions & 6 deletions samcli/lib/providers/provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,12 +244,6 @@ def __init__(
self._compatible_runtimes = compatible_runtimes
self._custom_layer_id = metadata.get(SAM_RESOURCE_ID_KEY)

if "BuildArchitecture" not in metadata:
LOG.warning(
"WARNING: No BuildArchitecture specifed in Layer `%s`" + " Metadata. Defaulting to x86_64.",
self._custom_layer_id,
)

self._build_architecture = cast(str, metadata.get("BuildArchitecture", X86_64))
self._compatible_architectures = compatible_architectures

Expand Down
14 changes: 12 additions & 2 deletions tests/integration/buildcmd/test_build_cmd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1736,7 +1736,17 @@ def test_build_fails_with_missing_metadata(self, runtime, use_container, layer_i
self.assertEqual(command_result.process.returncode, 1)
self.assertFalse(self.default_build_dir.joinpath(layer_identifier).exists())

@parameterized.expand([("python3.7", False, "LayerOne"), ("python3.7", "use_container", "LayerOne")])
def test_function_build_succeeds_with_referenced_layer(self):
overrides = {"Runtime": "python3.8"}
cmdlist = self.get_command_list(
use_container=False, parameter_overrides=overrides, function_identifier="FunctionTwo"
)

command_result = run_command(cmdlist, cwd=self.working_dir)
self.assertEqual(command_result.process.returncode, 0)
self.assertNotIn("No BuildArchitecture specifed", str(command_result.stderr))

@parameterized.expand([("python3.8", False, "LayerOne"), ("python3.8", "use_container", "LayerOne")])
def test_build_with_missing_buildarchitecture(self, runtime, use_container, layer_identifier):
if use_container and (SKIP_DOCKER_TESTS or SKIP_DOCKER_BUILD):
self.skipTest(SKIP_DOCKER_MESSAGE)
Expand All @@ -1747,7 +1757,7 @@ def test_build_with_missing_buildarchitecture(self, runtime, use_container, laye
)
command_result = run_command(cmdlist, cwd=self.working_dir)
self.assertEqual(command_result.process.returncode, 0)
self.assertIn("No BuildArchitecture specifed", str(command_result.stderr))
self.assertIn("No BuildArchitecture specified", str(command_result.stderr))

@parameterized.expand([("python3.7", False), ("python3.7", "use_container")])
def test_build_function_and_layer(self, runtime, use_container):
Expand Down
10 changes: 10 additions & 0 deletions tests/integration/testdata/buildcmd/layers-functions-template.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,16 @@ Resources:
Layers:
- !Ref LayerOne

FunctionTwo:
Type: AWS::Serverless::Function
Properties:
Handler: !Ref Handler
Runtime: !Ref Runtime
CodeUri: !Ref CodeUri
Timeout: 600
Layers:
- !Sub arn:${AWS::Partition}:lambda:${AWS::Region}:012345678912:layer:layerName:1

LayerOne:
Type: AWS::Serverless::LayerVersion
Properties:
Expand Down
Loading