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

Specify library version as part of the headers #74

Merged
merged 1 commit into from
Mar 19, 2024
Merged

Specify library version as part of the headers #74

merged 1 commit into from
Mar 19, 2024

Conversation

wjakob
Copy link
Contributor

@wjakob wjakob commented Mar 11, 2024

Dear @Tessil,

would you consider specifying the robin-map version as part of the header files?

I am the author of nanobind, a Python <-> C++ binding layer that uses robin-map in various internal data structures. Robin-map has been great, I am really happy with both design and performance.

nanobind pins a specific version of robin-map to ensure ABI compatibility. That way, multiple separately compiled C++ extensions can pool their internal data structures to exchange information about types and objects.

But one issue now that nanobind is being used by more people and different distributions is that they really don't like having a pinned dependency. They want to use the latest one available on the OS or package manager. That introduces the possibility of ABI contract violations.

Having an easily queriable #define as part of the robin map headers would make it easier to catch such future changes (perhaps unlikely, as this seems like a relatively stable project, but one can never be sure)

@Tessil
Copy link
Owner

Tessil commented Mar 17, 2024

Hi @wjakob

Thank you for your contribution, I'm glad the library is useful to nanobind.

The PR looks like a good addition. Just one minor comment, I would prefer to use the TSL_RH_ prefix instead of just TSL_ to avoid any potential conflict with other libraries in the tsl space (I might also add the version info in them).

@wjakob
Copy link
Contributor Author

wjakob commented Mar 17, 2024

Certainly, let's use whatever prefix you like. I will amend the commit. Since creating the PR, I realized that simply adding version numbers isn't the whole story. For them to be useful, the digits need to have an assigned meaning, and that should probably also be part of this.

In SemVer, the patch release part of the version number indicates minor bugfixes. Minor number increments communicate added features, and major number increments communicate API breaks.

Since this is a C++ data structure, there are two separate concerns: ABI breaks and API breaks. For simplicity, my suggestion would be to combine those two and have the following convention:

// A change of the major version indicates an API and/or ABI break (change of in-memory layout of the data structure)
#define TSL_RH_VERSION_MAJOR 1
// A change of the minor version indicates the addition of a feature without impact on the API/ABI
#define TSL_RH_VERSION_MINOR 2
// A change of the patch version indicates a bugfix without additional functionality
#define TSL_RH_VERSION_PATCH 2

For example, this way, I could static_assert on TSL_RH_VERSION_MAJOR in my project, and that will prevent compilation of libraries with an incompatible ABI.

Can you let me know if this sounds good to you?

@Tessil
Copy link
Owner

Tessil commented Mar 18, 2024

Yes, this sounds reasonable.

The library is mainly in maintenance mode (bug fix & others) as I consider it features complete in general, so I don't expect any big change in the future and if I do, I would create a 2.0 version.

@wjakob
Copy link
Contributor Author

wjakob commented Mar 18, 2024

Great — I amended the commit.

@Tessil Tessil merged commit 918f664 into Tessil:master Mar 19, 2024
11 checks passed
@Tessil
Copy link
Owner

Tessil commented Mar 19, 2024

Thanks, I merged the change and will check to create a new 1.2.2 release (updating the different versions values).

@wjakob
Copy link
Contributor Author

wjakob commented Mar 19, 2024

Thank you!

@Tessil
Copy link
Owner

Tessil commented Mar 19, 2024

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