Skip to content

fix: Enable --manifest flag for esbuild and npm build #513

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

Merged
moelasmar merged 3 commits into from
Jun 8, 2023
Merged

fix: Enable --manifest flag for esbuild and npm build #513

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae44cef
feat: support manifest exist outside the source directory for NPM.
moelasmar Jun 7, 2023
dbd868b
feat: support manifest exist outside the source directory for NPM ESB…
moelasmar Jun 7, 2023
3df1474
remove print
moelasmar Jun 7, 2023
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
24 changes: 14 additions & 10 deletions aws_lambda_builders/actions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import os
import shutil
from pathlib import Path
from typing import Iterator, Set, Tuple, Union
from typing import Iterator, Optional, Set, Tuple, Union

from aws_lambda_builders import utils
from aws_lambda_builders.utils import copytree, create_symlink_or_copy
Expand Down Expand Up @@ -169,14 +169,15 @@ class CopyDependenciesAction(BaseAction):

PURPOSE = Purpose.COPY_DEPENDENCIES

def __init__(self, source_dir, artifact_dir, destination_dir, maintain_symlinks=False):
def __init__(self, source_dir, artifact_dir, destination_dir, maintain_symlinks=False, manifest_dir=None):
self.source_dir = source_dir
self.artifact_dir = artifact_dir
self.dest_dir = destination_dir
self.manifest_dir = manifest_dir
self.maintain_symlinks = maintain_symlinks

def execute(self):
deps_manager = DependencyManager(self.source_dir, self.artifact_dir, self.dest_dir)
deps_manager = DependencyManager(self.source_dir, self.artifact_dir, self.dest_dir, self.manifest_dir)

for dependencies_source, new_destination in deps_manager.yield_source_dest():
if os.path.islink(dependencies_source) and self.maintain_symlinks:
Expand All @@ -198,13 +199,14 @@ class MoveDependenciesAction(BaseAction):

PURPOSE = Purpose.MOVE_DEPENDENCIES

def __init__(self, source_dir, artifact_dir, destination_dir):
def __init__(self, source_dir, artifact_dir, destination_dir, manifest_dir=None):
self.source_dir = source_dir
self.artifact_dir = artifact_dir
self.dest_dir = destination_dir
self.manifest_dir = manifest_dir

def execute(self):
deps_manager = DependencyManager(self.source_dir, self.artifact_dir, self.dest_dir)
deps_manager = DependencyManager(self.source_dir, self.artifact_dir, self.dest_dir, self.manifest_dir)

for dependencies_source, new_destination in deps_manager.yield_source_dest():
# shutil.move can't create subfolders if this is the first file in that folder
Expand Down Expand Up @@ -256,10 +258,11 @@ class DependencyManager:
# This allows for the installation of dependencies in the source directory
IGNORE_LIST = ["node_modules"]

def __init__(self, source_dir, artifact_dir, destination_dir) -> None:
def __init__(self, source_dir, artifact_dir, destination_dir, manifest_dir=None) -> None:
self._source_dir: str = source_dir
self._artifact_dir: str = artifact_dir
self._dest_dir: str = destination_dir
self._manifest_dir: Optional[str] = manifest_dir
self._dependencies: Set[str] = set()

def yield_source_dest(self) -> Iterator[Tuple[str, str]]:
Expand All @@ -268,12 +271,13 @@ def yield_source_dest(self) -> Iterator[Tuple[str, str]]:
yield os.path.join(self._artifact_dir, dep), os.path.join(self._dest_dir, dep)

def _set_dependencies(self) -> None:
source = self._get_source_files_exclude_deps()
source = self._get_source_files_exclude_deps(self._source_dir)
manifest = self._get_source_files_exclude_deps(self._manifest_dir) if self._manifest_dir else set()
artifact = set(os.listdir(self._artifact_dir))
self._dependencies = artifact - source
self._dependencies = artifact - (source.union(manifest))

def _get_source_files_exclude_deps(self) -> Set[str]:
source_files = set(os.listdir(self._source_dir))
def _get_source_files_exclude_deps(self, dir_path: str) -> Set[str]:
source_files = set(os.listdir(dir_path))
for item in self.IGNORE_LIST:
if item in source_files:
source_files.remove(item)
Expand Down
36 changes: 30 additions & 6 deletions aws_lambda_builders/workflows/nodejs_npm/workflow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,25 +74,49 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim

npm_copy_npmrc_and_lockfile = NodejsNpmrcAndLockfileCopyAction(tar_package_dir, source_dir, osutils=osutils)

self.manifest_dir = self.osutils.dirname(self.manifest_path)
is_building_in_source = self.build_dir == self.source_dir
is_external_manifest = self.manifest_dir != self.source_dir
self.actions = [
npm_pack,
npm_copy_npmrc_and_lockfile,
CopySourceAction(tar_package_dir, artifacts_dir, excludes=self.EXCLUDED_FILES),
]

