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

Add a prefix to the library names and to every global symbol #115

Merged
merged 5 commits into from
Sep 27, 2023

Conversation

egrimley-arm
Copy link
Collaborator

@egrimley-arm egrimley-arm commented Aug 4, 2023

A collision with another crate, even another crate that uses the same C library, should now be very unlikely, so the links line in Cargo.toml is removed. It should also be possible for a project to use multiple versions of this crate.

The libraries are now called libpsa_crypto_0_10_0_shim.a and libpsa_crypto_0_10_0_mbedcrypto.a and there is a new directory for them (.../out/llib) which does not contain any other libraries. All the globally defined symbols have the prefix psa_crypto_0_10_0_.

This relies on bindgen::callbacks::ParseCallbacks::generated_name_override, which was added in bindgen 0.64.0, which requires Rust 1.60.0. Currently we want psa-crypto to work with Rust 1.58.1. We could perhaps make this prefixing optional, enabled by a feature, but I don't think there's any way to make the links field in Cargo.toml conditional on a feature so perhaps we will just have to wait till people are happy to move to rust-version = "1.60.0".

I've done this on top of #116, which tests with two versions of Rust, and I've updated the minimal version from 1.58.1 to 1.60.0 in a separate commit.

@egrimley-arm egrimley-arm self-assigned this Aug 4, 2023
@egrimley-arm egrimley-arm changed the title WIP: Add a prefix to the library names and to every global symbol Add a prefix to the library names and to every global symbol Aug 7, 2023
@egrimley-arm egrimley-arm marked this pull request as ready for review August 7, 2023 09:34
@egrimley-arm egrimley-arm requested a review from ionut-arm August 7, 2023 09:35
Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

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

Amazing solution! Makes me think whether we should do this for other crates.

The code looks good, the one request I have is that you add a short notice about this in the psa-crypto-sys README.

@egrimley-arm
Copy link
Collaborator Author

The code looks good, the one request I have is that you add a short notice about this in the psa-crypto-sys README.

Done. Tell me if you think of any other comments/documentation that might be useful.

Copy link
Member

@ionut-arm ionut-arm left a comment

Choose a reason for hiding this comment

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

Thanks!

@egrimley-arm egrimley-arm marked this pull request as draft August 18, 2023 17:05
@egrimley-arm egrimley-arm force-pushed the pr-symprefix branch 2 times, most recently from 9d8496a to 834689d Compare August 22, 2023 09:54
@gowthamsk-arm
Copy link
Contributor

Is this still in draft state or can I review it? :)

@egrimley-arm
Copy link
Collaborator Author

Is this still in draft state or can I review it? :)

I've left this as "draft" because it requires (and includes as separate commits) updating the minimal Rust version from 1.58.1 to 1.60.0. I think someone said that there's a good reason for the minimal Rust version being 1.58.1, but it would be nice to document here what that reason is so that we can know when that reason has gone away.

But that's the only reason I've left this as "draft".

@tgonzalezorlandoarm
Copy link
Member

Hello @egrimley-arm could you resolve the conflicts? IMO this is ready for final review and merge

@tgonzalezorlandoarm tgonzalezorlandoarm marked this pull request as ready for review September 27, 2023 09:59
To reduce the risk of a name collision with some other crate.

No need for the println stuff: cc handles it for us.

Signed-off-by: Edmund Grimley Evans <[email protected]>
A collision with another crate, even another crate that uses the same
C library, should now be very unlikely, so the "links" line in
Cargo.toml is removed. It should also be possible for a project to use
multiple versions of this crate.

The libraries are now called libpsa_crypto_0_10_0_shim.a and
libpsa_crypto_0_10_0_mbedcrypto.a and there is a new directory for
them (.../out/llib) which does not contain any other libraries. All
the globally defined symbols have the prefix "psa_crypto_0_10_0_".

Signed-off-by: Edmund Grimley Evans <[email protected]>
@egrimley-arm
Copy link
Collaborator Author

It looks like an update to some crate we depend on has made rustc 1.60 no longer work. Perhaps I could make it work with 1.60 by adding some constraint to the Cargo.toml, but as far as I know there's nothing special about 1.60 so perhaps we should just increase the Rust version we test with.

@tgonzalezorlandoarm
Copy link
Member

@egrimley-arm Yes, if possible, could you use 1.66.0 instead? Thanks

Note that these are build-dependencies, which perhaps alleviates
the security implications. But changing our rust-version to
1.63.0 still might be a better plan.

Signed-off-by: Edmund Grimley Evans <[email protected]>
@tgonzalezorlandoarm tgonzalezorlandoarm merged commit 0d79b23 into parallaxsecond:main Sep 27, 2023
@egrimley-arm egrimley-arm deleted the pr-symprefix branch September 28, 2023 13:33
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