-
-
Notifications
You must be signed in to change notification settings - Fork 646
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
Add warning when building AWS Lambdas and Google Cloud Functions on macOS. #13790
Conversation
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
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.
Thank you! Would you be willing to also please update google_cloud_function/python/rules.py
? It's almost entirely copy and pasted from AWS Lambda. It has the same problem with macOS
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Thanks for the feedback :) Updated now. |
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.
Looks great! Only one small issue.
@@ -54,6 +55,17 @@ class PythonAwsLambdaFieldSet(PackageFieldSet): | |||
async def package_python_awslambda( | |||
field_set: PythonAwsLambdaFieldSet, lambdex: Lambdex, union_membership: UnionMembership | |||
) -> BuiltPackage: | |||
|
|||
if Platform.is_macos: | |||
logger.warning( |
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 might be useful information if the packing fails, but it's otherwise continual noise every time this is run and packaging succeeds. How about switching to a FallibleProcessResult and then conditioning this warning on both macOS and process failure?
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.
Agreed that would be a superior experience.
Per discussion at #11941, implementing it is trickier. We need to
- Add a
FalliblePex
type - Allow augmenting a
ProcessExecutionFailure
with additional details.- I started on this last week as part of go: suggesting
go mod tidy
may not always work #13136, but Benjy and I decided to punt on the ticket and the design work of how to show the extra info.
- I started on this last week as part of go: suggesting
So I suggested that using a warning is a good first start to get some progress.
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.
Erm. Ok. I'm not a fan of bridging the gap with noise, but looks like others are.
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.
Opened #13865 to track that second task.
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Thanks for this contribution! |
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.
Went ahead and pushed a fix for Black.
Thanks for the great change!
# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Fixes #11941.
[ci skip-rust]
[ci skip-build-wheels]