fix: allow boring2 and boring in 1 project

[JakubKoralewski]

haven't actually run it but at least cargo check seems ok. wdyt?

[gemini-code-assist]

Summary of Changes

Hello @JakubKoralewski, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses potential symbol conflicts that arise when both the boring and boring2 crates are used within the same Rust project. By introducing a unique b2_ prefix to all generated FFI symbols and updating the Cargo.toml linking metadata, the changes ensure that both crates can coexist without name clashes, improving compatibility and project flexibility.

Highlights

• Symbol Prefixing: Modified the boring-sys/build/prefix.rs file to add an additional b2_ prefix to all generated FFI symbols. This ensures unique symbol names when boring2 is used alongside other BoringSSL bindings.
• Cargo Metadata Update: Updated the links metadata in boring-sys/Cargo.toml from boringssl to b2_boringssl to align with the new symbol prefixing strategy and prevent linking conflicts.

Changelog

• boring-sys/Cargo.toml
  • Updated the links field from "boringssl" to "b2_boringssl".
• boring-sys/build/prefix.rs
  • Modified the PrefixCallback implementation to prepend "b2_" to the existing symbol prefix.

Activity

• The author has performed a cargo check to verify basic compilation.
• The author is seeking feedback from reviewers regarding the proposed changes.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request aims to allow boring-sys2 to coexist with other BoringSSL-backed crates by renaming the linked native library and prefixing its symbols. The change to Cargo.toml is correct for this purpose. However, the implementation of symbol prefixing in the build script is incomplete. While the bindgen FFI bindings are updated to use a new prefix, the corresponding logic that renames symbols in the compiled library is not, which will cause the build to fail with linker errors. I've added a critical comment with details on how to resolve this.

[gemini-code-assist]

This change correctly adds the b2_ prefix for the symbols in the bindgen-generated FFI bindings. However, this is incomplete and will lead to linker errors.

The prefix_symbols function in this same file is responsible for renaming the symbols in the compiled native library using objcopy. It has not been updated to use the same b2_ prefix. As a result, the Rust code will try to link against symbols like b2_boring-sys2_... which will not exist in the library (they will be named boring-sys2_...).

To fix this, you need to update the symbol renaming logic in prefix_symbols to be consistent. Please change line 64 to:

.map(|l| format!("{l} b2_{PREFIX}_{l}"))

[0x676e67]

This won't work. You must enable the "prefix_symbols" feature to avoid using the same symbol.

[JakubKoralewski]

HI @0x676e67, after some changes and headbanging against cmake it seems to work now. Here's an example project with how I checked the libraries built by boring-sys and boring-sys2 crates don't conflict: https://github.com/JakubKoralewski/boring-boring2 (see README.md and symbols dir for nm -a outputs)

I'd encourage you to check again, maybe? I pushed new commits to this branch, and it's those that I'd like you to check if possible.

This won't work. You must enable the "prefix_symbols" feature to avoid using the same symbol.

The parts I changed are for only when prefix-symbols feature is enabled. Just prefix-symbols did not work as the links = "boringssl" causes cargo to reject the build given two crates that export the same lib.a. The links change itself without prefix-symbols was enough (for my hello world example) but probably would lead to silent errors. With prefix-symbols enabled and no extra changes I had errors because C++ symbols where not prefixed, whereas BORINGSSL_PREFIX uses C++ namespaces to prefix those too.

[0x676e67]

To be honest, I’m not very familiar with this area and I don’t want to break anything right now. This solution looks quite messy.

cc @BarbossHack What do you think? This probably needs your review. I honestly don't even think it's necessary to support boring—we've already wasted so much time on OpenSSL compatibility.

[JakubKoralewski]

It's up to you, but for me this solution works and the current doesn't.

Also using something provided by boring (BORINGSSL_PREFIX) rather than a hand rolled version seems less messy, not more. Maybe my implementation of it is messy.

If you don't want to support multiple boring versions then I suggest removing the documentation around the prefixing feature that suggests it's possible to have multiple versions of boringssl at a time.

Also I'm not sure in prefix.rs the current env! prefix of CRATE_NAME works, as for me when enabled it I just saw a generic name of the build crate.

[0x676e67]

prefix-symbols was implemented by @BarbossHack, so I’m not intimately familiar with that part of the logic. I’ll need to check with him first. As long as it stays compatible with our previous implementation and doesn't introduce any regressions, I'm fine with it. My only concern is that I’d like the implementation to be as clean as possible to avoid major conflicts with upstream.

[BarbossHack]

Actually, none of your fixes seems to works... ?

-> I tried building a project with the fixes from this MR (with tokio-quiche like you did), but there are still many duplicated symbols. (Also, I only see two changes on this MR, is that a mistake?)

-> I also tried building your debug project: https://github.com/JakubKoralewski/boring-boring2 but there are still some duplicated symbols. I reviewed your prefix changes in the debug project and noticed that you are using BoringSSL’s read_symbols.go tool (BORINGSSL_PREFIX) (but you forgot to apply the prefixing by calling make_prefix_headers.go and rebuilding libcrypto/libssl). Please refer to my original merge request to understand why this may be a bad idea (this won't be sufficient) : #103 .

In this MR, you made two changes:

Renaming links in Cargo.toml

This would indeed be required in order to link two BoringSSL static libraries, but it is not sufficient.

Renaming the prefix to b2_{PREFIX}...

In this case, it is unnecessary. boring-sys (the one used by tokio-quiche) does not currently perform any symbol prefixing (as shown here: cloudflare/boring#401 , my MR is still open). Therefore, there is no need to introduce an additional symbol prefix; {PREFIX} already exists.

---

That said, using env!("CARGO_CRATE_NAME") was not ideal, since it is resolved as "build_script_main" at build time. As a result, the prefix used by boring-sys2 becomes "build_script_main_...". It is not very elegant, but it's sufficient (since neither openssl-sys nor boring-sys use this specific prefix). For clarity, we could replace it with: const PREFIX: &str = "boringssl2";

---

I’m not sure how to fix this MR properly, but if you manage to make it work on your side, and if it does not break prefixing compatibility with OpenSSL (please run the GitHub Actions on your fork to verify this), I would be happy to review it again 👍

[JakubKoralewski]

Also, I only see two changes on this MR, is that a mistake

This PR was closed early so it doesn't contain all the changes.

you forgot to apply the prefixing by calling make_prefix_headers.go

The boringssl/src/CMakeLists.txt runs it if it detects both BORINGSSL_PREFIX and BORINGSSL_PREFIX_SYMBOLS.

and rebuilding libcrypto/libssl

I saved the symbols-out.txt in the repo so there would be no need for double compilation, at expense of having to manually keep the file up to date. (I wrongly assumed, I realize now, a single file could cover all architectures etc.)

I'm not sure why boring-boring2 doesn't work for you. I was running on Linux aarch64. It's probably some environment differences.

But I see that you had good reasons not to require Go when building this Rust crate, makes sense. I can see that supporting all the combinations of OSs and architectures is a level harder problem than what I expected to do here. So I'm just gonna give up trying to merge this and be happy that I got it working on my side (which I wasn't sure I was going to :D) requiring Go for everyone who wants to build this Rust crate is not a good idea. (although I guess it's behind an optional feature...)

Anyway, thanks.