Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allowing for dynamic top-level namespacing #359

Merged
merged 3 commits into from
Aug 31, 2023
Merged

Allowing for dynamic top-level namespacing #359

merged 3 commits into from
Aug 31, 2023

Conversation

glhewett
Copy link
Contributor

@glhewett glhewett commented Aug 25, 2023

Taken from the original mls update. (thanks @suhasHere)

  • mls namespace has been changed to MLS_NAMESPACE
  • tls namespace has been changed to MLS_NAMESPACE::tls
  • hpke namespace has been changed to MLS_NAMESPACE::hpke
  • bytes_ns namespaces has been changed to MLS_NAMESPACE::bytes_ns
  • namespace.h is produced from template to #define MLS_NAMESPACE to configured namespace value. The result is stored in the ${PROJECT_BINARY_DIR}/include and build time includes have been updated to look there.
  • 2 CMAKE variables have been added that will change top cpp namespace and one that will change the exported project namespace.

Example Usage

# defaults will be mls + MLSPP
$ cmake -B build -DCMAKE_BUILD_TYPE=Debug

# changing the namespaces
$ cmake -B build -DCMAKE_BUILD_TYPE=Debug -DMLS_EXPORT_NAMESPACE=MLSPPv4 -DMLS_CPP_NAMESPACE=mlsv4

Other changes of note:

  1. Upgraded the clang-format version to use 16 (I found an inconsistency between 16 and 13, and this was quickest way to move forward and not cause issue for other developers)
  2. Upgraded the clang-format action to 4.11.0

@glhewett glhewett changed the title allowing for dynamic top-level namespacing. allowing for dynamic top-level namespacing Aug 25, 2023
@bifurcation bifurcation changed the title allowing for dynamic top-level namespacing Allowing for dynamic top-level namespacing Aug 27, 2023
@bifurcation
Copy link
Contributor

To add a little more context here, since this is a breaking change: The main thrust of this PR is to add a per-version top-level namespace, which contains all other MLSpp namespaces. This allows multiple versions of the library to be linked at the same time (e.g., for backward compatibility) without symbol conflicts.

(This is effectively what Rust/cargo does, but more manually.)

Copy link
Contributor

@bifurcation bifurcation left a comment

Choose a reason for hiding this comment

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

This looks OK to me, modulo the one CI failure.

My main question is whether the version-based namespacing is a little too fixed and too granular. If we were willing to assume that downstream consumers were building from source (as opposed to consuming a pre-built binary), then we could just make the namespace / version suffix a build-time option, so that the consumer could set it to whatever they want. Maybe I'm over-thinking this, though.

* changing namespace from mls to MLS_NAMESPACE
* changing namespace hpke with MLS_NAMESPACE::hpke
* changing bytes_ns with MLS_NAMESPACE::bytes_ns
* changing tls with MLS_NAMESPACE::tls
@bifurcation bifurcation merged commit 0e6ce41 into main Aug 31, 2023
13 checks passed
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.

2 participants