Skip to content

Conversation

@csware
Copy link
Contributor

@csware csware commented Aug 12, 2022

Not all libgit2 users use cmake.

Issue introduced in: #6191

Similar fix for features.h: #4346

Maybe a more generic flag should be introduced.

…eader

Signed-off-by: Sven Strickroth <email@cs-ware.de>
@csware csware force-pushed the no_experimental_h branch from 4090d1a to ffb613f Compare August 12, 2022 17:04
@ethomson
Copy link
Member

Definitely we need a solution for this. I think that there's two use cases here:

  1. A piece of software (like TortoiseGit) brings libgit2 in as a dependency into their source tree and uses something other than cmake.
  2. libgit2 is getting installed onto the system (via dpkg / rpm / chocolatey / etc)

What we don't want to do is install headers that let people #define their way into a crash without understanding it. If I were to casually read that header file in git2.h, I would conclude that I can #define LIBGIT2_NO_EXPERIMENTAL_H in my code to turn off experimental functionality. But these are not runtime configurable, if I was built with experimental sha256 support then that's on. There's no turning it off at this stage, and if you #define LIBGIT2_NO_EXPERIMENTAL_H in your code and build it then you're going to crash.

🤔

I wonder if we can ship experimental.h in the source tree, so that users in category #1 don't need to worry about its existence, and then cmake will still generate experimental.h and move that up in the include path so that it's seen first. This feels a bit clunky - if I'm getting started with libgit2, then I might get confused about why experimental.h is ignored. But I'd rather move the pain to libgit2 developers instead of libgit2 users.

I wonder if we can further improve this by getting rid of experimental.h.in and making the in-tree experimental.h the input to cmake generation. 🤔

@csware
Copy link
Contributor Author

csware commented Aug 12, 2022

Maybe the library needs to be named differently when experimental features are enabled to avoid unintended replacements of the binaries that would result in crashes.

@ethomson
Copy link
Member

It is.

@ethomson
Copy link
Member

@csware I implemented what I described above ☝️ - can you try this and let me know if it works for your setup?

@csware csware closed this Sep 19, 2022
@csware csware deleted the no_experimental_h branch September 19, 2022 11:39
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