Deprecation: export the deprecated functions properly

[ethomson]

Although the error functions were deprecated, we did not properly mark them as deprecated. We need to include the deprecated.h file in order to ensure that the functions get their export attributes.

Similarly, do not define GIT_DEPRECATE_HARD within the library, or those functions will also not get their export attributes. Define that only on the tests and examples.

Add a CMake option to enable hard deprecation; the resultant library will not include any deprecated functions. This may be useful for internal CI builds that create libraries that are not shared with end-users to ensure that we do not use deprecated bits internally.

Fixes #4978

Finally, enable hard deprecation in our builds to ensure that we do not call deprecated functions internally.

[b4n]

This is good, but not enough to fix git_buf_free(): you need to also include deprecated.h in buffer.h.
Ideally every single entry from deprecated.h should be checked, which should not prove that difficult (adapt paths as needed):

$ sed -n 's/^.*GIT_EXTERN([^)]*) \([a-zA-Z0-9_]*\).*$/\1/p' include/git2/deprecated.h 
git_buf_free
giterr_last
giterr_clear
giterr_set_str
giterr_set_oom

From what I see, only giterr_* and git_buf_free need special attention, and so once you added the include in buffer.h we should be all set.

Also to check it's indeed properly included in the resulting library (amend paths as needed)

$ sed -n 's/^.*GIT_EXTERN([^)]*) \([a-zA-Z0-9_]*\).*$/\1/p' include/git2/deprecated.h | while read f; do objdump -T /usr/lib/libgit2.so | grep -qF $f || echo "$f is missing" >&2; done

[b4n]

Adding this WFM indeed:

diff --git a/src/buffer.c b/src/buffer.c
index 51fb48a45..61bd30471 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -10,6 +10,8 @@
 #include "buf_text.h"
 #include <ctype.h>
 
+#include <git2/deprecated.h>
+
 /* Used as default value for git_buf->ptr so that people can always
  * assume ptr is non-NULL and zero terminated even for new git_bufs.
  */

[ethomson]

Thanks for the catch @b4n. I've moved the inclusion of deprecated.h into common.h, which all source files should include to ensure that we don't have to include deprecated.h piecemeal in a way that we could forget (like I did earlier).

I've also added some tests to ensure that we get the declarations for the deprecated functions (via common.h) when GIT_DEPRECATE_HARD is unset. Although this isn't bulletproof, this should help ensure that we handle deprecation rather responsibly. We'll get warnings about declarations if we use these deprecated functions without properly declaring them:

/Users/ethomson/libgit2/tests/core/deprecate.c:31:2: warning:
      implicit declaration of function 'giterr_set_oom'
      [-Wimplicit-function-declaration]
        giterr_set_oom();
        ^

(These warnings will be errors in the CI since we build with -Werror there.)

Obviously there's still possibilities here for a developer to introduce a new deprecated function that does not end up in this test case but this is a good start. Long term we could add another nightly build that produces a normally deprecated version of the library and ensures that the symbols are actually present and properly exported, but I think that this is a good step for now.

[ethomson]

Ha ha, thanks MSVC and its precompiled headers. 😡

[pks-t]

Makes me wish for a developer profile that automatically enables such things.

[ethomson]

Yeah, I see where you're going. Also -Werror and friends. I want to make this as surgical a fix as I can for a very prompt 0.28.1 to unbreak people, but I think that this is probably a good idea.

[pks-t]

I'm not quite happy with the fact that we're now building the library with deprecated functionality enabled by default. It makes it much easier to accidentally use the old functions and only notice as soon as CI fails, which is a needless waste of time.

At the least, it makes me wish for a developer profile that automatically enables such options. Alternatively, one might thing about having a macro GIT_EXTERN_DEPRECATED, that is either

#ifdef GIT_DEPRECATE_HARD
#define GIT_EXTERN_DEPRECATED(x) extern GIT_DEPRECATED(x)
#else 
#define GIT_EXTERN_DEPRECATED(x) extern x
#endif

Like this, we always have the function declared as extern but only produce warnings/errors in case where GIT_DEPRECATE_HARD is defined.

[ethomson]

OK, I'm dropping the test. I thought it would work out well enough, but precompiled headers on MSVC are proving challenging. (Since GIT_DEPRECATE_HARD is defined, it can be brought in the precompiled header, which is later implicitly included to speed up compilation.) Thus a test trying to #undef it won't work? And I can't pass /Y- to a single file in cmake, even with SET_SOURCE_FILE_PROPERTIES?

When the MSVC builds have GIT_DEPRECATE_HARD defined, they don't see the declaration. Which means they think that giterr_last returns an int not a pointer, so they're treating it like a 32 bit. Which means they're looking at the wrong pointer value. Oops!

Anyway, I don't know how to fix this offhand and as the disclaimer above points out, these tests are Not Really Perfect anyway, so I'm just going to drop them. I confirmed manually that we export the git_buf and giterr functions, and we won't be doing any more deprecation in the v0.28 branch.

So I want to get this merged and 0.28.1 released so that anybody else picking up the release won't see this breakage. We can plan a better test strategy for vnext. (Building a version of the library without any deprecation and ensuring that we can link to it would be my preference.)

[b4n]

Otherwise LGTM and WFM

[b4n]

Shouldn't this have a comment as to why it's there? It feels like it'd be easy for somebody to think this is useless and drop it. Especially if you don't include your deprecated functions test case, but even then they might loose some of their time.

[ethomson]

🤔 Seems reasonable enough.