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.

Sign up for GitHub

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

Jump to bottom

apply the change from aws-c-auth #590

Closed
wants to merge 13 commits into from
Closed

apply the change from aws-c-auth #590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
69a154b
apply the change from aws-c-auth
TingDaoK Aug 30, 2024
fb9867d
test aws-lc main
TingDaoK Sep 3, 2024
8294818
hmmm?
TingDaoK Sep 3, 2024
1cec299
is the check wrong?
TingDaoK Sep 3, 2024
c77a84b
GNU and x86
TingDaoK Sep 3, 2024
dcbb557
copy the check from aws-lc
TingDaoK Sep 3, 2024
00dedb1
clean it up and add comments
TingDaoK Sep 3, 2024
530d51b
submodules
TingDaoK Sep 3, 2024
f52c214
use latest aws-c-auth
TingDaoK Sep 3, 2024
94dfebf
add comments
TingDaoK Sep 4, 2024
af6950b
missing space
TingDaoK Sep 4, 2024
5114b54
update the comments
TingDaoK Sep 4, 2024
c8dcb53
that's better
TingDaoK Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions crt/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,15 @@ if(UNIX AND NOT APPLE)
set(MY_ASSEMBLER_IS_TOO_OLD_FOR_512AVX ON CACHE BOOL "Disable AVX512 on old GCC that not supports it")
endif()

string(TOLOWER "${CMAKE_SYSTEM_PROCESSOR}" CMAKE_SYSTEM_PROCESSOR_LOWER)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added this for aws/aws-lc@6fe8dcb to work on manylinux-1-x86, it's not released yet, but we will need this once it's

if(CMAKE_SYSTEM_PROCESSOR_LOWER MATCHES "x86_64|amd64|x86|i386|i686" AND CMAKE_SIZEOF_VOID_P EQUAL 4)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why doesn't AWS-LC's CMakeLists.txt handle this, vs having every consumer handle it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems weird to include x86_64 and amd64, since those are never 32bit.
And if you take those out, it's all 32-bit architectures, so you don't need to compare pointer size

Suggested change
if(CMAKE_SYSTEM_PROCESSOR_LOWER MATCHES "x86_64|amd64|x86|i386|i686" AND CMAKE_SIZEOF_VOID_P EQUAL 4)
if(CMAKE_SYSTEM_PROCESSOR_LOWER MATCHES "^(x86|i386|i686)$")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that aws-lc can still support machine without SSE2 with -DOPENSSL_NO_ASM to disable the assembly optimization. But, for us, we can assume we don't support 20 years old x86 CPUs.

Copy link
Contributor Author

@TingDaoK TingDaoK Sep 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/awslabs/aws-crt-python/actions/runs/10689757684/job/29632625853 line 429

-- XXXXXXXXX CMAKE_SYSTEM_PROCESSOR: x86_64

https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_PROCESSOR.html

this will correspond to the target architecture for the build, but this is not guaranteed. (E.g. on Windows, the host may be AMD64 even when using a MSVC cl compiler with a 32-bit target.)

So, I copied the logic from aws-lc. https://github.com/aws/aws-lc/blob/main/CMakeLists.txt#L791-L800
I'll copy the comments as well probably for clarity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah, I see in the CMAKE_SYSTEM_PROCESSOR docs that it may not be correct (emphasis mine):

In many cases, this will correspond to the target architecture for the build, but this is not guaranteed. (E.g. on Windows, the host may be AMD64 even when using a MSVC cl compiler with a 32-bit target.)

When cross-compiling, a CMAKE_TOOLCHAIN_FILE should set the CMAKE_SYSTEM_PROCESSOR variable to match target architecture that it specifies

Maybe edit the comment to be more like the docs, instead of some ramble about Docker.

Be wary that CMAKE_SYSTEM_PROCESSOR may not correspond to the target architecture when cross-compiling. (E.g. on Windows, the host may be AMD64 even when using a MSVC cl compiler with a 32-bit target). The CMAKE_TOOLCHAIN_FILE is supposed to set it correctly, but may not have.

Copy link
Contributor

Choose a reason for hiding this comment

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

or just

CMAKE_SYSTEM_PROCESSOR is supposed to match the target architecture when cross-compiling, but this is not guaranteed. See: https://cmake.org/cmake/help/v3.30/variable/CMAKE_SYSTEM_PROCESSOR.html

