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

chore: bundle sqlite by default for tests #3382

Merged

Conversation

stringhandler
Copy link
Collaborator

Description

Yet another attempt at bundling openssl and sqlite for easy debugging/testing on Windows

Motivation and Context

The current feature setup means that you cannot run cargo test in the wallet_ffi or any specific folder other than the root. I tried to follow the same way that other crates do this, but allowing a user to optionally specify whether a dependency is bundled or not, and not bundle it by default, but this makes running individual tests difficult.

Since tests are run regularly, I made the default on wallet_ffi include the bundles, but then added --no-default-features to the mobile builds

How Has This Been Tested?

Cargo test

@delta1
Copy link
Contributor

delta1 commented Sep 23, 2021

LGTM just waiting on CI

delta1
delta1 previously approved these changes Sep 23, 2021
philipr-za
philipr-za previously approved these changes Sep 23, 2021
Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -392,7 +392,7 @@ EOF
if [ "${MACHINE}" == "Mac" ]; then
if [ "${MAC_MAIN_VERSION}" -le 10 ]; then
if [ "${MAC_SUB_VERSION}" -ge 15 ]; then
cargo build --lib --release > "${ANDROID_LOG_PATH}/cargo_${PLATFORMABI}_${LEVEL}.txt" 2>&1
cargo build --lib --release --no-default-features > "${ANDROID_LOG_PATH}/cargo_${PLATFORMABI}_${LEVEL}.txt" 2>&1
else
cargo ndk --target ${PLATFORMABI} --android-platform ${LEVEL} -- build --release > "${ANDROID_LOG_PATH}/cargo_${PLATFORMABI}_${LEVEL}.txt" 2>&1
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we only keeping support for the latest macOS? If not, this will need to be updated as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

didn't realise there were so many logic branches here

@StriderDM
Copy link
Contributor

Looks like it could work, although I think we should do this before merging this in:
mobile_build.sh has to run and produce libraries for both android and iOS.
The wallet projects have to be built and run with these libraries (disable automatic download of dependencies or they will be overwritten) on the device or a simulator configured to use the device architectures we build for.

Cargo test in this case is unfortunately not enough to tell if this will work on the intended devices, the above will.

@StriderDM
Copy link
Contributor

There is also an alternative to this since the purpose of it is purely for tests. Since the libraries are not being static linked in to the wallet_ffi library by building with cargo, having the dependencies installed beforehand and in the path will be sufficient for the tests to run for wallet_ffi.

Just another option.

@aviator-app aviator-app bot removed the mq-failed label Sep 27, 2021
@aviator-app aviator-app bot merged commit dbe023c into tari-project:development Sep 27, 2021
StriderDM added a commit to StriderDM/tari that referenced this pull request Sep 30, 2021
revert bundle sqlite by default for tests (tari-project#3382)
workaround for  linker error for iOS framework
added dynamically linked dependencies to ci.yml for wallet_ffi tests
StriderDM added a commit to StriderDM/tari that referenced this pull request Oct 1, 2021
@stringhandler stringhandler deleted the mb-default-features branch October 4, 2021 06:30
StriderDM added a commit to StriderDM/tari that referenced this pull request Oct 4, 2021
StriderDM added a commit to StriderDM/tari that referenced this pull request Oct 4, 2021
StriderDM added a commit to StriderDM/tari that referenced this pull request Oct 4, 2021
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