Re: [PATCH] Makefile: link osxkeychain helper against Rust

From: Koji Nakamaru <koji.nakamaru@gree.net>
Date: May 8, 2026, 09:33 UTC
Message-ID: <CAOTNsDyygMEz4dgfkWKb=wWP0g9MhNFgDHzPPEODbMGmDQLTOQ@mail.gmail.com>
In-Reply-To: <xmqqlddufw5d.fsf@gitster.g>

On Fri, May 8, 2026 at 11:54 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 66 quoted lines

>
> "Shardul Natu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Shnatu <snatu@google.com>
>
> If your name is "Shardul Natu", we'd prefer (not 'require', but
> 'prefer') that the patches authored by you also identify with that
> name, both on "From:" and "Signed-off-by:"..
>
> > When Rust is enabled, ensure that the git-credential-osxkeychain
> > helper is linked with the necessary Rust libraries.
> >
> > Introduce the RUST_LIBS variable inside ifndef NO_RUST block
> > to hold the Rust library dependency, and use it in the helper's
> > build target. This cleanly handles cases where Rust is disabled,
> > making it a no-op and avoiding any build failures on systems
> > without Cargo.
> >
> > This addresses reviewer feedback from internal CL 910223487
> > by simplifying the variables and avoiding confusing "LINK"
> > terminology.
> >
> > Signed-off-by: Shnatu <snatu@google.com>
> > ---
> >     Makefile: link osxkeychain helper against Rust
>
> Thanks.  I've added to CC: a few folks who may be more clueful in
> the affected area than I am.  It somehow feels strange that we have
> to have RUST_LIB and RUST_LIBS separately, and apparently with the
> new definition the latter is expected to be a superset of the
> former, and it is unclear what are the things that should be added
> to the latter without getting added to the former.
>
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2288%2Fkiranani%2Fnext-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2288/kiranani/next-v1
> > Pull-Request: https://github.com/git/git/pull/2288
> >
> >  Makefile | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index f86173f93a..a17dca22b1 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1593,6 +1593,7 @@ ALL_LDFLAGS = $(LDFLAGS) $(LDFLAGS_APPEND)
> >  ifndef NO_RUST
> >  BASIC_CFLAGS += -DWITH_RUST
> >  GITLIBS += $(RUST_LIB)
> > +RUST_LIBS = $(RUST_LIB)
> >  ifeq ($(uname_S),Windows)
> >  EXTLIBS += -luserenv
> >  endif
> > @@ -4082,9 +4083,9 @@ $(LIBGIT_HIDDEN_EXPORT): $(LIBGIT_PARTIAL_EXPORT)
> >  contrib/libgit-sys/libgitpub.a: $(LIBGIT_HIDDEN_EXPORT)
> >       $(AR) $(ARFLAGS) $@ $^
> >
> > -contrib/credential/osxkeychain/git-credential-osxkeychain: contrib/credential/osxkeychain/git-credential-osxkeychain.o $(LIB_FILE) GIT-LDFLAGS
> > +contrib/credential/osxkeychain/git-credential-osxkeychain: contrib/credential/osxkeychain/git-credential-osxkeychain.o $(LIB_FILE) $(RUST_LIBS) GIT-LDFLAGS
> >       $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
> > -             $(filter %.o,$^) $(LIB_FILE) $(EXTLIBS) -framework Security -framework CoreFoundation
> > +             $(filter %.o,$^) $(LIB_FILE) $(RUST_LIBS) $(EXTLIBS) -framework Security -framework CoreFoundation
> >
> >  contrib/credential/osxkeychain/git-credential-osxkeychain.o: contrib/credential/osxkeychain/git-credential-osxkeychain.c GIT-CFLAGS
> >       $(QUIET_LINK)$(CC) -o $@ -c $(dep_args) $(compdb_args) $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<
> >
> > base-commit: 4f69b47b940100b02630f745a52f9d9850f122b2

How about simply wrapping the RUST_LIB-related sections in ifndef NO_RUST, as shown below? This way, we can avoid defining RUST_LIBS.

diff --git a/Makefile b/Makefile
index f86173f93a..daa1691950 100644
--- a/Makefile
+++ b/Makefile
@@ -947,11 +947,13 @@ else
 RUST_TARGET_DIR = target/release
 endif

+ifndef NO_RUST
 ifeq ($(uname_S),Windows)
 RUST_LIB = $(RUST_TARGET_DIR)/gitcore.lib
 else
 RUST_LIB = $(RUST_TARGET_DIR)/libgitcore.a
 endif
+endif

 GITLIBS = common-main.o $(LIB_FILE)
 EXTLIBS =
@@ -3027,11 +3029,13 @@ scalar$X: scalar.o GIT-LDFLAGS $(GITLIBS)
 $(LIB_FILE): $(LIB_OBJS)
        $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^

+ifndef NO_RUST
 $(RUST_LIB): Cargo.toml $(RUST_SOURCES) $(LIB_FILE)
        $(QUIET_CARGO)cargo build $(CARGO_ARGS)

 .PHONY: rust
 rust: $(RUST_LIB)
+endif

 export DEFAULT_EDITOR DEFAULT_PAGER

@@ -4082,9 +4086,9 @@ $(LIBGIT_HIDDEN_EXPORT): $(LIBGIT_PARTIAL_EXPORT)
 contrib/libgit-sys/libgitpub.a: $(LIBGIT_HIDDEN_EXPORT)
        $(AR) $(ARFLAGS) $@ $^

-contrib/credential/osxkeychain/git-credential-osxkeychain:
contrib/credential/osxkeychain/git-credential-osxkeychain.o
$(LIB_FILE) GIT-LDFLAGS
+contrib/credential/osxkeychain/git-credential-osxkeychain:
contrib/credential/osxkeychain/git-credential-osxkeychain.o
$(LIB_FILE) $(RUST_LIB) GIT-LDFLAGS
        $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
-               $(filter %.o,$^) $(LIB_FILE) $(EXTLIBS) -framework
Security -framework CoreFoundation
+               $(filter %.o,$^) $(LIB_FILE) $(RUST_LIB) $(EXTLIBS)
-framework Security -framework CoreFoundation

 contrib/credential/osxkeychain/git-credential-osxkeychain.o:
contrib/credential/osxkeychain/git-credential-osxkeychain.c GIT-CFLAGS
        $(QUIET_LINK)$(CC) -o $@ -c $(dep_args) $(compdb_args)
$(ALL_CFLAGS) $(EXTRA_CPPFLAGS) $<