Skip to content

Crt http integration #2293 rebased#2376

Merged
sdavtaker merged 4 commits intomainfrom
sr/pr2293rebased
Mar 17, 2023
Merged

Crt http integration #2293 rebased#2376
sdavtaker merged 4 commits intomainfrom
sr/pr2293rebased

Conversation

@SergeyRyabinin
Copy link
Contributor

@SergeyRyabinin SergeyRyabinin commented Mar 2, 2023

Issue #, if available:
A rebase of
Crt http integration #2293
Description of changes:

Check all that applies:

  • Did a review by yourself.
  • Added proper tests to cover this PR. (If tests are not applicable, explain.)
  • Checked if this PR is a breaking (APIs have been changed) change.
  • Checked if this PR will not introduce cross-platform inconsistent behavior.
  • Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

  • Linux
  • Windows
  • Android
  • MacOS
  • IOS
  • Other Platforms

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@SergeyRyabinin SergeyRyabinin marked this pull request as ready for review March 3, 2023 19:33
CMakeLists.txt Outdated
option(BUILD_SHARED_LIBS "If enabled, all aws sdk libraries will be build as shared objects; otherwise all Aws libraries will be built as static objects" ON)
option(FORCE_SHARED_CRT "If enabled, will unconditionally link the standard libraries in dynamically, otherwise the standard library will be linked in based on the BUILD_SHARED_LIBS setting" ON)
option(SIMPLE_INSTALL "If enabled, removes all the additional indirection (platform/cpu/config) in the bin and lib directories on the install step" ON)
option(USE_CRT_HTTP_CLIENT "If enabled, The common runtime HTTP client will be used, and the legacy systems such as WinHttp and libcurl will not be built or included" OFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo after comma.

If enabled, The common runtime HTTP
should be
If enabled, the common runtime HTTP

#include <aws/core/config/AWSProfileConfigLoader.h>
#include <aws/core/internal/AWSHttpResourceClient.h>

#include <aws/crt/Api.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we wrap this in conditional compile path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's "AWS-CRT-CPP" wrapper, not the http client from CRT.

m_client = Aws::MakeUnique<TranscribeStreamingServiceClient>(ALLOC_TAG, config);
m_clientWithWrongCreds = Aws::MakeUnique<TranscribeStreamingServiceClient>(ALLOC_TAG, Aws::Auth::AWSCredentials("a", "b"), config);
config.endpointOverride = "https://0xxxabcdefg123456789.com";
//config.endpointOverride = "https://0xxxabcdefg123456789.com";
Copy link
Contributor

Choose a reason for hiding this comment

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

if no longer used, we should remove this line

@sdavtaker sdavtaker merged commit f53eeee into main Mar 17, 2023
@sdavtaker sdavtaker deleted the sr/pr2293rebased branch March 17, 2023 20:22
@jmklix jmklix mentioned this pull request Mar 24, 2023
4 tasks
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.

4 participants