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

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

wants to merge 13 commits into from

Conversation

TingDaoK
Copy link
Contributor

@TingDaoK TingDaoK commented Sep 3, 2024

submodules update:

aws-c-auth         v0.7.25 -> v0.7.27
aws-c-cal          v0.7.3 -> v0.7.4
aws-c-common       v0.9.27 ✓
aws-c-compression  v0.2.18 -> v0.2.19
aws-c-event-stream v0.4.3 ✓
aws-c-http         v0.8.8 ✓
aws-c-io           v0.14.18 ✓
aws-c-mqtt         v0.10.4 ✓
aws-c-s3           v0.6.4 ✓
aws-c-sdkutils     v0.1.19 ✓
aws-checksums      v0.1.18 ✓
aws-lc             v1.33.0 -> v1.34.2
s2n                v1.5.0 -> v1.5.1

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

@@ -40,6 +40,12 @@ 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

@@ -40,6 +40,12 @@ 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)
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

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

@TingDaoK TingDaoK requested a review from graebm September 4, 2024 20:29
@TingDaoK
Copy link
Contributor Author

aws/aws-lc#1841

@TingDaoK TingDaoK closed this Sep 13, 2024
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.

2 participants