build: Revert to using @_implementationOnly for all BoringSSL modules

simonjbeaumont commented 4 days ago

Motivation

The 3.5.0 release has been causing some compilation failures for downstream projects that depend on other packages that also vendor in a copy of BoringSSL.

This is likely because we stopped using @_implementationOnly when importing our internal CCryptoBoringSSL target, which we did because we moved the ArbitraryPrecisionInteger and FiniteFieldArithmeticContext types, that were previously internal to Crypto, into CryptoBoringWrapper so they could be used in _CryptoExtras to implement RSABSSA.

While Swift Crypto goes to some lengths to avoid clashing with other vendor'd copies of BoringSSL by prefixing its symbols, it clearly isn't enough and we've heard reports of downstream projects failing to build.

We have shipped one patch release, 3.5.1, with a fix to the vendor script that has resolved a subset of adopters, it hasn't resolved everything.

At this stage, I'd like to propose that we mitigate the impact of the issue and give us time to revisit how we vendor and import the C module, rather than us chasing a long tail of reported issues downstream.

Modifications

Moved ArbitraryPrecisionInteger and FiniteFieldArithmeticContext back into Crypto as internal types, but bringing along the additional operations that were added as part of 3.5.0.
Revert to using @_implementationOnly imports of CCryptoBoringSSL and CCryptoBoringSSLShims everywhere.
Share these two files with _CryptoExtras by symlink.
Update CMakeLists.txt to account for new location of files in Crypto.
Update CMakeLists.txt to account for new symlinks in _CryptoExtras.
Update CMake script to accommodate use of symlinks.

Result

Greater confidence that we won't have broken downstream projects due to BoringSSL symbol clashes.

simonjbeaumont commented 4 days ago

One example of a project that currently does not build with 3.5.1 https://github.com/vapor/penny-bot. Here is the dependabot CI run that failed for 3.5.1: https://github.com/vapor/penny-bot/actions/runs/9767425786/job/26962669715?pr=211

I have confirmed I can reproduce that failure locally and that this patch resolves the issue.

simonjbeaumont commented 4 days ago

//cc @MahdiBM

Lukasa commented 4 days ago

This is very troubling. We did a fair bit of analysis that tried to identify a difference between the @_implementationOnly and non-@_implementationOnly use-cases, and couldn't identify any meaningful difference.

Can we identify one now?

apple / swift-crypto