-
Notifications
You must be signed in to change notification settings - Fork 142
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.
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
feat: support rust via cargo #174
Conversation
Exception.__init__(self, self.MESSAGE.format(**kwargs)) | ||
|
||
|
||
class OSUtils(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment from a first time contributor. These utils while helpful seem to be duplicated for just about every builder that does something with popen. Perhaps it would be useful to extract a common util so that new builders didn't need to continue the copy process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly code debt now. I can't quite remember exactly why we elected to do this in the beginning, but there was some reason. Over time, it just became 'easier' but we should extract these out. If you want to do it, I would open up a new PR for it but I wouldn't make that refactoring a requirement for this PR to get through.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Over time, it just became 'easier' but we should extract these out.
The irony is it made it harder for me. I had to copy util code and in doing so had to copy the tests for that util code to uphold this projects codecov requirements.
If you want to do it, I would open up a new PR
I'd plotted out the time to set aside to get these changes worked through and estimate the time to get this integrated into the separate sam cli repo before starting this journey.
Given the review turnaround time, I don't anticipate being able to invest additional time in cleaning up this project's existing tech debt. It would be a good investment on behalf of the project maintainers though. I just wanted to capture in a note some of the experience from a first time contributor incase that was useful to project maintainers.
Build metadata emitted by cargo | ||
""" | ||
(package, binary) = parse_handler(self.handler) | ||
exists = any( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize this expression is kind of dense to parse but I didn't know if there was a more pythonic way of expressing this beyond a series of maps
and anys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm.. Let me think through this and parse it out. This is hard to understand in the current state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the tests for this to guide your understanding.
# we utilize the handler identifier to | ||
# select the binary to build | ||
options = kwargs.get("options") or {} | ||
handler = options.get("artifact_executable_name", None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm reusing this interface used also by the go runtime to pass along a reference to a handler identifier. I didn't want to reinvent another interface
from aws_lambda_builders.workflows.rust_cargo.actions import OSUtils | ||
|
||
|
||
class TestOSUtils(TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
related to the above comment about copies of osutils. since there is a codecov % barrier to entry, it felt weird having to also copy tests to cover the copied util to keep that codecov %
edition = "2018" | ||
|
||
[dependencies] | ||
lambda = { git = "https://github.com/awslabs/aws-lambda-rust-runtime" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
depending on a git rev here is temporary until we can release a new version of the runtime awslabs/aws-lambda-rust-runtime#216
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now they have tagged releases, worth changing.
@softprops . You do not need an explicit rust cargo builder anymore, with the release of 0.9.0 of lambda builders and 0.51.0 of aws sam cli. You can use a makefile builder directly. I'm closing this PR. |
@sriram-mv not to sound too unsatisfied with that solution but I highly encourage you to reconsider reopening if you care about developer experience Yes, you can you make to build a rust lambda. The honest truth is you can also use make to build any of the other lambda runtimes. It's extremely unlikely you will see users of other platforms do that . Users often perfer use thier platform's provided tooling to guide thier workflow. In the case of rust, that's cargo. In my experience, very few rust projects use make. They use cargo. I can say from some personal experience from working within the rust community for more than 5 years, zero of my rust projects use make. It would become onerous to add a dependency on make when sam exists can could use cargo directly. With Sam's manifest hooks, this also means that Sam could intelligently identify the correct builder for the target language. This makes running In addition to make being an uncommon workflow for rust users, those that would use make need to conjure the knowledge how to build a binary that's executable on the lambda runtime. That would ultimately result in having to cargo cult implementation detail in every rust project that ideally could live in the deployment tool, in this case sam. In case of this repo, the goal I see is to specifically encode knowledge of how to build and package artifacts that can be deployed on lambda. Rust users have a specific recipe but sam would be asking for them to figure it out themselves. Would you reconsider reopening in light of that? My hope here is that Sam would optimize for the same developer experience afforded to other languages supported. I would like to see the bar set a bit higher for rust. |
@sriram-mv I agree with @softprops. Trying to standardize around using Make, despite Rust already having Cargo, is a mistake. |
Re-opening this PR. What was showcased with the makefile builder is that there is integration within sam cli to help build rust functions today. The makefile build target also uses cargo. However, Happy to look at and accept builder workflows into |
Thank you for reconsidering and being option to having a discussion before closing this out.
I totally understand from a maintainers perspective limiting the number of builders to be maintained. That that sense exposing make or bash builder would be all this repo needs. The tradeoff that makes is that it would greatly reduce the value proposition for using sam in the first place. Folks use sam today because it knows the workflows for how to use the platform tooling of language ecosystems to build lambdas for users. In the case of rust, asking folks to use make instead of cargo means two things.
|
It seems there are some issues in ci with windows. I'll take a look at those today. I was able to validate locally with this projects but running ...
aws_lambda_builders/workflows/rust_cargo/__init__.py 1 0 0 0 100%
aws_lambda_builders/workflows/rust_cargo/actions.py 86 3 21 0 95% 31, 34-35
aws_lambda_builders/workflows/rust_cargo/workflow.py 16 0 0 0 100%
-------------------------------------------------------------------------------------------------------------------
TOTAL 2019 88 374 31 94%
============================================================================ 350 passed in 18.20 seconds =============================================================================
(samcli37) ~/c/p/aws-lambda-builders ❯❯❯ make black test |
I'm now down to just the integration tests failing for windows which I anticipated in my pull description above. I found a lead on a way to resolve that issue in https://github.com/KodrAus/rust-cross-compile. I'll try some experiments and see if I can't get windows integration tests to pass |
windows integration tests are now passing! |
|
||
### Copy and Rename executable | ||
|
||
It then copies the executable to the target directory renaming the executable to "bootstrap" to honor the provided runtime's [expectation on executable names](https://docs.aws.amazon.com/lambda/latest/dg/runtimes-custom.html). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@softprops I currently have the following serverless.yml
that I would like to migrate to this SAM approach:
https://github.com/brainstorm/htsget-aws/blob/master/serverless.yml#L29
Before I do, would it be possible to have both reads
and variants
functions deployed and not just a single bootstrap
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replied here #174 (comment)
Hi @brainstorm. I maintain the serverless plugin as well as contribute to the rust runtime so I can clarify. The The rust runtime is an embedded language runtime meaning it is statically compiled with your application as a dependency. A An approach I recommend for rust projects with multiple lambdas is using cargo workspaces where each lambda is a separate member of that workspace. This allows you to limit the dependencies to only what that lambda needs as well as limit the IAM permissions to only what each lambda needs. Going back to your serverless framework example. The plugin actually works the same way. It maps a handler name to a cargo package/binary. In this specific example you are using the http trigger in both so having a single lambda that dispatches is relatively straightforward (dispatching on request path). You could actually do that with your deployment today but you will also be able to do that with Sam. Make sense? |
It does, thanks Doug! |
Pushed up one more change. While trying to integration similar changes into my serverless-framework rust plugin I found cases were building lambda on any linux didn't always were because glibc may vary on the linux host the lambda was built on and the linux the lambda runtime is deployed to. I found that using musl insulated local builds from this at the expense of needing to install sudo apt-get update && sudo apt-get install -y musl-tools |
@brainstorm what's the typical turn around on reviews on these? |
d588a21
to
ca7b2dd
Compare
rebased on develop to get the latest changes |
Oops, just as I approve this, CI goes red, bummer :_/ I know you'll fix it soon though, seems like you are just cleaning up and hit something that broke this particular build. Anyway, no idea about turn around times for this specific AWS team (not employed by AWS myself either). I know for a fact that AWS CDK folks are really fast reviewing and merging contributions from community. But then again, AWS is huge, many teams with different prios. /cc @davidbarsky |
|
||
### Build | ||
|
||
It builds a binary in the standard cargo target directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will want to make this configurable like all the other builders. When SAM CLI builds, it places all the artifacts into a .aws-sam/build
directory in the project. This is so we can:
- keep the source clean
- update the template to the new location of the build code.
This is kind of baked in and not sure we want to change that behavior now. To support this, we will want to be able to build into 'sam cli's target dir' when that configuration is passed into this library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way this is setup for rust is not unlike other workflows. The target directory for is provided as am argument to the workflow. The section below is there the builder copies the deployable artifact while also renaming it, the provided runtime expects the entry point to be a well known executable called "bootstap". This is covered in tests
|
||
Cargo builds binaries targeting host platforms. When the host platform is not the same as the target platform it leverages a parameterized notion of a named target, typically installed and managed by [rustup](https://github.com/rust-lang/rustup). In the case of `sam` we default this target to to `x86_64-unknown-linux-musl` when the host is not linux based. | ||
|
||
Users can simply run `rustup target add x86_64-unknown-linux-musl` if needed. It's also possible for sam to detect this when needed and do that for users by parsing the output of `rustup target list --installed` but it would be unusual for sam to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you think this is unusual behavior? How will we communicate that customers need to do this before running build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For rust customers this isn't exactly unusual. Rustup is the default installation tool for rust.
I would imagine possibly in a readme of sorts there's some documentation for individual runtimes. I would add the note there. I can use some guidance on the current state of customer documentation entry points.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To extend that thought there are likely two kinds of customers for any given runtime.
-
those that know lambda but don't know the runtime's target language
-
those that know the runtime's targeted language
That's how I would structure documentation for Sam. I just don't know where to put that documentation today beyond the design docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a bit too forward looking, but let's say that AWS could (potentially?) build lambdas to be run on to of Graviton2 for the underlying lambda runner infra... I reckon that SAM should be able to retarget to those automatically? (given a specific setting on the SAM template).
Exception.__init__(self, self.MESSAGE.format(**kwargs)) | ||
|
||
|
||
class OSUtils(object): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly code debt now. I can't quite remember exactly why we elected to do this in the beginning, but there was some reason. Over time, it just became 'easier' but we should extract these out. If you want to do it, I would open up a new PR for it but I wouldn't make that refactoring a requirement for this PR to get through.
|
||
def cargo_metadata(self): | ||
p = self.osutils.popen( | ||
["cargo", "metadata", "--no-deps", "--format-version=1"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is --format-version=1
is this formatting the output or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this doc stable rust features are often designed with retaining stability in mind. In this case the cargo metadata
command provides a way to structural stable way to extract project information.
In the previous pr an external oython toml dependency was added to manually parse out a binary name from a cargo.toml file. I found the extra dependency was not needed when a stable way to access the same information was built directly into the build tool in a way that is not likely to break as cargo evolves.
|
||
def build_command(self, package): | ||
cmd = [self.binaries["cargo"].binary_path, "build", "-p", package, "--target", "x86_64-unknown-linux-musl"] | ||
if self.mode != BuildMode.DEBUG: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we invert this to if self.mode == BuildMode.Release
. That way if we introduce a new mode, we are not blanketly applying --release
too it?
Unless maybe this makes sense for Rust. Does Rust build a debuggable artifact by default unless --release
is added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Rust build a debuggable artifact by default unless --release is added?
Correct. Rust builds debug artifacts by default. The default assumes you are building something you will run locally then asks you to pass --release for a deployment optimized artifact.
Sam is the opposite you assume deployment artifact by default and debug on demand
Inverting would largely depend on Sam or some higher level construct consitently passing debug mode workflow rather than something like None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if you feel strongly about this. I'd like to try to make the implementation follow sams semantics, default to deploy unless opting into debug.
Build metadata emitted by cargo | ||
""" | ||
(package, binary) = parse_handler(self.handler) | ||
exists = any( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm.. Let me think through this and parse it out. This is hard to understand in the current state.
options = kwargs.get("options") or {} | ||
handler = options.get("artifact_executable_name", None) | ||
system_platform = platform.system().lower() | ||
self.actions = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why we don't follow the 'scratch directory` concept the rest of the builders use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you provide some direction? These seems like it would not provide addition benefit for rust and further complicated the workflow implementation.
The way rust handles build isolation is it's builtin notion of a target directory. The deployable components of that are copied to Sams notion of a target dir, the one provided as an argument to the workflow.
from aws_lambda_builders.exceptions import WorkflowFailedError | ||
|
||
|
||
class TestRustCargo(TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know Rust won't compile if something is wrong. This means, we will want to ensure that the build logs and error messages are properly surfaced back to customers. I think adding some integ test around this will help us know:
- We are providing enough info back to customers about why their build failed.
- Help us not regress these actions accidentally in the future.
Would you be able to add a couple tests in this area?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rusts compilation errors are family help at understanding why a build failed and what to do about it. Rather than reinvent that it's exposed directly through the popen stdout/err hooks
I can see if there's a way to structure a passing test for a failing build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added an integration test for a failing build
@softprops I understand more of the concerns others are thinking about. Here is the dump: We do want to support Rust and other builders for provided like runtimes. We should be able to add the builder here and support this into SAM CLI in the future. Writing the builder here (as you did), is the right thing. Alex was looking at this holistically (SAM CLI + Lambda Builders) not just Lambda Builders. There are a couple spots that we will want to address. The current thought is to handle thinking these through before we merge this builder in so we don't surprise ourselves when we support the building in SAM CLI. It may help to open a small design PR to SAM CLI explaining the experience end to end. Concern 1Right now SAM CLI selects a builder based upon the runtime and the manifest file (in this case cargo.toml). This works ok when a runtime maps to a single manifest file, like python -> requirements.txt. For cases like Java, where we support building with gradle and maven, we select one and run with it. This isn't a great user experience, since the customer cannot tell us which builder they actually want to run. The provided runtime is/will turn into the Java case. Meaning as we accept more and more provided runtime builders, we will start auto-selecting which workflow to run. This might not always lead to the outcome a customer wants. So what we will want to do, is think through how we can support multiple builders for a given runtime. One way we can do this is through reading some known metadata in the Metadata section on the resource. This what we did to support building for Layers within SAM CLI. Concern 2We need a way to support building within a container for provided runtimes. From my understanding, the setup for cross compiling with rust is not very friendly. One way around this would be to build within a container. The challenge here is the provided images will not have what rust needs by default and we don't want to create a huge image for all build tools for all provided runtimes (that would take forever to download). For this, we need a way to enable the container building for provided and give customers the flexibility they may need/want. Thinking as I type: One solution may be to introduce a flag to build an image to build within or allow the a customer to say 'build within this image' as a command line option to Hopefully this make expectations more clear to you and helps in understanding our thought process behind adding this to SAM. Happy to talk through ideas and thoughts on how we can address the 2 concerns above. |
Thanks for your feedback! For concern 1. Can we not let the perfect be the enemy of good? I realize that sam's architecture is not perfect but I'd argume against delaying adding value for paying aws customers with interest in leaning into rust when it's possible to expose that value today. You can argue in numbers where folks are using lambda today to priorize this by runtime but I'd say the flaw in that argument is that customers are going to flow towards what's made easy. If rust is possible but not easy it's likely never going to be the case you'll see the same kinds of usage numbers across runtime's. I can say out side of the serverless framework deploying rust on lambda is not easy. I realize sam's goal is to make the workflow for deploying to lambda easy. That's why I'm here and trying to help! I believe if workflow selector configs were configured in order of most specific to least specific the concerns above with provided runtime would be addressed. If this is still a concern I believe sam has prompting built in. It would I'm theory be possible to be explicit about conflict resolution For concern 2. I've maintained the serverless framework plugin for rust for years and it was only until the last release that customers had to build rust lambda in docker containers. The trade off this makes its that a portion of that time is spent maintaining an extension of the lambdaci community projects provided runtime builder with current versions of rust installed. While not terrible the tradeoff from customer feedback I've learned is that with dockerized build environments some customers will always have on additional dependency not included in the build container. I'm assuming sam has not overcome that hurdle yet. That said I know how to make this work if that's a blocker. The drawback is that Sam maintainers would have to take on the responsibility for maintaining said container and I see this project has enough to maintain as it stands today! Customer feedback from serverless framework has motivated the dockerless build direction specifically to unblock those use cases and to reduce project maintenance.
That was my initial impression as well. A good portion of the work that went to this pull was researching the factual reality of that assumption. In practice, it's not free but it's actually easier than expected to accommodate. I mentioned these notes on the design doc.
This is precisely an alternative I ended up doing with serverless framework plugin. |
noticing another ci failure also unrelated to these changes.
|
On your response to Concern 1: |
My apologies. I misunderstood the manifest selector to be the disambigutor not a source of ambiguity. All workflows that use this today have the design flaw that its possible that they all can conflict with each other when present in the same repository.
This is was what I was referring to when I said the design of sam is not perfect As I understand this is an existing problem Not one this change set introduces. My suggestion above was to use the builtin prompt interface if needed and provide a flag to be explicit of needed. With rust specifically I think the concern is valid in rare cases. Rarely does the rust community leverage the make build tool for these types because cargo is a build tool. Cargo exists so that we don't need make for common build cases. I hear you that this is possibly not true for other usages of the provided runtime that are not rust. I want to re-emphasize I'm here as a paying aws customer that would like aws to provide a better developer experience. There are alternatives with serverless framework and those might actually be the best tool for this specific problem today An area were sam might benefit from serverless framework design is to decouple these capabilities and make them user plugins. This allows the customer to leverage the value a tool offers while not being blocked by its feature development. Anecdotally it took a day to make this work in the rust plugin for serverless framework and it was published for use the next. |
@jfuss I think there's a ci tooling issue that's preventing me from seeing positive results from tests I see locally. Can someone on your team assist? This is the second ci error unrelated to me changes I hit so far.
|
@softprops : looking into the the pylint issue, looks to be breaking just on ubuntu python3.6 Edit: I re-ran the CI, and tests seem to be passing. |
Lovin it. Thanks! |
The official SAM makefile-centric Rust example docs seem to be slightly off?. Apparently it should be
Perhaps the docs could point to a minimal example repo instead? I would rather prefer to have this pullrequest merged for better developer experience, IMHO... |
Also, the documented Rust example does not seem to locate the
I guess I shall find some |
Solved the previous issue by:
Not ideal, but 🤷🏻♂️ ... also that cannot be written in the Makefile because that hardlinking apparently needs to happen before the Makefile gets parsed/executed?... 🤦🏻♂️ Now I'm getting into the lambda payload itself, finally:
... did I mention I really wish this PR got merged instead of going through this DX? |
Just a quick update. Something I'd been waiting for happened awslabs/aws-lambda-rust-runtime#294 (comment) The runtime as been in flux for quite a while so the examples here were based in a git branch and not a crates release. I should be able to fix that. I'll also need to rebase. It's been a while! I'm still trying to gauge interest this from the sam team. It's something that rust engineers would love. |
|
||
Cargo builds binaries targeting host platforms. When the host platform is not the same as the target platform it leverages a parameterized notion of a named target, typically installed and managed by [rustup](https://github.com/rust-lang/rustup). In the case of `sam` we default this target to to `x86_64-unknown-linux-musl` when the host is not linux based. | ||
|
||
Users can simply run `rustup target add x86_64-unknown-linux-musl` if needed. It's also possible for sam to detect this when needed and do that for users by parsing the output of `rustup target list --installed` but it would be unusual for sam to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reckon that with the new provided.al2, all references to MUSL should be removed for simplicity since x86_64-unknown-linux
works fine, potentially faster and with less object size issues?
This would be so awesome. 👍 |
Closing as there's the new PR #350 |
Issue #167
Description of changes:
👋 About a year has passed on since any activity on #67 so I thought I'd kick the tires on this and give it another try. I'm taking a similar approach with a few notable differences
Cargo.toml
aloneA point of interest I haven't been able to test is what support for windows looks like. Is it possible to not let windows be a blocker for the linux and mac osx population?Update: I've got windows support working
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.