From: Kyle Lippincott <spectral@google.com>
To: phillip.wood@dunelm.org.uk
Cc: Calvin Wan <calvinwan@google.com>,
Git Mailing List <git@vger.kernel.org>,
"brian m. carlson" <sandals@crustytoothpaste.net>,
rsbecker@nexbridge.com, Josh Steadmon <steadmon@google.com>,
Emily Shaffer <nasamuffin@google.com>,
Enrico Mrass <emrass@google.com>
Subject: Re: [RFD] Libification proposal: separate internal and external interfaces
Date: Fri, 10 May 2024 14:35:52 -0700 [thread overview]
Message-ID: <CAO_smVgEkLN=rFDEDM-JV9mXQK4CCx0u5W1d+YXse0Qy3QMnsA@mail.gmail.com> (raw)
In-Reply-To: <12a4cd37-1cef-45a0-9b96-36a978e52dba@gmail.com>
On Fri, May 10, 2024 at 2:52 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Kyle
>
> On 09/05/2024 02:00, Kyle Lippincott wrote:
> > // RENAMED from previous code block (no other changes)
> > // In a .c file that is "library internal".
> > // This translation unit can assume that we've done #include
> > "git-compat-util.h" and anything else it wants.
> > int strbuf_grow_impl(struct strbuf *sb, size_t extra)
> > {
> > int new_buf = !sb->alloc;
> > if (unsigned_add_overflows(extra, 1) ||
> > unsigned_add_overflows(sb->len, extra + 1))
> > return -1;
> > if (new_buf)
> > sb->buf = NULL;
> > ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
> > if (new_buf)
> > sb->buf[0] = '\0';
> > return 0;
> > }
> >
> > // In a .c file for the interface as used by other projects:
> > int gitlib_strbuf_grow(struct strbuf *sb, size_t extra)
> > {
> > return strbuf_grow_impl(sb, extra);
> > }
> >
> > // NEW from previous code block
> > // In a .c file for the interface as used by the git project itself:
> > void strbuf_grow(struct strbuf *sb, size_t extra)
> > {
> > if (strbuf_grow_impl(sb, extra))
> > die("you want to use way too much memory")
> > }
> >
> > I'm recommending this pattern primarily because of our platform
> > support concerns. If we can't elevate the entire project to assume
> > that C99 is available in a standards compliant way, we can't have
> > header files that look like this be part of building the `git` binary
> > itself (or any of the helper binaries):
> >
> > #include <stdint.h> /* Our platform support policy doesn't allow this */
> > int gitlib_strbuf_grow(struct strbuf *sb, size_t extra);
>
> We have had a test balloon [1] requiring C99 for two and a half years
> without any bug reports so I think we are probably safe to assume the
> test balloon has succeeded and that we can depend on on the presence of
> <stdint.h>.
In the past 5 months, I have been told that this isn't a valid
assumption to make yet. I will note that the test balloon just checks
for the compiler claiming it supports C99, not that it actually does
so properly. This is why stdbool is still in test balloon stage
(8277dbe987 (git-compat-util: convert skip_{prefix,suffix}{,_mem} to
bool, 2023-12-16)).
If that's no longer the case, and we _can_ rely on functionality
specified by the C99 standard, such as stdint.h existing, then using
the same interfaces internally that we expose externally becomes a lot
easier.
> Note that the [u]intptr_t types are optional and so we'd
> need to make sure we avoid them in public interfaces.
>
> [1] 7bc341e21b5 (git-compat-util: add a test balloon for C99 support,
> 2021-12-01)
>
> > It's not just the #includes, though. As stated in the original
> > document, we run into problems with platform-defined types and
> > everything else that's tweaked in/provided by git-compat-util.h:
> > - This header file that's included in the non-git projects can't use
> > `off_t` or `struct stat`.
>
> In principle we could change the interfaces that currently use `off_t`
> to use `int64_t` and convert to `off_t` in the function body which would
> avoid having to have separate wrappers for the internal and external
> callers. I'm not sure how invasive that would be though. `struct stat`
> is trickier - where do we expose that in our interfaces?
I hadn't actually checked if we do expose `struct stat` in any
function declaration, it was just an example of something that changes
size/behavior due to the #defines. But we do use it in the following
places:
entry.h's `fstat_checkout_output` and `update_ce_after_write`
reftable/stack.h's `struct reftable_stack` has a `struct stat` member.
4 uses in git-compat-util.h
several uses in read-cache-ll.h (which sounds like it probably
wouldn't be exposed in an interface, being lowlevel :))
statinfo.h
object-file.h's `index_fd` and `index_path`
dir-iterator.h's `struct dir_iterator` has a `struct stat` member
I may have missed one or two .h files. Maybe all of these are internal
things that we won't be exposing in an "external" interface, and
that's great: no need to worry about it then.
>
> > - This header file can't assume that any types related to sockets are
> > available, because those come from <sys/socket.h> on Linux and from
> > winsock2.h on Windows.
> > - It can't assume that we have `NORETURN` (and it can't assume that we
> > don't need it), or `MAYBE_UNUSED`, or ...
>
> These problems and the _GNU_SOURCE on you mention below must be pretty
> common for cross-platform libraries - how do other projects handle them?
> On the face of it this seems like it would be fairly simple to solve by
> including a file that contains the subset of git-compat-util.h that
> defines these macros (with a suitable LIBGIT_ prefix) in libgit.h.
I think that either the library interface just #includes whatever they
need, and if that doesn't work, you held the library wrong and it's up
to you to fix it (by doing whatever #defines are necessary either on
the compiler commandline or before including the library's header), or
they wrap everything in a layer of indirection, so only the library's
code itself needs to be compiled with any special options/#defines.
I'm advocating for the second here.
>
> > Most of those issues _may_ be able to be resolved by having a
> > "gitlib-compat-util.h" file included at the top of the "external
> > project" .h file. But that's insufficient. Example:
> >
> > #include <unistd.h>
> > #include "git/gitlib.h" // Oops, the `#define _GNU_SOURCE` in the
> > transitive "gitlib-compat-util.h" has no effect!
> >
> > Or the opposite:
> >
> > #include "git/gitlib.h" // Oops, this set _FILE_OFFSET_BITS=64 when
> > the project wasn't expecting it!
> > #include <unistd.h> // For this translation unit only, `off_t` might
> > be a different size than elsewhere in the project, I hope you like
> > debugging segfaults.
> >
> > The only ways I could come up with to solve these problems were to
> > hold the "external interface" to a different standard, that is
> > simultaneously both more permissive (it can assume C99), and
> > restrictive (it can't rely on things like off_t),
> >
> > incompatible with these external interfaces being used by the git
> > project itself, which has a broader set of platforms it needs to
> > support. But the external interfaces must be very simple wrappers
> > around code that _is_ shared with the git executable.
>
> I agree we should minimize the amount of code in the wrappers for
> external callers.
>
> Best Wishes
>
> Phillip
next prev parent reply other threads:[~2024-05-10 21:36 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-02 14:18 [RFD] Libification proposal: separate internal and external interfaces Calvin Wan
2024-04-07 21:33 ` brian m. carlson
2024-04-07 21:48 ` rsbecker
2024-04-08 1:09 ` brian m. carlson
2024-04-08 11:07 ` rsbecker
2024-04-08 21:29 ` Junio C Hamano
2024-04-09 0:35 ` brian m. carlson
2024-04-09 17:26 ` Calvin Wan
2024-04-09 9:40 ` Phillip Wood
2024-04-09 17:30 ` Calvin Wan
2024-04-22 16:26 ` Calvin Wan
2024-04-22 20:28 ` Junio C Hamano
2024-04-23 9:57 ` phillip.wood123
2024-05-09 1:00 ` Kyle Lippincott
2024-05-10 9:52 ` Phillip Wood
2024-05-10 21:35 ` Kyle Lippincott [this message]
2024-05-09 19:45 ` Kyle Lippincott
2024-05-09 20:14 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAO_smVgEkLN=rFDEDM-JV9mXQK4CCx0u5W1d+YXse0Qy3QMnsA@mail.gmail.com' \
--to=spectral@google.com \
--cc=calvinwan@google.com \
--cc=emrass@google.com \
--cc=git@vger.kernel.org \
--cc=nasamuffin@google.com \
--cc=phillip.wood@dunelm.org.uk \
--cc=rsbecker@nexbridge.com \
--cc=sandals@crustytoothpaste.net \
--cc=steadmon@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).