Makefile: drop duplicate %.a from link recipes

Makefile perf: all · line 3395

Quoted 3 lines · Harald Nordgren via GitGitGadget

t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS)

Makefile +1 -1

-     $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
+     $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)

I think the reason why the pattern to use only the .o files among the prerequisites and then use only the .a files among the same prerequisites (both filters $^) is used here is to make sure that the linker sees object files first before library archives, so that by the time its left-to-right scan sees the first library archive, all the missing symbols in the object files are known. The above change depends on LIBS being a strict superset of all the library archive files ($GITLIBS in the current code, but that can be updated in the future) listed as prerequisites for the rule, but there is nothing to guarantee that, so it looks brittle.

Exact same comment applies to the other two rules touched by this patch.

Quoted 18 lines · Junio C Hamano

Makefile perf: all · line 3395

Quoted 3 lines · Harald Nordgren via GitGitGadget

t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS)

Makefile +1 -1

-     $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
+     $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)

I think the reason why the pattern to use only the .o files among the prerequisites and then use only the .a files among the same prerequisites (both filters $^) is used here is to make sure that the linker sees object files first before library archives, so that by the time its left-to-right scan sees the first library archive, all the missing symbols in the object files are known. The above change depends on LIBS being a strict superset of all the library archive files ($GITLIBS in the current code, but that can be updated in the future) listed as prerequisites for the rule, but there is nothing to guarantee that, so it looks brittle.

Quoted 1 line

Exact same comment applies to the other two rules touched by this patch.

Hmm, there are other constructs like this that rely on $(LIBS) being a superset of the archives, so the three rules changed here align with the trend rather than introduce a new trend.

Not saying we shouldn't find a way to handle the overall brittleness.

Harald

From: Harald Nordgren <[email protected]>

A handful of link recipes listed archive files twice: once explicitly via $(filter %.a,$^) and again implicitly through $(LIBS), which expanded to $(filter-out %.o,$(GITLIBS)) $(EXTLIBS). On macOS the linker warned about the duplicates:

ld: warning: ignoring duplicate libraries: 'libgit.a', 'target/release/libgitcore.a'

Redefine $(LIBS) to list archive prerequisites from $^ first, then the rest of the library list with those archives filtered out so each appears only once.

Signed-off-by Harald Nordgren <[email protected]>

---
Makefile: drop duplicate %.a from test-helper link rule

Redefine $(LIBS) to list archive prerequisites from $^ first, then the rest of the library list to avoid brittleness in the future.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2314%2FHaraldNordgren%2Fmakefile-test-helper-dedup-libs-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2314/HaraldNordgren/makefile-test-helper-dedup-libs-v2
Pull-Request: https://github.com/git/git/pull/2314

Range-diff vs v1:

1: f6166450b0 ! 1: 0ef442ea05 Makefile: drop duplicate %.a from link recipes
@@ Metadata
Author: Harald Nordgren <[email protected]>

## Commit message ##
- Makefile: drop duplicate %.a from link recipes
+ Makefile: dedup archives in $(LIBS) so link recipes don't repeat them

- Three link recipes list archive files twice on the link line: once
- via $(filter %.a,$^) and again through $(LIBS), which expands to
- $(filter-out %.o,$(GITLIBS)) $(EXTLIBS). On macOS the linker warns
- about the duplicates:
+ A handful of link recipes listed archive files twice: once explicitly
+ via $(filter %.a,$^) and again implicitly through $(LIBS), which
+ expanded to $(filter-out %.o,$(GITLIBS)) $(EXTLIBS). On macOS the
+ linker warned about the duplicates:

ld: warning: ignoring duplicate libraries: 'libgit.a', 'target/release/libgitcore.a'

- Drop the redundant filter from the test-helper, fuzz-program, and
- unit-test recipes so they match the pattern used by other link
- recipes in the file.
+ Redefine $(LIBS) to list archive prerequisites from $^ first, then
+ the rest of the library list with those archives filtered out so each
+ appears only once.

Signed-off-by Harald Nordgren <[email protected]>

