-
Notifications
You must be signed in to change notification settings - Fork 152
Add -trimpath to GoModulesBuilder
#389
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
Conversation
|
Overall, this looks good. These options will need to be passed in from AWS SAM CLI as well here: https://github.com/aws/aws-sam-cli/blob/develop/samcli/lib/build/app_builder.py#L715 |
sriram-mv
left a comment
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 am happy to approve with a small test change.
| pathname = Path(self.artifacts_dir, "no-deps-main-arm64") | ||
| self.assertEqual(get_executable_arch(pathname), "AArch64") | ||
|
|
||
| def test_builds_with_trimpath(self): |
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 add a test to show the difference between when trim path is enabled vs not enabled?
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.
@sriram-mv @mndeveci I have added this test case. I wrote a utility that will return the md5 hexdigest of the built binaries. I created a copy of the no-deps go project in testdata. By building an exact copy of that project with -trimpath enabled we can assert that their md5 hex digests are the same. Similarly, we can assert that they will not be equal without the -trimpath flag. This was the most straightforward way that I could think of. Hopefully it is sufficient.
mndeveci
left a comment
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 for your contribution! I think changes looks good to me. Can we add an intergration test to validate this case? Integration test file can be found here; https://github.com/aws/aws-lambda-builders/blob/develop/tests/integration/workflows/go_modules/test_go.py#L18
sriram-mv
left a comment
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 for making the changes!
mndeveci
left a comment
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 for your contribution, looks good to me as well!
|
@scrofungulus to use this effectively, the next step is to make changes in AWS SAM CLI as well. since it passes in the options based on the builder that is chosen. Those changes are showcased here: https://github.com/aws/aws-sam-cli/blob/develop/samcli/lib/build/app_builder.py#L714 Would you be open to contributing those changes there as well? |
|
@sriram-mv I'm definitely open to contributing! Looks straightforward. |
|
@sriram-mv pull request number 4300 addresses your above comment! 😃 |
This PR addresses the issue described here in the AWS SAM CLI repository.
Description of changes:
Adds a
trim_go_pathoption to theGoModulesBuilder. Directly from thego help buildcommand from the Go CLI:Ultimately, this addresses an issue that occurs when using
sam build. Without-trimpaththe compiled binaries computed hashes are different each time forcing CloudFormation to redeploy a Lambda function without code changes.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.