Move public Themis headers to "include" directory #759
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Recent release of bindgen 0.56.0 resulted in a change in the order of the derived traits in the generated code. The changelog does not say anything about that, but I doubt this is considered a notable change. However, it breaks our build system which meticulously checks for any changes in the generated code compared to checked in version. It does this so that we don't miss any changes. Well, the CI is going to be using bindgen 0.56.0 from now on, let's use the new trait order as well, whatever it is.
As the subject line says, let's move all public Themis and Soter headers into the new top-level directory "include". This is a common structure for C projects to keep public headers separate. That way public API is much more cleanly delineated. Currently, Themis technically exports some private headers with private API which might change in the future. With this new structure it will be more obvious which files you can change without risking to introduce API or ABI incompatibility.
Update Themis Core, AndroidThemis, and Carthage build files to include the new "include" directory into the header search path. Since its structure is the same as the previous "src" directory, no other changes are necessary for compiler to be able to resolve "<themis/themis.h>".
Since CocoaPods is special needs package manager, a separate commit takes care of it. First, just like with other build systems, add the new "include" directory into header search paths. Instruct CocoaPods to tell Xcode. Then add new headers to the "source_files" list so that CocoaPods knows that there are some new project headers and does not delete them (because it deletes everything not explicitly mentioned there). Also add the new headers to the "private_header_files" so that they won't get treated as public headers by default. We don't need to export THemis Core headers in ObjCThemis, but we do need them in the project. With this directory split we don't really need the "header_dir" setting, but we very much do need the "header_mappings_dir" so that CocoaPods does not flatten the header structure. It's vitally important for include path resolution since it's <soter/foo.h> and <themis/bar.h> everywhere in the source code. Note that "header_mappings_dir" must be a common directory of all headers. Well, with the "include" directory being on the top-level, the only common directory we have left is the repository root. So use it. Repeat this for all three subspecs that we have. Reformat as necessary. Don't forget that BoringSSL uses its own source set.
Update the installation rules to use public headers from "include", not the ones from "src". However, include both in the header lists used by code formatting and static analysis rules.
EC keygen interface does not export EC key structure. RSA code should do the same. The header structure for EC and RSA is a bit different due to historical reasons, but both implementation details should be hidden. The <soter/soter_rsa_key.h> header is a private one, it should not be used in public headers. Remove it from <soter/soter_rsa_key_pair_gen.h> and then manually add it in places which expected this header to be transitively included.
The TODO says to "probably move this to private headers" and this is exactly what we're doing here. All of these definitions are implementation details of Secure Session and do not belong to the public Themis API. Move them to appropriate internal headers. Also, drop all the swathes of commented unused code from there.
We need to tell bindgen about new header location in "include". After that, regenerate FFI bindings to clean up all the private API which got finally hidden from public headers.
Pull in this bingden update so that the build gets green.
ilammy
added
core
ilammy
requested review from
vixentael,
ignatk,
Lagovas,
julepka,
shadinua and
iamnotacake
|
Getting you all comfortable with me dropping PRs on weekends. Soon I'll probably do so only on weekends, heh... Anyway... This change might sabotage @julepka's work on SPM since it will need to be told about the additional header location. Tread carefully here. |
vixentael
reviewed
Nov 28, 2020
| ss.header_dir = "src" | ||
| ss.header_mappings_dir = "src" | ||
| # Prevent CocoaPods from flattening the headers, we need structure. | ||
| ss.header_mappings_dir = "." |
|
Have you tried publishing a temp podspec with these changes, and then building our test project?
Sure, did that too. My testing capabilities are limited but I believe if simulator builds are working, then it should be okay.
|
julepka
approved these changes
Nov 29, 2020
|
This PR should make my struggle with SPM easier 🙂 |
|
I've checked Apple FairyLand, including backwards compatibility issues. All good. I've checked the following settings:
for following cases: |
vixentael
approved these changes
Nov 29, 2020
|
Merge this baby! 🌟 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As the subject line says, let's move all public Themis and Soter headers into the new top-level directory
include. This is a common structure for C projects to keep public headers separate. That way public API is much more cleanly delineated.Rationale
Currently, Themis technically exports some private headers with private API which might change in the future. With this new structure it will be more obvious which files you can change without risking to introduce API or ABI incompatibility.
Change details
It's not a simple
git mv, more that one place needs a fix up for this all to work. The details are described in commit messages, I won't repeat them here.Previously, @vixentael has been very afraid of this change breaking something in Apple Fairy Land, but now that we have proper CI, I am more than sure that it works. It definitely works on my machine, heh (as if that's worth anything).
On the breaking changes
Technically, this is a breaking change so I'm giving it a proper label. However, the users were not expected to use these now-removed APIs, include our private headers, etc. Users of our high-level wrappers should not be affected by this. The only people who may be affected are authors of third-party wrappers who (for some weird reason) depended on the unintentionally exported private APIs.
If you are one of those people reading this, please accept my condolences. You can contact Themis maintainers for advice if your build has been broken.
The changelog includes a list of removed headers and extra symbols. Of course no one will read it before an upgrade, but it's a good reference for troubleshooting.
Checklist
Example projects and code samples are up-to-date(they don't use private API)