feat: Makefile based builder for provided runtimes #166

sriram-mv · 2020-04-10T18:57:33Z

Why is this change necessary?

provided runtimes are runtimes that are custom built with own
bootstraps etc, and its hard to determine which language it actually is.
A generic makefile based builder gives the control back to the user on
how the project needs to be built.

How does it address the issue?

Provide a way to build for provided runtimes, which does not exist
today.

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

Why is this change necessary? * provided runtimes are runtimes that are custom built with own bootstraps etc, and its hard to determine which language it actually is. A generic makefile based builder gives the control back to the user on how the project needs to be built. How does it address the issue? * Provide a way to build for provided runtimes, which does not exist today.

awood45 · 2020-04-13T23:43:25Z

aws_lambda_builders/workflows/provided_make/DESIGN.md

+* Which environment variables are usable in this makefile?
+  * There are a series of whitelisted environment variables that need to be defined and not be overriden within the Makefile to work. Currently that is just `$ARTIFACTS_DIR`
+
+* Can this be used even for runtimes that have builders associated with it? eg: python3.8?


This is probably something we need to address in the design now. Even if it is not supported immediately, how will it be added.

Perhaps an optional build parameter where you can specify a Makefile explicitly, which then overrides any auto-detected build process? I can see though how this could be a problem across many functions, but that's a starting point.

The fact there are complexities may be a reason to come up with an answer before shipping this.

@awood45 This is something that should be considered in SAM CLI not lambda builders. Lambda builders is a library of builders that someone can run, how SAM CLI picks which one is completely within SAM CLI.

Yes, this would be choosing a workflow based on the manifest in SAM CLI. for eg: if a makefile is present and a build target is present, we choose the makefile workflow, else if we choose the bundled the workflow.

The logic to choose an appropriate workflow would need to be added there.

https://github.com/awslabs/aws-sam-cli/blob/develop/samcli/lib/build/workflow_config.py#L137

Even in sam-cli do we need to provide use of makefile for known workflows. I understand it is adding one more feature with some flexibility. But is it needed? What problem will this be solving? I think it'll add another case which then need lot of testing to make sure we didn't miss an edge case.

I would think it would be opting on a makefile workflow if necessary. Not neccesarily forced. if you are using a makefile based build approach, you would be responsible for the build instead.

Why is this change necessary? * Tests to ensure that the makefile based approach for provided runtimes works.

Why is this change necessary? * On Windows, certain env variables are required for functioning of binaries within the Makefile How does it address the issue? * pass current environmental variables along with new ones over to the subprocess runner. What side effects does this change have? * None

Why is this change necessary? * on powershell on windows vs git bash on windows, paths are treated differently. How does it address the issue? * Dynamically change path by looking up `sh`. If present, its unix path else its windows paths. What side effects does this change have? * `sh` is the check which determines which path to use. If `sh` aint present, this will end up in breaking the workflow.

c2tarun · 2020-04-13T18:37:50Z

aws_lambda_builders/workflows/provided_make/DESIGN.md

+Sample Makefile:
+
+````
+build-HelloWorldFunction:


Shouldn't this be HelloWorldFunction-build?

this is how the design and implementation are designed at this point. Is there a reason you prefer it the other way around?

Nope in line 21 it says {Function_Logical_Id]-build that is why I said.

Good catch!

c2tarun · 2020-04-16T21:06:46Z

aws_lambda_builders/workflows/provided_make/DESIGN.md

+* Which environment variables are usable in this makefile?
+  * There are a series of whitelisted environment variables that need to be defined and not be overriden within the Makefile to work. Currently that is just `$ARTIFACTS_DIR`
+
+* Can this be used even for runtimes that have builders associated with it? eg: python3.8?


Even in sam-cli do we need to provide use of makefile for known workflows. I understand it is adding one more feature with some flexibility. But is it needed? What problem will this be solving? I think it'll add another case which then need lot of testing to make sure we didn't miss an edge case.

c2tarun · 2020-04-16T21:09:39Z

aws_lambda_builders/workflows/provided_make/actions.py

+    def _artifact_dir_path(self):
+        # This is required when running on windows to determine if we are running in linux
+        # subsystem or on native cmd or powershell.
+        return Path(self.artifacts_dir).as_posix() if self.osutils.which("sh") else self.artifacts_dir


Does this consider other types of shells? like zsh, csh etc? On that note, I think we should have special case for windows environment, which have rather low number of shells. Linux have lot of shells.