if is_external_manifest:

@mildaniel mildaniel Jun 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm a little confused by this. I thought that today this feature worked for regular Node.js builds.

@moelasmar moelasmar Jun 7, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

not sure if it was a bug or a regression, but I verified that it is not working for Node. Build will succeed, but actually if you try to run sam local invoke it will fail, as the source code does not exist in the artifacts directory

@moelasmar moelasmar Jun 7, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

$ sam build --manifest manifest/package.json 

	SAM CLI now collects telemetry to better understand customer needs.

	You can OPT OUT and disable telemetry collection by setting the
	environment variable SAM_CLI_TELEMETRY=0 in your shell.
	Thanks for your help!

	Learn More: https://docs.aws.amazon.com/serverless-application-model/latest/developerguide/serverless-sam-telemetry.html

Building codeuri: /Volumes/workplace/sam-apps/bug_4832/sam-app-node/hello-world runtime: nodejs18.x metadata: {} architecture: x86_64     
functions: HelloWorldFunction                                                                                                             
Running NodejsNpmBuilder:NpmPack                                                                                                          
Running NodejsNpmBuilder:CopyNpmrcAndLockfile                                                                                             
Running NodejsNpmBuilder:CopySource                                                                                                       
Running NodejsNpmBuilder:NpmInstall                                                                                                       
Running NodejsNpmBuilder:CleanUpNpmrc                                                                                                     
Running NodejsNpmBuilder:LockfileCleanUp                                                                                                  

Build Succeeded

Built Artifacts  : .aws-sam/build
Built Template   : .aws-sam/build/template.yaml

Commands you can use next
=========================
[*] Validate SAM template: sam validate
[*] Invoke Function: sam local invoke
[*] Test Function in the Cloud: sam sync --stack-name {{stack-name}} --watch
[*] Deploy: sam deploy --guided

@moelasmar moelasmar Jun 7, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

$ sam local invoke 
Invoking app.lambdaHandler (nodejs18.x)                                                                                                   
Local image is up-to-date                                                                                                                 
Using local image: public.ecr.aws/lambda/nodejs:18-rapid-x86_64.                                                                          
                                                                                                                                          
Mounting /Volumes/workplace/sam-apps/bug_4832/sam-app-node/.aws-sam/build/HelloWorldFunction as /var/task:ro,delegated, inside runtime    
container                                                                                                                                 
START RequestId: ed901fe3-6a35-4cfd-a70c-7a7e4d4a7ea0 Version: $LATEST
2023-06-07T17:10:20.910Z	undefined	ERROR	Uncaught Exception 	{"errorType":"Runtime.ImportModuleError","errorMessage":"Error: Cannot find module 'app'\nRequire stack:\n- /var/runtime/index.mjs","stack":["Runtime.ImportModuleError: Error: Cannot find module 'app'","Require stack:","- /var/runtime/index.mjs","    at _loadUserApp (file:///var/runtime/index.mjs:997:17)","    at async UserFunction.js.module.exports.load (file:///var/runtime/index.mjs:1032:21)","    at async start (file:///var/runtime/index.mjs:1195:23)","    at async file:///var/runtime/index.mjs:1201:1"]}
07 Jun 2023 17:10:21,025 [ERROR] (rapid) Init failed error=Runtime exited with error: exit status 129 InvokeID=
2023-06-07T17:10:22.198Z	undefined	ERROR	Uncaught Exception 	{"errorType":"Runtime.ImportModuleError","errorMessage":"Error: Cannot find module 'app'\nRequire stack:\n- /var/runtime/index.mjs","stack":["Runtime.ImportModuleError: Error: Cannot find module 'app'","Require stack:","- /var/runtime/index.mjs","    at _loadUserApp (file:///var/runtime/index.mjs:997:17)","    at async UserFunction.js.module.exports.load (file:///var/runtime/index.mjs:1032:21)","    at async start (file:///var/runtime/index.mjs:1195:23)","    at async file:///var/runtime/index.mjs:1201:1"]}
END RequestId: 9aa21ed6-ab13-44df-a7a3-0b1bc26adb8b
REPORT RequestId: 9aa21ed6-ab13-44df-a7a3-0b1bc26adb8b	Init Duration: 1.00 ms	Duration: 2689.49 ms	Billed Duration: 2690 ms	Memory Size: 128 MB	Max Memory Used: 128 MB	
{"errorType":"Runtime.ImportModuleError","errorMessage":"Error: Cannot find module 'app'\nRequire stack:\n- /var/runtime/index.mjs","trace":["Runtime.ImportModuleError: Error: Cannot find module 'app'","Require stack:","- /var/runtime/index.mjs","    at _loadUserApp (file:///var/runtime/index.mjs:997:17)","    at async UserFunction.js.module.exports.load (file:///var/runtime/index.mjs:1032:21)","    at async start (file:///var/runtime/index.mjs:1195:23)","    at async file:///var/runtime/index.mjs:1201:1"]}

