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

ifndef BYO_CRYPTO #144

Merged
merged 4 commits into from
Jul 20, 2021
Merged

ifndef BYO_CRYPTO #144

merged 4 commits into from
Jul 20, 2021

Conversation

ilevyor
Copy link
Contributor

@ilevyor ilevyor commented Jul 19, 2021

Issue #, if available:

Description of changes:

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

Copy link
Contributor

@justinboswell justinboswell left a comment

Choose a reason for hiding this comment

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

I don't understand this change, the body_streaming_elg exists whether or not BYO_CRYPTO is enabled, how does this relate?

Copy link
Contributor

@TingDaoK TingDaoK left a comment

Choose a reason for hiding this comment

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

it's complaining about on_error not used for BYO_CRYPTO, I guess you need to add something like goto on_error on line 339. And raise some error

@ilevyor
Copy link
Contributor Author

ilevyor commented Jul 19, 2021

commit is necessary due to BYO_CRYPTO builds failing in aws-crt-cpp
https://github.com/awslabs/aws-crt-cpp/runs/3106110215?check_suite_focus=true

/root/aws-crt-cpp/crt/aws-c-s3/source/s3_client.c:411:1: error: label 'on_error' defined but not used [-Werror=unused-label] on_error:

/* If the initial request already has partNumber, the request is not splittable(?). Treat it as a Default
* request.
* TODO: Still need tests to verify that the request of a part is splittable or not */
if (aws_http_headers_has(initial_message_headers, aws_byte_cursor_from_c_str("partNumber"))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why delete this part? And have you tried to use this branch for aws-crt-cpp and see the CI is happy or not?

Copy link
Contributor Author

@ilevyor ilevyor Jul 19, 2021

Choose a reason for hiding this comment

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

mistake, fixed with new commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ilevyor ilevyor requested a review from TingDaoK July 19, 2021 20:24
@ilevyor ilevyor merged commit dca8df1 into main Jul 20, 2021
@ilevyor ilevyor deleted the byo_fix branch July 20, 2021 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants