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

AWS_CRT_BUILD_FORCE_STATIC_LIBS #596

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

AWS_CRT_BUILD_FORCE_STATIC_LIBS #596

wants to merge 4 commits into from

Conversation

graebm
Copy link
Contributor

@graebm graebm commented Sep 12, 2024

Issue:
#593 introduced env-var, AWS_CRT_BUILD_USE_SYSTEM_LIBS=1, but it didn't work unless those system libs were built statically.

Diagnosis:
The reason it didn't work is: there's a hack in setup.py that forces dependencies to be linked statically on Unix variants (excluding macOS). It worked when I tested on macOS because the hack isn't applied there.

The reason for the hack is: in Brazil (internal AWS build system), dependencies are available as both static and dynamic libs. But we prefer to link the static ones so that, if a python application is packaged for AWS lambda, the developer has fewer files they need to chase down and copy into their .zip package.

Description of changes:
Don't force static libs to be used unless AWS_CRT_BUILD_FORCE_STATIC_LIBS=1 (new env-var). Otherwise (by default), the linker will use whatever version of the lib is available (if both are available, linkers generally prefer dynamic). This only applies to non-OS dependencies, we won't force libc to be linked statically.

It's unlikely this env-var will be used anywhere except Brazil.

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

@graebm graebm mentioned this pull request Sep 12, 2024
extra_link_args += ['-Wl,--no-execute-only']

# FreeBSD doesn't have execinfo as a part of libc like other Unix variant.
# Passing linker flag to link execinfo properly
if sys.platform.startswith('freebsd'):
extra_link_args += ['-lexecinfo']
libraries += ['execinfo']
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this wasn't broken, it was just weird we were adding this library via extra_link_args instead of via libraries

# hide the symbols from libcrypto.a
# this prevents weird crashes if an application also ends up using
# libcrypto.so from the system's OpenSSL installation.
# Do this even if using system libcrypto, since it could still be a static lib.
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 moved this out of the if-statement because:

Do this even if using system libcrypto, since it could still be a static lib.

# aws-lc produces libcrypto.a
AWS_LIBS.append(AwsLib('aws-lc', libname='crypto'))
# aws-lc produces libcrypto.a
AWS_LIBS.append(AwsLib('aws-lc', libname='crypto'))
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 did this so that aws-lc is in AWS_LIBS even if we're not building it, in the same way that all the other non-OS libs are in AWS_LIBS even if we're not building them. This removes some weirdness later where we had to add crypto back into the list of libraries we need to link against

@BwL1289
Copy link

BwL1289 commented Nov 5, 2024

@graebm my sincerest apologies for the long delay. We built from this PR and can confirm it works. Would appreciate a new version if and when you have a chance 👍.

Thanks again.

@graebm
Copy link
Contributor Author

graebm commented Nov 7, 2024

Oof bad timing. Some other recent build changes (1, 2, 3) are making things rocky for our team this last week. Once those issues settle down, I'll work on merging this

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