## Makefile ##
+@@ Makefile: endif
+ #
+ # where we use it as a dependency. Since we also pull object files
+ # from the dependency list, that would make each entry appear twice.
+-LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
++# Archives from $^ come first, then the rest with those archives
++# filtered out so each appears only once.
++LIBS = $(filter %.a,$^) $(filter-out $(filter %.a,$^),$(filter-out %.o,$(GITLIBS)) $(EXTLIBS)) +
+ BASIC_CFLAGS += $(COMPAT_CFLAGS)
+ LIB_OBJS += $(COMPAT_OBJS)
@@ Makefile: perf: all
t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) $(UNIT_TEST_DIR)/test-lib.o

 Makefile | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Makefile +6 -4

index b31ecb0756..a828a66f28 100644
@@ -2503,7 +2503,9 @@ endif
 #
 # where we use it as a dependency. Since we also pull object files
 # from the dependency list, that would make each entry appear twice.
-LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
+# Archives from $^ come first, then the rest with those archives
+# filtered out so each appears only once.
+LIBS = $(filter %.a,$^) $(filter-out $(filter %.a,$^),$(filter-out %.o,$(GITLIBS)) $(EXTLIBS))
 
 BASIC_CFLAGS += $(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)

@@ -3392,7 +3394,7 @@ perf: all
 t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) $(UNIT_TEST_DIR)/test-lib.o
 
 t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS)
-       $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
+       $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
 
 check-sha1:: t/helper/test-tool$X
        t/helper/test-sha1.sh

@@ -4015,13 +4017,13 @@ fuzz-all: $(FUZZ_PROGRAMS)
 $(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS
        $(QUIET_LINK)$(FUZZ_CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \
                -Wl,--allow-multiple-definition \
-               $(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)
+               $(filter %.o,$^) $(LIBS) $(LIB_FUZZING_ENGINE)
 
 $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_OBJS) \
        $(GITLIBS) GIT-LDFLAGS
        $(call mkdir_p_parent_template)
        $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
-               $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
+               $(filter %.o,$^) $(LIBS)
 
 GIT-TEST-SUITES: FORCE
        @FLAGS='$(CLAR_TEST_SUITES)'; \

base-commit: 9ac3f193c05c2237e2b14ebaa1149e9fc8a1abe0 --
gitgitgadget

From: Harald Nordgren <[email protected]>

A handful of link recipes listed archive files twice: once explicitly via $(filter %.a,$^) and again implicitly through $(LIBS), which expanded to $(filter-out %.o,$(GITLIBS)) $(EXTLIBS). On macOS the linker warned about the duplicates:

ld: warning: ignoring duplicate libraries: 'libgit.a', 'target/release/libgitcore.a'

Redefine $(LIBS) to list archive prerequisites from $^ first, then the rest of the library list with those archives filtered out so each appears only once.

Signed-off-by Harald Nordgren <[email protected]>

---
Makefile: drop duplicate %.a from test-helper link rule

Redefine $(LIBS) to list archive prerequisites from $^ first, then the rest of the library list to avoid brittleness in the future.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2314%2FHaraldNordgren%2Fmakefile-test-helper-dedup-libs-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2314/HaraldNordgren/makefile-test-helper-dedup-libs-v2
Pull-Request: https://github.com/git/git/pull/2314

Range-diff vs v1:

1: f6166450b0 ! 1: 0ef442ea05 Makefile: drop duplicate %.a from link recipes
@@ Metadata
Author: Harald Nordgren <[email protected]>

## Commit message ##
- Makefile: drop duplicate %.a from link recipes
+ Makefile: dedup archives in $(LIBS) so link recipes don't repeat them

- Three link recipes list archive files twice on the link line: once
- via $(filter %.a,$^) and again through $(LIBS), which expands to
- $(filter-out %.o,$(GITLIBS)) $(EXTLIBS). On macOS the linker warns
- about the duplicates:
+ A handful of link recipes listed archive files twice: once explicitly
+ via $(filter %.a,$^) and again implicitly through $(LIBS), which
+ expanded to $(filter-out %.o,$(GITLIBS)) $(EXTLIBS). On macOS the
+ linker warned about the duplicates:

ld: warning: ignoring duplicate libraries: 'libgit.a', 'target/release/libgitcore.a'

- Drop the redundant filter from the test-helper, fuzz-program, and
- unit-test recipes so they match the pattern used by other link
- recipes in the file.
+ Redefine $(LIBS) to list archive prerequisites from $^ first, then
+ the rest of the library list with those archives filtered out so each
+ appears only once.

Signed-off-by Harald Nordgren <[email protected]>

## Makefile ##
+@@ Makefile: endif
+ #
+ # where we use it as a dependency. Since we also pull object files
+ # from the dependency list, that would make each entry appear twice.
+-LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
++# Archives from $^ come first, then the rest with those archives
++# filtered out so each appears only once.
++LIBS = $(filter %.a,$^) $(filter-out $(filter %.a,$^),$(filter-out %.o,$(GITLIBS)) $(EXTLIBS)) +
+ BASIC_CFLAGS += $(COMPAT_CFLAGS)
+ LIB_OBJS += $(COMPAT_OBJS)
@@ Makefile: perf: all
t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) $(UNIT_TEST_DIR)/test-lib.o

 Makefile | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Makefile +6 -4

