Add prefix feature #127
Add prefix feature #127
Conversation
The prefix feature adds a psa_crypto_(version) prefix to the library names and all the symbols. By default it is disabled. Signed-off-by: Gowtham Suresh Kumar <[email protected]>
Signed-off-by: Gowtham Suresh Kumar <[email protected]>
|
With the prefix feature enabled, there are some issues related to cross-compiling. This will be fixed in another PR. Having it as a feature and enabling only when needed would be better. |
gowthamsk-arm
requested review from
tgonzalezorlandoarm,
egrimley-arm and
ionut-arm
|
What is the general approach to add features:
I'm more inclined towards option 2 as having lots of features enabled by default might not be convenient. Any thoughts? |
| @@ -35,6 +35,16 @@ | |||
| #![allow(clippy::multiple_crate_versions)] | |||
|
|
|||
| fn main() -> std::io::Result<()> { | |||
| // If the prefix feature is not enabled then set the "CARGO_PKG_LINKS" | |||
| // parameter to mbedcrypto to avoid any duplicate symbols from any other | |||
There was a problem hiding this comment.
Should this be mbedtls instead of mbedcrypto so that we can detect an attempt to combine with mbedtls-sys-auto (https://github.com/fortanix/rust-mbedtls/blob/master/mbedtls-sys/Cargo.toml#L14)?
There was a problem hiding this comment.
I'm worried changing this would affect if there is another crate that links to libmbedcrypto. The user in this case can use the prefix feature to avoid any collisions instead of changing mbedcrypto?
There was a problem hiding this comment.
In fact I've also suggested that mbedtls-sys-auto change its links line: fortanix/rust-mbedtls#289
If both crates changed, then we'd be back to where we started! The links line is a very crude mechanism for detecting collisions.
There was a problem hiding this comment.
Since you have already raised an issue there, I think we can stick to mbedcrypto for now.
I agree with option 2, I would say that the criteria is not to add any features by "surprise", so if you disable the defaults, you have to explicitly enable each feature that you want enabled. This may result in a longer Cargo.toml line, but it avoid the surprise factor of bringing along a crate that you weren't expecting to bring |
|
I agree with option 2 as well - as long as we've not had the renaming as a default in the previous release (?). Having more features to enable isn't a problem, especially if they can have drastic effects on how the crate works for dependents. |
|
Initially, there was no prefixing, but then |
|
Provided |
|
Hmm, ok, I guess as long as it's documented somewhere, this should be ok. EDIT: We can even yank v0.11.0 to prevent these jarring changes, perhaps some would have issues with the library changing underneath them. |
|
Makes sense. Will do that:) |
The prefix feature adds a psa_crypto_(version) prefix to the library names and all the symbols. By default it is disabled.
Signed-off-by: Gowtham Suresh Kumar [email protected]