@moelasmar moelasmar Jun 7, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

but if I used the new lambda builders library:

$ samdev build --manifest manifest/package.json 
2023-06-07 10:11:23 Attaching import module proxy for analyzing dynamic imports
Building codeuri: /Volumes/workplace/sam-apps/bug_4832/sam-app-node/hello-world runtime: nodejs18.x metadata: {} architecture: x86_64     
functions: HelloWorldFunction                                                                                                             
Running NodejsNpmBuilder:NpmPack                                                                                                          
Running NodejsNpmBuilder:CopyNpmrcAndLockfile                                                                                             
Running NodejsNpmBuilder:CopySource                                                                                                       
Running NodejsNpmBuilder:CopySource                                                                                                       
Running NodejsNpmBuilder:NpmInstall                                                                                                       
Running NodejsNpmBuilder:CleanUpNpmrc                                                                                                     
Running NodejsNpmBuilder:LockfileCleanUp                                                                                                  

Build Succeeded

Built Artifacts  : .aws-sam/build
Built Template   : .aws-sam/build/template.yaml

Commands you can use next
=========================
[*] Validate SAM template: sam validate
[*] Invoke Function: sam local invoke
[*] Test Function in the Cloud: sam sync --stack-name {{stack-name}} --watch
[*] Deploy: sam deploy --guided

$ sam local invoke 
Invoking app.lambdaHandler (nodejs18.x)                                                                                                   
Local image is up-to-date                                                                                                                 
Using local image: public.ecr.aws/lambda/nodejs:18-rapid-x86_64.                                                                          
                                                                                                                                          
Mounting /Volumes/workplace/sam-apps/bug_4832/sam-app-node/.aws-sam/build/HelloWorldFunction as /var/task:ro,delegated, inside runtime    
container                                                                                                                                 
START RequestId: ae2e371c-730d-48f3-8e4d-68745ab6a8c3 Version: $LATEST
END RequestId: ae2e371c-730d-48f3-8e4d-68745ab6a8c3
REPORT RequestId: ae2e371c-730d-48f3-8e4d-68745ab6a8c3	Init Duration: 1.02 ms	Duration: 2961.36 ms	Billed Duration: 2962 ms	Memory Size: 128 MB	Max Memory Used: 128 MB	
{"statusCode":200,"body":"{\"message\":\"hello world\"}"}

@moelasmar moelasmar Jun 7, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

so I also added some integration test cases to SAM CLI to cover this use case to avoid regressions in the future if it was a regression.

# npm pack only copies source code if the manifest is in the same directory as the source code, we need to
# copy the source code if the customer specified a different manifest path
self.actions.append(CopySourceAction(self.source_dir, artifacts_dir, excludes=self.EXCLUDED_FILES))

if self.download_dependencies:
self.actions.append(
NodejsNpmWorkflow.get_install_action(
source_dir=source_dir,
install_dir=self.build_dir,
# run npm install in the directory where the manifest (package.json) exists if customer is building
# in source, and manifest directory is different from source.
# This will let NPM find the local dependencies that are defined in the manifest file (they are
# usually defined as relative to the manifest location, and that is why we run `npm install` in the
# manifest directory instead of source directory).
# If customer is not building in source, so it is ok to run `npm install` in the build
# directory (the artifacts directory in this case), as the local dependencies are not supported.
install_dir=self.manifest_dir if is_building_in_source and is_external_manifest else self.build_dir,
subprocess_npm=subprocess_npm,
osutils=osutils,
build_options=self.options,
install_links=self.build_dir == self.source_dir,
install_links=is_building_in_source,
)
)

if self.download_dependencies and self.build_dir == self.source_dir:
if is_building_in_source and is_external_manifest:
# Since we run `npm install` in the manifest directory, so we need to link the node_modules directory in
# the source directory.
source_dependencies_path = os.path.join(self.source_dir, "node_modules")
manifest_dependencies_path = os.path.join(self.manifest_dir, "node_modules")
self.actions.append(
LinkSinglePathAction(source=manifest_dependencies_path, dest=source_dependencies_path)
)

if self.download_dependencies and is_building_in_source:
self.actions += self._actions_for_linking_source_dependencies_to_artifacts

