fix: iOS linker error workaround #3401
Conversation
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
There was a problem hiding this comment.
So I think that rather than jsut blanket reverting the changes to making bundling of sqlite and openssl the default case we should try and consider what mikethetike was trying to achieve here.
By default crates using tari_comms and tari_wallet want openssl and sqlite to be bundled. the tari_wallet_ffi is the exception that doesn't want it. So we could be introducing features flags into the crates that tari_wallet_ffi depends on so that only tari_wallet_ffi can exclude the bundling. The complication is that from tari_wallet_ffi's perspective tari_wallet and tari_core use tari_comms and tari_core is a indirect dependency via tari_wallet. So that means you need to add non-default flags to those two crates to make sure their dependencies don't use the bundle features. So you need a feature in tari_wallet that excludes the default bundled features and that feature is passed on to its dependency tari_core to also exclude the bundling in its inclusion of tari_comms
| @@ -55,6 +55,6 @@ env_logger = "0.7.1" | |||
| prost = "0.8.0" | |||
|
|
|||
| [features] | |||
| default=["bundled_sqlite"] | |||
| c_integration = [] | |||
There was a problem hiding this comment.
So instead of just reverting these changes lets try and clean up the feature situation. As far as I can tell this c_integration flag doesn't do anything anymore so lets remove it.
Having Sqlite bundled by default seems correct to me so I would say leave this in and then we make sure that when the wallet_ffi build happens it doesn't use these features.
| @@ -7,13 +7,13 @@ version = "0.18.6" | |||
| edition = "2018" | |||
|
|
|||
| [dependencies] | |||
| tari_comms = { version = "^0.10", path = "../../comms" } | |||
| tari_comms = { version = "^0.10", path = "../../comms", features = ["c_integration"]} | |||
There was a problem hiding this comment.
Does this feature even get used in the tari_comms crate anymore?
| tari_comms_dht = { version = "^0.10", path = "../../comms/dht", default-features = false } | ||
| tari_common_types = {path="../common_types"} | ||
| tari_crypto = { git = "https://github.com/tari-project/tari-crypto.git", branch = "main" } | ||
| tari_key_manager = { version = "^0.10", path = "../key_manager" } | ||
| tari_p2p = { version = "^0.10", path = "../p2p" } | ||
| tari_wallet = { version = "^0.10", path = "../wallet", default-features = false } | ||
| tari_wallet = { version = "^0.10", path = "../wallet", features = ["c_integration"]} |
There was a problem hiding this comment.
Pretty sure that the c_integration flag does nothing in the wallet lib, the default-features = false should mean that this crate does not use the bundled sql lib when building wallet. Not using the bundled Sqlite should be an exception to the default.
| @@ -65,6 +65,6 @@ tempfile = "3.1.0" | |||
| tari_common = { version = "^0.10", path = "../common", features = ["build"] } | |||
|
|
|||
| [features] | |||
| bundled_openssl = ["openssl-sys/vendored"] | |||
| c_integration = [] | |||
There was a problem hiding this comment.
now is this correct? All the other clients of tari_comms would like to bundle openssl but we are removing that option because just the FFI crate doesn't like it. The FFI crate should just not use this feature.
The problem of course is that Core and the Wallet crates use Comms and they both by default use the bundled features in comms. So we should make a feature in Core and Wallet that excludes this feature from Comms. That way Wallet_FFI can choose to use non-default features to exclude this bundling rather than making the default to exclude it.
Description
revert bundle sqlite by default for tests (#3382)
workaround for linker error for iOS framework
added dynamically linked dependencies to ci.yml for wallet_ffi tests
Motivation and Context
Resolves this linker error on iOS:
How Has This Been Tested?
Manually.