-
Notifications
You must be signed in to change notification settings - Fork 703
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
Allow static and shared libs to be mixed. #3467
Conversation
Currently, users cannot build a shared lib that uses a static libs2n.a, and they cannot build a static lib that uses a shared libs2n.so. With this change, a library can use either static libs2n.a or shared libs2n.so. If both are static and shared libs2n are installed, BUILD_SHARED_LIBS serves as a tie-breaker.
Can you add a script to test that this all works as intended? I just want to make sure this behavior is preserved and doesn't break in the future. You can see an example script for the libcrypto interning support. |
Yeah, I'll put something together. Good suggestion |
Added a test similar to the interning one some tests are failing ... but they don't seem related to my changes. What now? |
nevermind my last comment. the tests that did run all seem to have passed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few call-outs around libcrypto use.
# create installation dir with libs2n.so | ||
if [ ! -d $WORK_DIR/s2n-install-shared ]; then | ||
(set -x; cmake -B$WORK_DIR/s2n-build-shared -DCMAKE_INSTALL_PREFIX=$WORK_DIR/s2n-install-shared -DBUILD_SHARED_LIBS=ON ${COMMON_S2N_BUILD_ARGS[@]}) | ||
(set -x; cmake --build $WORK_DIR/s2n-build-shared --target install) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --target install
seems to cause cmake to link against the system libcrypto (openssl 1.1 on ubuntu18). Setting S2N_LIBCRYPTO
to anything else causes this test to fail (built vs linked). Might be good to check the S2N_LIBCRYPTO/LIBCRYPTO_ROOT and bail out with something like only supported on openssl-1.1.1 in CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clue that --target install
was doing something fishy.
Turns out the install step strips off the RPATH/RUNPATH.
I pushed a fix that sets LD_LIBRARY_PATH to point at libcrypto.so's location. That way an installed s2n can still use the same version of libcrypto.so it was built against, even if libcrypto.so isn't in an offical system lib directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests passed with aws-lc locally, lgtm.
Resolved issues:
This is part of a fix for an issue from another repo: aws4embeddedlinux/meta-aws#191
Description of changes:
Currently, users cannot build a shared lib that uses a static libs2n.a, and they cannot build a static lib that uses a shared libs2n.so.
With this change, a library can use either static libs2n.a or shared libs2n.so. If both are static and shared libs2n are installed, BUILD_SHARED_LIBS serves as a tie-breaker.
Testing:
Local testing:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.