Cleanup header includes

[ilammy]

The main issue addressed by this PR are circular dependencies in header files. Many of them include umrella headers (<soter/soter.h>, <themis/themis.h>) which creates a circular dependency in other headers included from umrella headers. This prevents individual headers from compiled and analyzed separate by the tools. It also introduces hidden dependency on the inclusion order. To resolve issue avoid using umbrella headers in library code: include specific headers where necessary. Umbrella headers should be used only for the user code (and tests).

While we're here, do some cosmetic improvement as well:

• unify include guard style to be <PROJECT>_<HEADER>_H everywhere
• sort included headers alphabetically
• group headers in the meaningful groups
• use proper relative paths in headers
• consistently use quotes "soter/soter_rsa_key.h" for internal inclusions
• consistently use angle brackets <soter/soter.h> for public inclusions

These changes are inspired and rationalized by Google C++ Style Guide.

[Lagovas]

why you include soter/openssl/boringssl/soter_engine.h in several variants: as <path> and "path"?

[Lagovas]

why we use angle braces here? should boringssl/soter_engine.h be public?

[ilammy]

Mmm... I think you're right, it's not public. I initially assumed all headers to be public and thus use only angled brackets there (and quotes in *.c files). I'll review the headers and will tweak the quotes.

Now when I think of it... It's kinda hard to distinguish public headers from private ones. I've seen quite a few libraries put their public headers into a separate top-level directory include. For example:

include
  themis
    secure_cell.h
    themis.h
    ...
src
  themis
    message.h
    portable_endian.h
    secure_session_utils.h
    ...

How do you feel about moving to this layout in the next PR?

[ilammy]

Based on the include analysis, here are the public headers. I.e., this gets included with #include <themis/themis.h>:

soter/soter.h
soter/soter_asym_cipher.h
soter/soter_asym_ka.h
soter/soter_asym_sign.h
soter/soter_error.h
soter/soter_hash.h
soter/soter_hmac.h
soter/soter_kdf.h
soter/soter_rand.h
soter/soter_rsa_key.h
soter/soter_rsa_key_pair_gen.h
soter/soter_sym.h

themis/themis.h
themis/themis_error.h
themis/secure_cell.h
themis/secure_comparator.h
themis/secure_keygen.h
themis/secure_message.h
themis/secure_session.h

And these ones are private:

soter/boringssl/*
soter/ed25519/*
soter/openssl/*
soter/soter_container.h
soter/soter_crc32.h
soter/soter_ec_key.h
soter/soter_sign_ecdsa.h
soter/soter_sign_rsa.h
soter/soter_t.h

themis/message.h
themis/portable_endian.h
themis/secure_cell_alg.h
themis/secure_comparator_t.h
themis/secure_message_wrapper.h
themis/secure_session_peer.h
themis/secure_session_t.h
themis/secure_session_utils.h
themis/sym_enc_message.h

[vixentael]

1. Please do not change header structure! We spent a lot of time to make sure that some wrappers (like iOS one) export correct header structure (especially in ObjC++). Moreover I don’t see any real benefits from changing folders layout except “for the sake of style” one — we don’t expect many users to use core library as is, so beauty won’t be noticed, but compatibility problems might arise. 2. Please do not merge this PR until I confirm it’s working for iOS — @ilammy ping me when you’re done, I’ll fetch branch to check.

[ilammy]

@vixentael, understood. Thank you for the warning.

[ilammy]

@vixentael you can try checking the iOS wrapper now.

I've managed to see the tests pass locally and it seems to have used the correct source code for that, but I'm not really sure. Please check.

Now that I've interacted with iOS wrapper a bit I completely agree with you to not touch that fine-tuned header magic unless absolutely necessary. Though, it seems that we export too many headers for iOS, but I'm no expert, maybe we really need to do that because of reasons.

[vixentael]

@ilammy i've checked iOS wrapper – it's working fine.
If @Lagovas doesn't have any more comments – let's merge!

The reasoning behind my warning (lots of details inside)

It's not a magic of any kind, it's more a fine-tuned Header Search Paths.

In podspec we precisely define what Themis core files and iOS wrapper files should be exported, what path should be preserved and what are public headers:

# don't use as independent target
so.subspec 'core' do |ss|
    ss.source_files = "src/themis/*.{h,c}", "src/soter/*.{c,h}", "src/soter/ed25519/*.{c,h}", "src/soter/openssl/*.{c,h}"
    ss.header_mappings_dir = "src"
    ss.header_dir = 'src'
    ss.preserve_paths = "src/themis/*.h", "src/soter/*.h", "src/soter/ed25519/*.h", "src/soter/openssl/*.h"
    ss.public_header_files = "src/themis/*.h", "src/soter/*.h", "src/soter/ed25519/*.h", "src/soter/openssl/*.h"
end

# don't use as independent target
so.subspec 'objcwrapper' do |ss|
    ss.header_mappings_dir = 'src/wrappers/themis/Obj-C/objcthemis'
    ss.source_files = "src/wrappers/themis/Obj-C/objcthemis/*.{m,h}"
    ss.public_header_files = 'src/wrappers/themis/Obj-C/objcthemis/*.h'
    ss.header_dir = 'objcthemis'
    ss.dependency 'themis/themis-openssl/core'
end

This is how these headers in Themis pod.xcconfig looks like:

HEADER_SEARCH_PATHS = $(inherited) "${PODS_ROOT}/Headers/Private" "${PODS_ROOT}/Headers/Private/themis" "${PODS_ROOT}/Headers/Public" "${PODS_ROOT}/Headers/Public/GRKOpenSSLFramework" "${PODS_ROOT}/Headers/Public/themis" "${PODS_ROOT}/themis/src" "${PODS_ROOT}/themis/src/wrappers/themis/Obj-C"

This is how they are transformed (notice a "flat" structure, without any folders):

[Lagovas]

why here we use relative to file's folder path instead soter/soter_sign_ecdsa.h ? it's third variant of imports in the code. do we have any reason for that?

[ilammy]

do we have any reason for that?

No, that's just me not having enough attention. Thanks for noticing this, I'll review the includes once again. All paths should start from the 'project root'.