index b31ecb0756..a828a66f28 100644
@@ -2503,7 +2503,9 @@ endif
 #
 # where we use it as a dependency. Since we also pull object files
 # from the dependency list, that would make each entry appear twice.
-LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
+# Archives from $^ come first, then the rest with those archives
+# filtered out so each appears only once.
+LIBS = $(filter %.a,$^) $(filter-out $(filter %.a,$^),$(filter-out %.o,$(GITLIBS)) $(EXTLIBS))
 
 BASIC_CFLAGS += $(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)

@@ -3392,7 +3394,7 @@ perf: all
 t/helper/test-tool$X: $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS)) $(UNIT_TEST_DIR)/test-lib.o
 
 t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS)
-       $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
+       $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
 
 check-sha1:: t/helper/test-tool$X
        t/helper/test-sha1.sh

@@ -4015,13 +4017,13 @@ fuzz-all: $(FUZZ_PROGRAMS)
 $(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS
        $(QUIET_LINK)$(FUZZ_CXX) $(FUZZ_CXXFLAGS) -o $@ $(ALL_LDFLAGS) \
                -Wl,--allow-multiple-definition \
-               $(filter %.o,$^) $(filter %.a,$^) $(LIBS) $(LIB_FUZZING_ENGINE)
+               $(filter %.o,$^) $(LIBS) $(LIB_FUZZING_ENGINE)
 
 $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_OBJS) \
        $(GITLIBS) GIT-LDFLAGS
        $(call mkdir_p_parent_template)
        $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
-               $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
+               $(filter %.o,$^) $(LIBS)
 
 GIT-TEST-SUITES: FORCE
        @FLAGS='$(CLAR_TEST_SUITES)'; \

base-commit: 9ac3f193c05c2237e2b14ebaa1149e9fc8a1abe0 --
gitgitgadget