# Add -msse2 for x86-32bit because of https://github.com/aws/aws-lc/commit/6fe8dcbe96e580ea85233fdb98a142e42951b70b
# CMAKE_SYSTEM_PROCESSOR is supposed to match the target architecture when cross-compiling,
# but this is not guaranteed. See: https://cmake.org/cmake/help/v3.30/variable/CMAKE_SYSTEM_PROCESSOR.html
# So, check for both CMAKE_SIZEOF_VOID_P and CMAKE_SYSTEM_PROCESSOR_LOWER for x86-32bit
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -msse2")
endif()

add_subdirectory(aws-lc)
endif()

Expand Down
2 changes: 1 addition & 1 deletion crt/aws-c-auth
Open in desktop
Submodule aws-c-auth updated 29 files
+1 −1 .github/workflows/ci.yml
+11 −6 source/aws_signing.c
+3 −1 source/credentials_provider_cached.c
+2 −0 tests/CMakeLists.txt
+13 −0 tests/aws-signing-test-suite/v4/post-unsigned-payload/context.json
+10 −0 tests/aws-signing-test-suite/v4/post-unsigned-payload/header-canonical-request.txt
+1 −0 tests/aws-signing-test-suite/v4/post-unsigned-payload/header-signature.txt
+8 −0 tests/aws-signing-test-suite/v4/post-unsigned-payload/header-signed-request.txt
+4 −0 tests/aws-signing-test-suite/v4/post-unsigned-payload/header-string-to-sign.txt
+9 −0 tests/aws-signing-test-suite/v4/post-unsigned-payload/query-canonical-request.txt
+1 −0 tests/aws-signing-test-suite/v4/post-unsigned-payload/query-signature.txt
+6 −0 tests/aws-signing-test-suite/v4/post-unsigned-payload/query-signed-request.txt
+4 −0 tests/aws-signing-test-suite/v4/post-unsigned-payload/query-string-to-sign.txt
+5 −0 tests/aws-signing-test-suite/v4/post-unsigned-payload/request.txt
+1 −1 tests/aws-signing-test-suite/v4/post-vanilla/context.json
+1 −1 tests/aws-signing-test-suite/v4/post-x-www-form-urlencoded/context.json
+13 −0 tests/aws-signing-test-suite/v4a/post-unsigned-payload/context.json
+11 −0 tests/aws-signing-test-suite/v4a/post-unsigned-payload/header-canonical-request.txt
+1 −0 tests/aws-signing-test-suite/v4a/post-unsigned-payload/header-signature.txt
+9 −0 tests/aws-signing-test-suite/v4a/post-unsigned-payload/header-signed-request.txt
+4 −0 tests/aws-signing-test-suite/v4a/post-unsigned-payload/header-string-to-sign.txt
+4 −0 tests/aws-signing-test-suite/v4a/post-unsigned-payload/public-key.json
+9 −0 tests/aws-signing-test-suite/v4a/post-unsigned-payload/query-canonical-request.txt
+1 −0 tests/aws-signing-test-suite/v4a/post-unsigned-payload/query-signature.txt
+6 −0 tests/aws-signing-test-suite/v4a/post-unsigned-payload/query-signed-request.txt
+4 −0 tests/aws-signing-test-suite/v4a/post-unsigned-payload/query-string-to-sign.txt
+5 −0 tests/aws-signing-test-suite/v4a/post-unsigned-payload/request.txt
+6 −7 tests/credentials_provider_sts_tests.c
+23 −0 tests/sigv4_signing_tests.c
2 changes: 1 addition & 1 deletion crt/aws-c-cal
Open in desktop
Submodule aws-c-cal updated 1 files
+70 −26 source/unix/openssl_platform_init.c
2 changes: 1 addition & 1 deletion crt/aws-c-compression
Open in desktop
Submodule aws-c-compression updated 5 files
+12 −3 .github/workflows/ci.yml
+4 −6 .github/workflows/clang-format.yml
+47 −0 format-check.py
+0 −24 format-check.sh
+1 −1 source/compression.c
2 changes: 1 addition & 1 deletion crt/aws-lc
Open in desktop
Submodule aws-lc updated from 057477 to 2f1879
2 changes: 1 addition & 1 deletion crt/s2n
Open in desktop
Submodule s2n updated from 79c0f1 to 87f4a0
1 change: 0 additions & 1 deletion test/test_websocket.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,6 @@ def test_send_receive_data(self):
self.assertIsNone(handler.exception)

def test_send_frame_exceptions(self):
init_logging(LogLevel.Trace, 'stderr')
with WebSocketServer(self.host, self.port) as server:
handler = ClientHandler()
handler.connect_sync(self.host, self.port)
Expand Down
Loading