Skip to content

fix {Open,Boring}SSL conflicts - #376

Merged
nbsp merged 4 commits into
mainfrom
nbsp/worktree/bssl
Aug 14, 2024
Merged

nbsp merged 4 commits into
mainfrom
nbsp/worktree/bssl

Conversation

@nbsp

@nbsp nbsp commented Jul 17, 2024

Copy link
Copy Markdown
Contributor

uses objcopy to rename all exported BoringSSL symbols, prepending LK_ to them, so that they don't conflict with native OpenSSL on the Rust side.

previous PR description before figuring this out below.


PR for review.
trying a different approach than manually patching in the list (though that's still available & commented out), since even when it finally built (90 minute patch time!) it fixed nothing.

the BoringSSL build doc gives us an option to prefix all BSSL symbols with whatever we want, in our case LK. i've punched that into the BUILD.gn file, and it builds just fine in this diff, but the basic_room linking fails with a lot of the following error

/usr/bin/ld: openssl_adapter.cc:(.text+0x7f): undefined reference to `LK_i2d_X509' # and similar symbols

even after i've manually gone in and updated the files to use LK_-prefixed symbols, they just don't link. fresh building environment. i don't know exactly how it would compile by itself (clearly no undefined symbols) and then fail at the basic_room link phase.

@nbsp
nbsp requested a review from theomonnom July 17, 2024 01:30
@nbsp
nbsp force-pushed the nbsp/worktree/bssl branch from c51cca8 to 76a778b Compare July 17, 2024 02:19
@nbsp
nbsp force-pushed the nbsp/worktree/bssl branch from 81c93a5 to b60a9d5 Compare August 14, 2024 04:45
@nbsp
nbsp marked this pull request as ready for review August 14, 2024 04:47
@nbsp
nbsp requested review from davidzhao and kuba-- August 14, 2024 04:47
@nbsp nbsp changed the title draft: attempt to fix {Open,Boring}SSL conflicts fix {Open,Boring}SSL conflicts Aug 14, 2024
@nbsp nbsp linked an issue Aug 14, 2024 that may be closed by this pull request

@davidzhao davidzhao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AWESOME WORK! 🚀 🚀 🚀

fi

cd src
git fetch origin m114_release

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess this branch has not updated to m125 yet? it would be upgrade too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

not yet. currently we use m114, but because m125 was released we now need to manually checkout. the fix_m114 patch deals with a few bugs that were fixed in the new version. i think upgrading is better suited for a future PR, let's get this tagged first

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sounds good. agreed it should be in a separate PR

@kuba--

kuba-- commented Aug 14, 2024

Copy link
Copy Markdown
Contributor

Definitivelly awesome work!!!
And looks like it all works (also if I add extra isahc which uses curl+openssl).

@nbsp
nbsp merged commit 00c1ca1 into main Aug 14, 2024
@nbsp
nbsp deleted the nbsp/worktree/bssl branch August 14, 2024 09:14
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.

libwebrtc-sys conflicting with openssl-sys

3 participants