I think this would be quite nice to fix for all the macOS developers (I don't know how many we have who are active on this list), but when running repeated tests it does take up some space on the terminal:

````
❯ git rebase --keep-base -x 'make -s && cd t && prove -j8 t345?-history*.sh && echo'

Executing: make -s && cd t && prove -j8 t345?-history*.sh && echo GIT_VERSION=2.55.0.rc1.20.g1e31474ef6
ld: warning: ignoring duplicate libraries: 'libgit.a', 'target/release/libgitcore.a'
ld: warning: ignoring duplicate libraries: 'libgit.a', 'target/release/libgitcore.a'

From: Harald Nordgren <[email protected]>

Building on macOS with Xcode 15 or newer emits:

ld: warning: ignoring duplicate libraries: 'libgit.a', 'target/release/libgitcore.a'

Some link recipes list the same archive twice, which is harmless. Quiet the warning instead.

Pass -Wl,-no_warn_duplicate_libraries on Xcode 15 and newer, whose linkers added both the warning and the suppression flag (ld64-907 and dyld-1009). Earlier linkers reject the flag, so gate on the linker version. Broaden the existing -fno-common version probe to also match the "ld64-NNN" and "dyld-NNN" forms Xcode 15 reports.

Signed-off-by Harald Nordgren <[email protected]>

---

 config.mak.uname | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

config.mak.uname +9 -2

index 8719e09f66..9ebd240378 100644
@@ -173,8 +173,15 @@ ifeq ($(uname_S),Darwin)
 		NEEDS_GOOD_LIBICONV = UnfortunatelyYes
         endif
 
-	# Silence Xcode 16.3+ linker warning about __DATA,__common alignment.
-	LD_MAJOR_VERSION = $(shell ld -v 2>&1 | sed -n 's/.*PROJECT:ld-\([0-9]*\).*/\1/p')
+	# ld reports "PROJECT:{ld,ld64,dyld}-NNN", match any of the three.
+	LD_MAJOR_VERSION = $(shell ld -v 2>&1 | sed -n 's/.*PROJECT:[^ ]*-\([0-9][0-9]*\).*/\1/p')
+
+	# Silence the Xcode 15+ warning about archives listed more than once.
+        ifeq ($(shell test -n "$(LD_MAJOR_VERSION)" && test "$(LD_MAJOR_VERSION)" -ge 907 && echo 1),1)
+		BASIC_LDFLAGS += -Wl,-no_warn_duplicate_libraries
+        endif
+
+	# Silence the Xcode 16.3+ warning about __DATA,__common alignment.
         ifeq ($(shell test -n "$(LD_MAJOR_VERSION)" && test "$(LD_MAJOR_VERSION)" -ge 1167 && echo 1),1)
 		BASIC_CFLAGS += -fno-common
         endif

base-commit: 95e20213faefeb95df29277c58ac1980ab68f701

Quoted 10 lines · Harald Nordgren via GitGitGadget

From: Harald Nordgren <[email protected]>

Building on macOS with Xcode 15 or newer emits:

ld: warning: ignoring duplicate libraries: 'libgit.a', 'target/release/libgitcore.a'

Some link recipes list the same archive twice, which is harmless. Quiet the warning instead.

Quoted 8 lines

Pass -Wl,-no_warn_duplicate_libraries on Xcode 15 and newer, whose linkers added both the warning and the suppression flag (ld64-907 and dyld-1009). Earlier linkers reject the flag, so gate on the linker version. Broaden the existing -fno-common version probe to also match the "ld64-NNN" and "dyld-NNN" forms Xcode 15 reports.

Signed-off-by Harald Nordgren <[email protected]>

---

Yeah, this looks like what I expected.

A few things to note.

* Can folks with different versions of Xcode (or is 15 sufficiently old that practically nobody is expected to have anything older?) test this patch?

* We only patch Makefile here; can folks who use meson report how well your build goes?

Thanks.

Quoted 26 lines · Junio C Hamano

Quoted 10 lines · Harald Nordgren via GitGitGadget

From: Harald Nordgren <[email protected]>

Building on macOS with Xcode 15 or newer emits:

ld: warning: ignoring duplicate libraries: 'libgit.a', 'target/release/libgitcore.a'

Some link recipes list the same archive twice, which is harmless. Quiet the warning instead.

Quoted 8 lines

Pass -Wl,-no_warn_duplicate_libraries on Xcode 15 and newer, whose linkers added both the warning and the suppression flag (ld64-907 and dyld-1009). Earlier linkers reject the flag, so gate on the linker version. Broaden the existing -fno-common version probe to also match the "ld64-NNN" and "dyld-NNN" forms Xcode 15 reports.

Signed-off-by Harald Nordgren <[email protected]>

---

Yeah, this looks like what I expected.

A few things to note.

Quoted 8 lines

* Can folks with different versions of Xcode (or is 15 sufficiently old that practically nobody is expected to have anything older?) test this patch?

* We only patch Makefile here; can folks who use meson report how well your build goes?

Thanks.

On one (old) machine I have available:

    $ pkgutil --pkg-info=com.apple.pkg.CLTools_Executables
    [trimmed]
    version: 14.2.0.0.1.1668646533

On said machine, I don't get the duplicate warnings on a Meson build. No issues with the patch when running make.

I think I have seen this on my other machine, which is much newer. When I get around to trying it there, I'll report back as well.

Quoted 10 lines · Harald Nordgren via GitGitGadget

From: Harald Nordgren <[email protected]>

Building on macOS with Xcode 15 or newer emits:

ld: warning: ignoring duplicate libraries: 'libgit.a', 'target/release/libgitcore.a'

Some link recipes list the same archive twice, which is harmless. Quiet the warning instead.

Quoted 8 lines

Pass -Wl,-no_warn_duplicate_libraries on Xcode 15 and newer, whose linkers added both the warning and the suppression flag (ld64-907 and dyld-1009). Earlier linkers reject the flag, so gate on the linker version. Broaden the existing -fno-common version probe to also match the "ld64-NNN" and "dyld-NNN" forms Xcode 15 reports.

Signed-off-by Harald Nordgren <[email protected]>

---

Yeah, this looks like what I expected.

A few things to note.

* Can folks with different versions of Xcode (or is 15 sufficiently old that practically nobody is expected to have anything older?) test this patch?

* We only patch Makefile here; can folks who use meson report how well your build goes?

Quoted 10 lines

Yeah, this looks like what I expected.

A few things to note.

* Can folks with different versions of Xcode (or is 15 sufficiently old that practically nobody is expected to have anything older?) test this patch?

* We only patch Makefile here; can folks who use meson report how well your build goes?

When using Meson we also see a warning. This got partially fixed in Meson itself though via [1], where it started to disable the warning when compiling with "--fatal-warnings" so that it doesn't cause builds to break. So starting with that commit it really only is a harmless (albeit annoying) warning.

Arguably, it might make sense to unconditionally disable this warning, as it doesn't seem to add anything of value. I've Cc'd Paolo, one of the Meson maintainers.

Thanks!

Patrick

[1]: https://github.com/mesonbuild/meson/commit/17d1cc60ed8246b8e7f0786421bf1cdf5cb19254

Quoted 1 line

[1]:https://github.com/mesonbuild/meson/ commit/17d1cc60ed8246b8e7f0786421bf1cdf5cb19254

I took my inspiration for -Wl,-no-whatever from Paolo's other attempt, referenced in

     https://github.com/mesonbuild/meson/issues/15553

which is

https://github.com/mesonbuild/meson/
commit/7c901d7a8af214e31788eb6d1a1edd5b75124e66

Quoted 7 lines · Junio C Hamano

Quoted 2 lines

[1]:https://github.com/mesonbuild/meson/
commit/17d1cc60ed8246b8e7f0786421bf1cdf5cb19254

I took my inspiration for -Wl,-no-whatever from Paolo's other attempt, referenced in

     https://github.com/mesonbuild/meson/issues/15553

Quoted 4 lines

which is

https://github.com/mesonbuild/meson/
commit/7c901d7a8af214e31788eb6d1a1edd5b75124e66

Yeah, it makes sense for Meson to disable it unconditionally. I wouldn't bother adding a check in meson.build though, since as Patrick mentioned it's mostly a nuisance.

Quoted 13 lines · Paolo Bonzini

Quoted 11 lines · Junio C Hamano

Quoted 2 lines

[1]:https://github.com/mesonbuild/meson/
commit/17d1cc60ed8246b8e7f0786421bf1cdf5cb19254

I took my inspiration for -Wl,-no-whatever from Paolo's other attempt, referenced in

     https://github.com/mesonbuild/meson/issues/15553

which is

https://github.com/mesonbuild/meson/
commit/7c901d7a8af214e31788eb6d1a1edd5b75124e66

Quoted 3 lines

Yeah, it makes sense for Meson to disable it unconditionally. I wouldn't bother adding a check in meson.build though, since as Patrick mentioned it's mostly a nuisance.

Is this something you want to implement in Meson yourself? Otherwise I'm happy to create a pull request.