this is a good point, we can check if we are on windows before we start looking for any shell.

c2tarun · 2020-04-16T21:22:54Z

aws_lambda_builders/workflows/provided_make/make.py

+        LOG.debug("executing Make: %s", invoke_make)
+
+        p = self.osutils.popen(
+            invoke_make, stdout=self.osutils.pipe, stderr=self.osutils.pipe, cwd=cwd, env=env if env else {}


nit: why don't we default env to {} instead of None?

sure,we can.

c2tarun · 2020-04-16T21:24:08Z

aws_lambda_builders/workflows/provided_make/make.py

+
+        LOG.debug("executing Make: %s", invoke_make)
+
+        p = self.osutils.popen(


I have noticed that we have an OSUtil for os specific calls. Should have a POpenUtil for open specific calls with consistent logging around the actual call for all workflows? It'll also avoid some code duplication.

probably. but the osutils are desinged in a manner such that each is independent, so that each workflow can in essence be its own module. But open to considering it.

Why is this change necessary? * on unix, its not necessary to transform the artifacts_dir path. How does it address the issue? * Do a check if we are windows, before attempting to look for binaries. What side effects does this change have? * None.

jfuss

Overall this looks really good. Nothing major in my feedback, mostly just small things or suggestions.

jfuss · 2020-04-20T16:52:01Z

aws_lambda_builders/workflows/provided_make/DESIGN.md

+When the makefile is called through a lambda builder workflow, the appropriate target is triggered and artifacts should be copied to the exposed environmental variable `$ARTIFACTS_DIR`. This environment variable is seeded by the lambda builder with a value set to it. The targets are defined as 
+
+```
+build-{Function_Logical_Id}


I think this is a great place to start, but think customers will be asking for a make build target as well. This would allow additional support for common builds between functions and remove the need to maintain a build for each Logical Id.

its easy to setup such a target by the user themselves and throw common stuff there. if there is a general target it becomes hard to do logical separation from building perspective, especially when multiple functions might share the same Makefile.

jfuss · 2020-04-20T16:55:11Z

aws_lambda_builders/workflows/provided_make/actions.py

+LOG = logging.getLogger(__name__)
+
+
+class ProvidedMakeAction(BaseAction):


Should we be labeling this Provided? I can see us wanting to expand this out to other runtimes, like Go/Python/etc in the future.

we can rename it to just custom. its more generic that way.

jfuss · 2020-04-20T16:56:30Z

aws_lambda_builders/workflows/provided_make/actions.py

+        :type osutils: aws_lambda_builders.workflows.provided_make.utils.OSUtils
+        :param osutils: An instance of OS Utilities for file manipulation
+
+        :type subprocess_make aws_lambda_builders.workflows.provided_make.make.SubprocessMake


Forgot build_logical_id doc strings.

jfuss · 2020-04-20T16:57:35Z

aws_lambda_builders/workflows/provided_make/actions.py

+        self.subprocess_make = subprocess_make
+        self.build_logical_id = build_logical_id
+
+    def _artifact_dir_path(self):


Should these be a @property?

jfuss · 2020-04-20T17:01:15Z

aws_lambda_builders/workflows/provided_make/make.py

+        :type args: list
+        :param args: Command line arguments to pass to Make
+
+        :type args: dict


Should this be env?

Suggested change

:type args: dict

:type env: dict

jfuss · 2020-04-20T17:02:50Z

aws_lambda_builders/workflows/provided_make/utils.py

+from aws_lambda_builders.utils import which
+
+
+class OSUtils(object):


The famous Copy-paste OSUtils. We should make a generic one eventually

yeah. we should.

jfuss · 2020-04-20T17:04:06Z

aws_lambda_builders/workflows/provided_make/utils.py

+        return p
+
+    def environ(self):
+        return os.environ


Shouldn't this be a os.environ.copy()? That way we don't risk mutating customers env var by accident? Some workflows already do this (like Go Dep for example).

jfuss · 2020-04-20T17:05:19Z

aws_lambda_builders/workflows/provided_make/workflow.py

+
+        # Find the logical id of the function to be built.
+        options = kwargs.get("options") or {}
+        build_logical_id = options.get("build_logical_id", None)


What happens when this is not provided?

Should we just error out here if it is a requirement of the builder?

yeah we would error out saying that the required make file target was not present.

jfuss · 2020-04-20T17:10:40Z

tests/integration/workflows/provided_make/test_provided_make.py

+        self.assertEquals(expected_files, output_files)
+
+    def test_must_build_python_project_through_makefile_unknown_target(self):
+        with self.assertRaises(WorkflowFailedError):


Should we have a way to make this more specific of an Error?

jfuss · 2020-04-20T17:11:40Z

tests/unit/workflows/provided_make/test_actions.py

+
+class TestProvidedMakeAction(TestCase):
+    def setUp(self):
+        self.original_env = os.environ


Why is this change necessary? * Without this, the make manifest file is always assumed to be present with the source_dir. How does it address the issue? * An explicitly passed in manifest file path is now not ignored. What side effects does this change have? * None.

jfuss · 2020-04-22T16:03:56Z

aws_lambda_builders/workflows/custom_make/actions.py

+            self.osutils.makedirs(self.artifacts_dir)
+
+        try:
+            current_env = self.osutils.environ().copy()


We should just do this in OSUtils so we don't have to remember to copy anytime we access the self.osutils.environ().

awood45 reviewed Apr 14, 2020

View reviewed changes

sriram-mv marked this pull request as draft April 14, 2020 21:08

sriram-mv force-pushed the provided_builder branch 6 times, most recently from 0ccce7b to b7080bf Compare April 15, 2020 17:22

tests: add unit, functional and integration tests

37a7bd1

Why is this change necessary? * Tests to ensure that the makefile based approach for provided runtimes works.

sriram-mv force-pushed the provided_builder branch 3 times, most recently from 5634fe5 to 0e0d1a9 Compare April 15, 2020 22:18

sriram-mv force-pushed the provided_builder branch 2 times, most recently from 3658e58 to 8698121 Compare April 16, 2020 17:17

fix: does this work on windows?

1fddb92

sriram-mv force-pushed the provided_builder branch from 8698121 to 1fddb92 Compare April 16, 2020 17:43

chore: add pytest logging

2ff7196

sriram-mv force-pushed the provided_builder branch 2 times, most recently from 59ae79a to 20d786c Compare April 16, 2020 20:46

sriram-mv force-pushed the provided_builder branch from 20d786c to aafed9b Compare April 16, 2020 21:14

sriram-mv marked this pull request as ready for review April 16, 2020 21:15

sriram-mv changed the title ~~[WIP] feat: Makefile based builder for provided runtimes~~ feat: Makefile based builder for provided runtimes Apr 16, 2020

c2tarun reviewed Apr 16, 2020

View reviewed changes

sriram-mv added 2 commits April 16, 2020 15:54

fix: only check for shells on windows

76065c3

Why is this change necessary? * on unix, its not necessary to transform the artifacts_dir path. How does it address the issue? * Do a check if we are windows, before attempting to look for binaries. What side effects does this change have? * None.

fix: default for env is None, not {}

f335e67

jfuss suggested changes Apr 20, 2020

View reviewed changes

fix: address comments

3f296c2

c2tarun previously approved these changes Apr 21, 2020

View reviewed changes

sriram-mv dismissed c2tarun’s stale review via f16710c April 30, 2020 19:29

sriram-mv mentioned this pull request May 6, 2020

SAM Build support for Provided runtime aws/aws-sam-cli#1099

Closed

jfuss reviewed May 7, 2020

View reviewed changes

jfuss previously approved these changes May 7, 2020

View reviewed changes

fix: address comments

6af13ee

sriram-mv dismissed jfuss’s stale review via 6af13ee May 7, 2020 18:36

sriram-mv force-pushed the provided_builder branch from aeddcf7 to 6af13ee Compare May 7, 2020 18:36

sriram-mv requested a review from jfuss May 7, 2020 18:37

jfuss approved these changes May 7, 2020

View reviewed changes

sriram-mv merged commit d47d42a into aws:develop May 7, 2020


		LOG.debug("executing Make: %s", invoke_make)

		p = self.osutils.popen(

		LOG = logging.getLogger(__name__)


		class ProvidedMakeAction(BaseAction):

		from aws_lambda_builders.utils import which


		class OSUtils(object):

feat: Makefile based builder for provided runtimes #166

feat: Makefile based builder for provided runtimes #166

Uh oh!

Conversation

sriram-mv commented Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfuss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sriram-mv commented Apr 10, 2020 •

edited

Loading