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.

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: validate build target for AWS Lambda, and check target component in toolchain #39

Merged
merged 14 commits into from Mar 31, 2022

Conversation

rnag
Copy link
Contributor

@rnag rnag commented Mar 29, 2022

Hi @calavera, just creating this as a draft PR for you to review when you have time; hopefully this resolves #30.

I've also added a test for adding the target component to the toolchain, and confirmed it works as expected. It looks like the check to confirm if a target component is present in the toolchain should be "zero cost", as it involves just a lookup to see if a directory exists or not in the rustup home.

--

Below are the changes added as part of this feature:

  • validate that the build target passed in to --target is currently supported for deployments to AWS Lambda
  • check if the target component is installed in the host toolchain, and install it with rustup target add <component> if needed

@rnag rnag changed the title Feature: validate build target for AWS Lambda, and check target component in toolchain feat: validate build target for AWS Lambda, and check target component in toolchain Mar 29, 2022
calavera
calavera previously approved these changes Mar 29, 2022
Copy link
Collaborator

@calavera calavera left a comment

Choose a reason for hiding this comment

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

This looks really good @rnag !

@rnag
Copy link
Contributor Author

rnag commented Mar 29, 2022

Awesome, am glad to hear it! I went ahead and marked this PR as open for review in that case.

I also marked the one test as ignored since it's technically an integration test, and ideally might want to skip running it on CI when using cargo test.

@rnag rnag marked this pull request as ready for review March 29, 2022 18:03
@calavera
Copy link
Collaborator

I don't understand why the workflows didn't run 🤔

@calavera calavera self-requested a review March 29, 2022 21:02
src/toolchain.rs Outdated Show resolved Hide resolved
@calavera calavera self-requested a review March 29, 2022 21:33
Copy link
Collaborator

@calavera calavera left a comment

Choose a reason for hiding this comment

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

well, I have no idea why GitHub doesn't give me the option to trigger the workflows for your PR. I'll run some tests locally and I'll merge it if everything looks good on my end.

src/build.rs Outdated Show resolved Hide resolved
@messense
Copy link
Contributor

I don't understand why the workflows didn't run 🤔

FYI, you removed the pull_request event in 15af83c#diff-5c3fa597431eda03ac3339ae6bf7f05e1a50d6fc7333679ec38e21b337cb6721R1

@calavera
Copy link
Collaborator

FYI, you removed the pull_request event in 15af83c#diff-5c3fa597431eda03ac3339ae6bf7f05e1a50d6fc7333679ec38e21b337cb6721R1
oh, I see. I was under the impression that push events were enough. I'll add it back, thanks @messense !

@rnag
Copy link
Contributor Author

rnag commented Mar 30, 2022

I reverted changes and moved the let final_target up to L60. I think this way it's a few less changes overall.

@calavera
Copy link
Collaborator

Thanks @rnag, I'll run local tests again this evening.

Copy link
Collaborator

@calavera calavera left a comment

Choose a reason for hiding this comment

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

This works great now, thanks @rnag !

I'll release a new minor version this weekend with this new feature. Thanks a lot for your help 🤘

@calavera calavera merged commit 9bb2229 into cargo-lambda:main Mar 31, 2022
@rnag
Copy link
Contributor Author

rnag commented Mar 31, 2022

@calavera No problem, glad it helps out and I agree it simplifies the build process a bit. I also found a minor issue when doing local testing recently, where if we pass in glibc version in --target then it checks for the wrong target component in the toolchain. I opened a PR #44 to address this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check if the target toolchain is installed before building, and install it if it's missing
4 participants