# if no dependencies dir, just cleanup artifacts and we're done
Expand All @@ -107,9 +131,7 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim
# then copy them into the artifacts dir
elif self.combine_dependencies:
self.actions.append(
CopySourceAction(
self.dependencies_dir, artifacts_dir, maintain_symlinks=self.build_dir == self.source_dir
)
CopySourceAction(self.dependencies_dir, artifacts_dir, maintain_symlinks=is_building_in_source)
)

self.actions += self._actions_for_cleanup
Expand Down Expand Up @@ -145,6 +167,7 @@ def _actions_for_updating_dependencies_dir(self):
artifact_dir=self.artifacts_dir,
destination_dir=self.dependencies_dir,
maintain_symlinks=self.build_dir == self.source_dir,
manifest_dir=self.manifest_dir,
)
)
else:
Expand All @@ -153,6 +176,7 @@ def _actions_for_updating_dependencies_dir(self):
source_dir=self.source_dir,
artifact_dir=self.artifacts_dir,
destination_dir=self.dependencies_dir,
manifest_dir=self.manifest_dir,
)
)

Expand Down
39 changes: 34 additions & 5 deletions aws_lambda_builders/workflows/nodejs_npm_esbuild/workflow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@

import json
import logging
import os
from pathlib import Path

from aws_lambda_builders.actions import (
CleanUpAction,
CopySourceAction,
LinkSinglePathAction,
LinkSourceAction,
MoveDependenciesAction,
)
Expand Down Expand Up @@ -53,6 +55,10 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim
self.osutils = osutils or OSUtils()
self.subprocess_npm = SubprocessNpm(self.osutils)
self.subprocess_esbuild = self._get_esbuild_subprocess()
self.manifest_dir = self.osutils.dirname(self.manifest_path)

is_building_in_source = self.build_dir == self.source_dir
is_external_manifest = self.manifest_dir != self.source_dir

bundler_config = self.get_build_properties()

Expand Down Expand Up @@ -80,22 +86,45 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim
# if we're building in the source directory, we don't have to copy the source code
self.actions = (
[]
if self.build_dir == self.source_dir
if is_building_in_source
else [CopySourceAction(source_dir=self.source_dir, dest_dir=self.build_dir, excludes=self.EXCLUDED_FILES)]
)

if is_external_manifest and not is_building_in_source:
# copy the manifest file (package.json) to the build directory in case if the manifest file is not in the
# same directory as the source code, and customer is not building in source.
self.actions.append(
CopySourceAction(source_dir=self.manifest_dir, dest_dir=self.build_dir, excludes=self.EXCLUDED_FILES)
)

if self.download_dependencies:
self.actions.append(
NodejsNpmWorkflow.get_install_action(
source_dir=source_dir,
install_dir=self.build_dir,
# run npm install in the directory where the manifest (package.json) exists if customer is building
# in source, and manifest directory is different from source.
# This will let NPM find the local dependencies that are defined in the manifest file (they are
# usually defined as relative to the manifest location, and that is why we run `npm install` in the
# manifest directory instead of source directory).
# If customer is not building in source, so it is ok to run `npm install` in the build
# directory (the artifacts directory in this case), as the local dependencies are not supported.
install_dir=self.manifest_dir if is_building_in_source and is_external_manifest else self.build_dir,
subprocess_npm=self.subprocess_npm,
osutils=self.osutils,
build_options=self.options,
install_links=self.build_dir == self.source_dir,
install_links=is_building_in_source,
)
)

if is_building_in_source and is_external_manifest:
# Since we run `npm install` in the manifest directory, so we need to link the node_modules directory in
# the source directory.
source_dependencies_path = os.path.join(self.source_dir, "node_modules")

@mildaniel mildaniel Jun 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice! This is a clever solution.

manifest_dependencies_path = os.path.join(self.manifest_dir, "node_modules")
self.actions.append(
LinkSinglePathAction(source=manifest_dependencies_path, dest=source_dependencies_path)
)

bundle_action = EsbuildBundleAction(
working_directory=self.build_dir,
output_directory=self.artifacts_dir,
Expand All @@ -109,7 +138,7 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim
# If there's no dependencies_dir, just bundle and we're done.
# Same thing if we're building in the source directory (since the dependencies persist in
# the source directory, we don't want to move them or symlink them back to the source)
if not self.dependencies_dir or self.build_dir == self.source_dir:
if not self.dependencies_dir or is_building_in_source:
self.actions.append(bundle_action)
return

Expand All @@ -118,7 +147,7 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim
self.actions += [
bundle_action,
CleanUpAction(self.dependencies_dir),
MoveDependenciesAction(self.source_dir, self.scratch_dir, self.dependencies_dir),
MoveDependenciesAction(self.source_dir, self.scratch_dir, self.dependencies_dir, self.manifest_dir),
]
else:
# if we're reusing dependencies, then we need to symlink them before bundling
Expand Down
Loading