Remove the hardcoded lists of objects in favor of using
$(wildcard). This means that every time a built-in, test tool etc. is
added we won't need to patch the top-level Makefile, except for the
few remaining cases where the asset in question would make it onto one
of our list of exceptions.
Ever since 81b50f3ce40 (Move 'builtin-*' into a 'builtin/'
subdirectory, 2010-02-22) this has been relatively easy to do (and
even before that we could glob builtin-*.c). This pattern of
exhaustively enumerating files was then carried forward for
e.g. TEST_BUILTINS_OBJS in efd71f8913a (t/helper: add an empty
test-tool program, 2018-03-24).
One reason not to do this is that now a new *.c file at the top-level
will be immediately picked up, so if a new *.c file is being worked on
"make" will error if it doesn't compile, whereas before that file
would need to be explicitly listed in the Makefile. I think than small
trade-off is worth it.
There's a few small "while we're at it" changes here, since I'm
touching the code in question:
- Start splitting up the the "Guard against the environment" section
at the top, but don't move anything that exists there out to avoid
merge conflicts
- The $(TEST_BUILTINS_OBJS) variable was needlessly complex, because
it didn't have the full paths we'd pathsubst it back & forth.
- Introduce *_SRC in addition to *_OBJ for the variable I'm
touching. Eventually we'll want to do this for all the *.o files,
i.e. make the *.c list a source of truth for *.o, which means we can
e.g. use that exhaustive list for "make TAGS".
- Add a missing "curl-objs" target. See 029bac01a87 (Makefile: add
{program,xdiff,test,git,fuzz}-objs & objects targets, 2021-02-23)
for the commit that added the rest.
- De-indent an "ifndef" block, we don't usually indent their
contents.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
There's probably never a good time to submit a change like this,
i.e. it'll likely always conflict with something, perhaps the
around-release period is paradoxically better than most.
This conflicts with some existing topics (including one of my own to
add the "hook" built in), but those merge conflicts are resolvable by
keeping this side of the conflict. I.e. we'll no longer need to
manually maintain these lists in the Makefile for the common cases.
Makefile | 484 +++++++------------------------------------------------
1 file changed, 54 insertions(+), 430 deletions(-)
@@ -590,6 +590,19 @@ TEST_OBJS =TEST_PROGRAMS_NEED_X=THIRD_PARTY_SOURCES=+## Guard against env: programs+TEST_PROGRAMS=++## Guard against env: sources+CURL_SRC=+TEST_PROGRAMS_NEED_X_SRC=+XDIFF_SRC=++## Guard against env: objects+ALL_COMPAT_OBJS=+CURL_OBJS=+LIB_OBJS_DIRS=+# Having this variable in your environment would break pipelines because# you cause "cd" to echo its destination to stdout. It can also take# scripts to unexpected places. If you like CDPATH, define it for your
@@ -688,87 +701,23 @@ X =PROGRAMS+=$(patsubst%.o,git-%$X,$(PROGRAM_OBJS))-TEST_BUILTINS_OBJS+=test-advise.o-TEST_BUILTINS_OBJS+=test-bitmap.o-TEST_BUILTINS_OBJS+=test-bloom.o-TEST_BUILTINS_OBJS+=test-chmtime.o-TEST_BUILTINS_OBJS+=test-config.o-TEST_BUILTINS_OBJS+=test-crontab.o-TEST_BUILTINS_OBJS+=test-ctype.o-TEST_BUILTINS_OBJS+=test-date.o-TEST_BUILTINS_OBJS+=test-delta.o-TEST_BUILTINS_OBJS+=test-dir-iterator.o-TEST_BUILTINS_OBJS+=test-drop-caches.o-TEST_BUILTINS_OBJS+=test-dump-cache-tree.o-TEST_BUILTINS_OBJS+=test-dump-fsmonitor.o-TEST_BUILTINS_OBJS+=test-dump-split-index.o-TEST_BUILTINS_OBJS+=test-dump-untracked-cache.o-TEST_BUILTINS_OBJS+=test-example-decorate.o-TEST_BUILTINS_OBJS+=test-fast-rebase.o-TEST_BUILTINS_OBJS+=test-genrandom.o-TEST_BUILTINS_OBJS+=test-genzeros.o-TEST_BUILTINS_OBJS+=test-getcwd.o-TEST_BUILTINS_OBJS+=test-hash-speed.o-TEST_BUILTINS_OBJS+=test-hash.o-TEST_BUILTINS_OBJS+=test-hashmap.o-TEST_BUILTINS_OBJS+=test-index-version.o-TEST_BUILTINS_OBJS+=test-json-writer.o-TEST_BUILTINS_OBJS+=test-lazy-init-name-hash.o-TEST_BUILTINS_OBJS+=test-match-trees.o-TEST_BUILTINS_OBJS+=test-mergesort.o-TEST_BUILTINS_OBJS+=test-mktemp.o-TEST_BUILTINS_OBJS+=test-oid-array.o-TEST_BUILTINS_OBJS+=test-oidmap.o-TEST_BUILTINS_OBJS+=test-oidtree.o-TEST_BUILTINS_OBJS+=test-online-cpus.o-TEST_BUILTINS_OBJS+=test-parse-options.o-TEST_BUILTINS_OBJS+=test-parse-pathspec-file.o-TEST_BUILTINS_OBJS+=test-partial-clone.o-TEST_BUILTINS_OBJS+=test-path-utils.o-TEST_BUILTINS_OBJS+=test-pcre2-config.o-TEST_BUILTINS_OBJS+=test-pkt-line.o-TEST_BUILTINS_OBJS+=test-prio-queue.o-TEST_BUILTINS_OBJS+=test-proc-receive.o-TEST_BUILTINS_OBJS+=test-progress.o-TEST_BUILTINS_OBJS+=test-reach.o-TEST_BUILTINS_OBJS+=test-read-cache.o-TEST_BUILTINS_OBJS+=test-read-graph.o-TEST_BUILTINS_OBJS+=test-read-midx.o-TEST_BUILTINS_OBJS+=test-ref-store.o-TEST_BUILTINS_OBJS+=test-regex.o-TEST_BUILTINS_OBJS+=test-repository.o-TEST_BUILTINS_OBJS+=test-revision-walking.o-TEST_BUILTINS_OBJS+=test-run-command.o-TEST_BUILTINS_OBJS+=test-scrap-cache-tree.o-TEST_BUILTINS_OBJS+=test-serve-v2.o-TEST_BUILTINS_OBJS+=test-sha1.o-TEST_BUILTINS_OBJS+=test-sha256.o-TEST_BUILTINS_OBJS+=test-sigchain.o-TEST_BUILTINS_OBJS+=test-simple-ipc.o-TEST_BUILTINS_OBJS+=test-strcmp-offset.o-TEST_BUILTINS_OBJS+=test-string-list.o-TEST_BUILTINS_OBJS+=test-submodule-config.o-TEST_BUILTINS_OBJS+=test-submodule-nested-repo-config.o-TEST_BUILTINS_OBJS+=test-subprocess.o-TEST_BUILTINS_OBJS+=test-trace2.o-TEST_BUILTINS_OBJS+=test-urlmatch-normalization.o-TEST_BUILTINS_OBJS+=test-userdiff.o-TEST_BUILTINS_OBJS+=test-wildmatch.o-TEST_BUILTINS_OBJS+=test-windows-named-pipe.o-TEST_BUILTINS_OBJS+=test-write-cache.o-TEST_BUILTINS_OBJS+=test-xml-encode.o-# Do not add more tests here unless they have extra dependencies. Add# them in TEST_BUILTINS_OBJS above.TEST_PROGRAMS_NEED_X+=test-fake-sshTEST_PROGRAMS_NEED_X+=test-tool-TEST_PROGRAMS=$(patsubst%,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))+TEST_PROGRAMS_NEED_X_SRC+=$(TEST_PROGRAMS_NEED_X:%=t/helper/%.c)+TEST_PROGRAMS+=$(TEST_PROGRAMS_NEED_X_SRC:%.c=%$X)+TEST_BUILTINS_SRC+=$(filter-out$(TEST_PROGRAMS_NEED_X_SRC),$(wildcardt/helper/*.c))+TEST_BUILTINS_OBJS+=$(TEST_BUILTINS_SRC:%.c=%.o)-# List built-in command $C whose implementation cmd_$C() is not in-# builtin/$C.o but is linked in as part of some other command.+# List built-in command $C whose implementation cmd_$C() is in+# builtin/$C.o+BUILTIN_OBJS=$(patsubst%.c,%.o,$(wildcardbuiltin/*.c))BUILT_INS+=$(patsubstbuiltin/%.o,git-%$X,$(BUILTIN_OBJS))+# List built-in command $C whose implementation cmd_$C() is not in+# builtin/$C.o but is linked in as part of some other command.BUILT_INS+=git-cherry$XBUILT_INS+=git-cherry-pick$XBUILT_INS+=git-format-patch$X
@@ -828,355 +777,28 @@ LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentat-nameDocumentation-prune-o\-name'*.h'-print)))-LIB_OBJS+=abspath.o-LIB_OBJS+=add-interactive.o-LIB_OBJS+=add-patch.o-LIB_OBJS+=advice.o-LIB_OBJS+=alias.o-LIB_OBJS+=alloc.o-LIB_OBJS+=apply.o-LIB_OBJS+=archive-tar.o-LIB_OBJS+=archive-zip.o-LIB_OBJS+=archive.o-LIB_OBJS+=attr.o-LIB_OBJS+=base85.o-LIB_OBJS+=bisect.o-LIB_OBJS+=blame.o-LIB_OBJS+=blob.o-LIB_OBJS+=bloom.o-LIB_OBJS+=branch.o-LIB_OBJS+=bulk-checkin.o-LIB_OBJS+=bundle.o-LIB_OBJS+=cache-tree.o-LIB_OBJS+=cbtree.o-LIB_OBJS+=chdir-notify.o-LIB_OBJS+=checkout.o-LIB_OBJS+=chunk-format.o-LIB_OBJS+=color.o-LIB_OBJS+=column.o-LIB_OBJS+=combine-diff.o-LIB_OBJS+=commit-graph.o-LIB_OBJS+=commit-reach.o-LIB_OBJS+=commit.o+# LIB_OBJS: compat/* objects that live at the top-level+ALL_COMPAT_OBJS+=unix-socket.o+ALL_COMPAT_OBJS+=unix-stream-server.o+ALL_COMPAT_OBJS+=sha1dc_git.o++# LIB_OBJS: Mostly glob *.c at the top-level, with some exlusions+LIB_OBJS+=$(filter-out\+$(ALL_COMPAT_OBJS)\+git.ocommon-main.o$(PROGRAM_OBJS)\+$(FUZZ_OBJS)$(CURL_OBJS),\+$(patsubst%.c,%.o,$(wildcard*.c)))++# LIB_OBJS: Directories that contain only LIB_OBJS+LIB_OBJS_DIRS+=ewah+LIB_OBJS_DIRS+=negotiator+LIB_OBJS_DIRS+=refs+LIB_OBJS_DIRS+=trace2+LIB_OBJS+=$(patsubst%.c,%.o,$(foreachdir,$(LIB_OBJS_DIRS),$(wildcard$(dir)/*.c)))++# LIB_OBJS: unconditional compat/* objectsLIB_OBJS+=compat/obstack.oLIB_OBJS+=compat/terminal.o-LIB_OBJS+=config.o-LIB_OBJS+=connect.o-LIB_OBJS+=connected.o-LIB_OBJS+=convert.o-LIB_OBJS+=copy.o-LIB_OBJS+=credential.o-LIB_OBJS+=csum-file.o-LIB_OBJS+=ctype.o-LIB_OBJS+=date.o-LIB_OBJS+=decorate.o-LIB_OBJS+=delta-islands.o-LIB_OBJS+=diff-delta.o-LIB_OBJS+=diff-merges.o-LIB_OBJS+=diff-lib.o-LIB_OBJS+=diff-no-index.o-LIB_OBJS+=diff.o-LIB_OBJS+=diffcore-break.o-LIB_OBJS+=diffcore-delta.o-LIB_OBJS+=diffcore-order.o-LIB_OBJS+=diffcore-pickaxe.o-LIB_OBJS+=diffcore-rename.o-LIB_OBJS+=diffcore-rotate.o-LIB_OBJS+=dir-iterator.o-LIB_OBJS+=dir.o-LIB_OBJS+=editor.o-LIB_OBJS+=entry.o-LIB_OBJS+=environment.o-LIB_OBJS+=ewah/bitmap.o-LIB_OBJS+=ewah/ewah_bitmap.o-LIB_OBJS+=ewah/ewah_io.o-LIB_OBJS+=ewah/ewah_rlw.o-LIB_OBJS+=exec-cmd.o-LIB_OBJS+=fetch-negotiator.o-LIB_OBJS+=fetch-pack.o-LIB_OBJS+=fmt-merge-msg.o-LIB_OBJS+=fsck.o-LIB_OBJS+=fsmonitor.o-LIB_OBJS+=gettext.o-LIB_OBJS+=gpg-interface.o-LIB_OBJS+=graph.o-LIB_OBJS+=grep.o-LIB_OBJS+=hash-lookup.o-LIB_OBJS+=hashmap.o-LIB_OBJS+=help.o-LIB_OBJS+=hex.o-LIB_OBJS+=hook.o-LIB_OBJS+=ident.o-LIB_OBJS+=json-writer.o-LIB_OBJS+=kwset.o-LIB_OBJS+=levenshtein.o-LIB_OBJS+=line-log.o-LIB_OBJS+=line-range.o-LIB_OBJS+=linear-assignment.o-LIB_OBJS+=list-objects-filter-options.o-LIB_OBJS+=list-objects-filter.o-LIB_OBJS+=list-objects.o-LIB_OBJS+=ll-merge.o-LIB_OBJS+=lockfile.o-LIB_OBJS+=log-tree.o-LIB_OBJS+=ls-refs.o-LIB_OBJS+=mailinfo.o-LIB_OBJS+=mailmap.o-LIB_OBJS+=match-trees.o-LIB_OBJS+=mem-pool.o-LIB_OBJS+=merge-blobs.o-LIB_OBJS+=merge-ort.o-LIB_OBJS+=merge-ort-wrappers.o-LIB_OBJS+=merge-recursive.o-LIB_OBJS+=merge.o-LIB_OBJS+=mergesort.o-LIB_OBJS+=midx.o-LIB_OBJS+=name-hash.o-LIB_OBJS+=negotiator/default.o-LIB_OBJS+=negotiator/noop.o-LIB_OBJS+=negotiator/skipping.o-LIB_OBJS+=notes-cache.o-LIB_OBJS+=notes-merge.o-LIB_OBJS+=notes-utils.o-LIB_OBJS+=notes.o-LIB_OBJS+=object-file.o-LIB_OBJS+=object-name.o-LIB_OBJS+=object.o-LIB_OBJS+=oid-array.o-LIB_OBJS+=oidmap.o-LIB_OBJS+=oidset.o-LIB_OBJS+=oidtree.o-LIB_OBJS+=pack-bitmap-write.o-LIB_OBJS+=pack-bitmap.o-LIB_OBJS+=pack-check.o-LIB_OBJS+=pack-objects.o-LIB_OBJS+=pack-revindex.o-LIB_OBJS+=pack-write.o-LIB_OBJS+=packfile.o-LIB_OBJS+=pager.o-LIB_OBJS+=parallel-checkout.o-LIB_OBJS+=parse-options-cb.o-LIB_OBJS+=parse-options.o-LIB_OBJS+=patch-delta.o-LIB_OBJS+=patch-ids.o-LIB_OBJS+=path.o-LIB_OBJS+=pathspec.o-LIB_OBJS+=pkt-line.o-LIB_OBJS+=preload-index.o-LIB_OBJS+=pretty.o-LIB_OBJS+=prio-queue.o-LIB_OBJS+=progress.o-LIB_OBJS+=promisor-remote.o-LIB_OBJS+=prompt.o-LIB_OBJS+=protocol.o-LIB_OBJS+=protocol-caps.o-LIB_OBJS+=prune-packed.o-LIB_OBJS+=quote.o-LIB_OBJS+=range-diff.o-LIB_OBJS+=reachable.o-LIB_OBJS+=read-cache.o-LIB_OBJS+=rebase-interactive.o-LIB_OBJS+=rebase.o-LIB_OBJS+=ref-filter.o-LIB_OBJS+=reflog-walk.o-LIB_OBJS+=refs.o-LIB_OBJS+=refs/debug.o-LIB_OBJS+=refs/files-backend.o-LIB_OBJS+=refs/iterator.o-LIB_OBJS+=refs/packed-backend.o-LIB_OBJS+=refs/ref-cache.o-LIB_OBJS+=refspec.o-LIB_OBJS+=remote.o-LIB_OBJS+=replace-object.o-LIB_OBJS+=repo-settings.o-LIB_OBJS+=repository.o-LIB_OBJS+=rerere.o-LIB_OBJS+=reset.o-LIB_OBJS+=resolve-undo.o-LIB_OBJS+=revision.o-LIB_OBJS+=run-command.o-LIB_OBJS+=send-pack.o-LIB_OBJS+=sequencer.o-LIB_OBJS+=serve.o-LIB_OBJS+=server-info.o-LIB_OBJS+=setup.o-LIB_OBJS+=shallow.o-LIB_OBJS+=sideband.o-LIB_OBJS+=sigchain.o-LIB_OBJS+=sparse-index.o-LIB_OBJS+=split-index.o-LIB_OBJS+=stable-qsort.o-LIB_OBJS+=strbuf.o-LIB_OBJS+=streaming.o-LIB_OBJS+=string-list.o-LIB_OBJS+=strmap.o-LIB_OBJS+=strvec.o-LIB_OBJS+=sub-process.o-LIB_OBJS+=submodule-config.o-LIB_OBJS+=submodule.o-LIB_OBJS+=symlinks.o-LIB_OBJS+=tag.o-LIB_OBJS+=tempfile.o-LIB_OBJS+=thread-utils.o-LIB_OBJS+=tmp-objdir.o-LIB_OBJS+=trace.o-LIB_OBJS+=trace2.o-LIB_OBJS+=trace2/tr2_cfg.o-LIB_OBJS+=trace2/tr2_cmd_name.o-LIB_OBJS+=trace2/tr2_dst.o-LIB_OBJS+=trace2/tr2_sid.o-LIB_OBJS+=trace2/tr2_sysenv.o-LIB_OBJS+=trace2/tr2_tbuf.o-LIB_OBJS+=trace2/tr2_tgt_event.o-LIB_OBJS+=trace2/tr2_tgt_normal.o-LIB_OBJS+=trace2/tr2_tgt_perf.o-LIB_OBJS+=trace2/tr2_tls.o-LIB_OBJS+=trailer.o-LIB_OBJS+=transport-helper.o-LIB_OBJS+=transport.o-LIB_OBJS+=tree-diff.o-LIB_OBJS+=tree-walk.o-LIB_OBJS+=tree.o-LIB_OBJS+=unpack-trees.o-LIB_OBJS+=upload-pack.o-LIB_OBJS+=url.o-LIB_OBJS+=urlmatch.o-LIB_OBJS+=usage.o-LIB_OBJS+=userdiff.o-LIB_OBJS+=utf8.o-LIB_OBJS+=varint.o-LIB_OBJS+=version.o-LIB_OBJS+=versioncmp.o-LIB_OBJS+=walker.o-LIB_OBJS+=wildmatch.o-LIB_OBJS+=worktree.o-LIB_OBJS+=wrapper.o-LIB_OBJS+=write-or-die.o-LIB_OBJS+=ws.o-LIB_OBJS+=wt-status.o-LIB_OBJS+=xdiff-interface.o-LIB_OBJS+=zlib.o--BUILTIN_OBJS+=builtin/add.o-BUILTIN_OBJS+=builtin/am.o-BUILTIN_OBJS+=builtin/annotate.o-BUILTIN_OBJS+=builtin/apply.o-BUILTIN_OBJS+=builtin/archive.o-BUILTIN_OBJS+=builtin/bisect--helper.o-BUILTIN_OBJS+=builtin/blame.o-BUILTIN_OBJS+=builtin/branch.o-BUILTIN_OBJS+=builtin/bugreport.o-BUILTIN_OBJS+=builtin/bundle.o-BUILTIN_OBJS+=builtin/cat-file.o-BUILTIN_OBJS+=builtin/check-attr.o-BUILTIN_OBJS+=builtin/check-ignore.o-BUILTIN_OBJS+=builtin/check-mailmap.o-BUILTIN_OBJS+=builtin/check-ref-format.o-BUILTIN_OBJS+=builtin/checkout--worker.o-BUILTIN_OBJS+=builtin/checkout-index.o-BUILTIN_OBJS+=builtin/checkout.o-BUILTIN_OBJS+=builtin/clean.o-BUILTIN_OBJS+=builtin/clone.o-BUILTIN_OBJS+=builtin/column.o-BUILTIN_OBJS+=builtin/commit-graph.o-BUILTIN_OBJS+=builtin/commit-tree.o-BUILTIN_OBJS+=builtin/commit.o-BUILTIN_OBJS+=builtin/config.o-BUILTIN_OBJS+=builtin/count-objects.o-BUILTIN_OBJS+=builtin/credential-cache--daemon.o-BUILTIN_OBJS+=builtin/credential-cache.o-BUILTIN_OBJS+=builtin/credential-store.o-BUILTIN_OBJS+=builtin/credential.o-BUILTIN_OBJS+=builtin/describe.o-BUILTIN_OBJS+=builtin/diff-files.o-BUILTIN_OBJS+=builtin/diff-index.o-BUILTIN_OBJS+=builtin/diff-tree.o-BUILTIN_OBJS+=builtin/diff.o-BUILTIN_OBJS+=builtin/difftool.o-BUILTIN_OBJS+=builtin/env--helper.o-BUILTIN_OBJS+=builtin/fast-export.o-BUILTIN_OBJS+=builtin/fast-import.o-BUILTIN_OBJS+=builtin/fetch-pack.o-BUILTIN_OBJS+=builtin/fetch.o-BUILTIN_OBJS+=builtin/fmt-merge-msg.o-BUILTIN_OBJS+=builtin/for-each-ref.o-BUILTIN_OBJS+=builtin/for-each-repo.o-BUILTIN_OBJS+=builtin/fsck.o-BUILTIN_OBJS+=builtin/gc.o-BUILTIN_OBJS+=builtin/get-tar-commit-id.o-BUILTIN_OBJS+=builtin/grep.o-BUILTIN_OBJS+=builtin/hash-object.o-BUILTIN_OBJS+=builtin/help.o-BUILTIN_OBJS+=builtin/index-pack.o-BUILTIN_OBJS+=builtin/init-db.o-BUILTIN_OBJS+=builtin/interpret-trailers.o-BUILTIN_OBJS+=builtin/log.o-BUILTIN_OBJS+=builtin/ls-files.o-BUILTIN_OBJS+=builtin/ls-remote.o-BUILTIN_OBJS+=builtin/ls-tree.o-BUILTIN_OBJS+=builtin/mailinfo.o-BUILTIN_OBJS+=builtin/mailsplit.o-BUILTIN_OBJS+=builtin/merge-base.o-BUILTIN_OBJS+=builtin/merge-file.o-BUILTIN_OBJS+=builtin/merge-index.o-BUILTIN_OBJS+=builtin/merge-ours.o-BUILTIN_OBJS+=builtin/merge-recursive.o-BUILTIN_OBJS+=builtin/merge-tree.o-BUILTIN_OBJS+=builtin/merge.o-BUILTIN_OBJS+=builtin/mktag.o-BUILTIN_OBJS+=builtin/mktree.o-BUILTIN_OBJS+=builtin/multi-pack-index.o-BUILTIN_OBJS+=builtin/mv.o-BUILTIN_OBJS+=builtin/name-rev.o-BUILTIN_OBJS+=builtin/notes.o-BUILTIN_OBJS+=builtin/pack-objects.o-BUILTIN_OBJS+=builtin/pack-redundant.o-BUILTIN_OBJS+=builtin/pack-refs.o-BUILTIN_OBJS+=builtin/patch-id.o-BUILTIN_OBJS+=builtin/prune-packed.o-BUILTIN_OBJS+=builtin/prune.o-BUILTIN_OBJS+=builtin/pull.o-BUILTIN_OBJS+=builtin/push.o-BUILTIN_OBJS+=builtin/range-diff.o-BUILTIN_OBJS+=builtin/read-tree.o-BUILTIN_OBJS+=builtin/rebase.o-BUILTIN_OBJS+=builtin/receive-pack.o-BUILTIN_OBJS+=builtin/reflog.o-BUILTIN_OBJS+=builtin/remote-ext.o-BUILTIN_OBJS+=builtin/remote-fd.o-BUILTIN_OBJS+=builtin/remote.o-BUILTIN_OBJS+=builtin/repack.o-BUILTIN_OBJS+=builtin/replace.o-BUILTIN_OBJS+=builtin/rerere.o-BUILTIN_OBJS+=builtin/reset.o-BUILTIN_OBJS+=builtin/rev-list.o-BUILTIN_OBJS+=builtin/rev-parse.o-BUILTIN_OBJS+=builtin/revert.o-BUILTIN_OBJS+=builtin/rm.o-BUILTIN_OBJS+=builtin/send-pack.o-BUILTIN_OBJS+=builtin/shortlog.o-BUILTIN_OBJS+=builtin/show-branch.o-BUILTIN_OBJS+=builtin/show-index.o-BUILTIN_OBJS+=builtin/show-ref.o-BUILTIN_OBJS+=builtin/sparse-checkout.o-BUILTIN_OBJS+=builtin/stash.o-BUILTIN_OBJS+=builtin/stripspace.o-BUILTIN_OBJS+=builtin/submodule--helper.o-BUILTIN_OBJS+=builtin/symbolic-ref.o-BUILTIN_OBJS+=builtin/tag.o-BUILTIN_OBJS+=builtin/unpack-file.o-BUILTIN_OBJS+=builtin/unpack-objects.o-BUILTIN_OBJS+=builtin/update-index.o-BUILTIN_OBJS+=builtin/update-ref.o-BUILTIN_OBJS+=builtin/update-server-info.o-BUILTIN_OBJS+=builtin/upload-archive.o-BUILTIN_OBJS+=builtin/upload-pack.o-BUILTIN_OBJS+=builtin/var.o-BUILTIN_OBJS+=builtin/verify-commit.o-BUILTIN_OBJS+=builtin/verify-pack.o-BUILTIN_OBJS+=builtin/verify-tag.o-BUILTIN_OBJS+=builtin/worktree.o-BUILTIN_OBJS+=builtin/write-tree.o# THIRD_PARTY_SOURCES is a list of patterns compatible with the# $(filter) and $(filter-out) family of functions. They specify source
@@ -2427,17 +2049,12 @@ reconfigure config.mak.autogen: config.status.PHONY:reconfigure# This is a convenience target.endif-XDIFF_OBJS+=xdiff/xdiffi.o-XDIFF_OBJS+=xdiff/xemit.o-XDIFF_OBJS+=xdiff/xhistogram.o-XDIFF_OBJS+=xdiff/xmerge.o-XDIFF_OBJS+=xdiff/xpatience.o-XDIFF_OBJS+=xdiff/xprepare.o-XDIFF_OBJS+=xdiff/xutils.o+XDIFF_SRC+=$(wildcardxdiff/*.c)+XDIFF_OBJS+=$(XDIFF_SRC:.c=.o).PHONY:xdiff-objsxdiff-objs:$(XDIFF_OBJS)-TEST_OBJS:=$(patsubst%$X,%.o,$(TEST_PROGRAMS))$(patsubst%,t/helper/%,$(TEST_BUILTINS_OBJS))+TEST_OBJS:=$(patsubst%$X,%.o,$(TEST_PROGRAMS))$(TEST_BUILTINS_OBJS).PHONY:test-objstest-objs:$(TEST_OBJS)
Another way to write this would be:
LIB_OBJS += $(patsubst %.c,%.o,$(wildcard $(addsuffix /*.c,$(LIB_OBJS_DIRS)))
I don't know that there's any reason to choose one over the other. I
don't think there's any real performance difference although one could
imagine this version to be VERY SLIGHTLY faster. Also this one is a
little more "Lisp-ish"... that might be a pro or a con depending :).
Just kibitzing while waiting for dinner to arrive...
From: Jeff King <hidden> Date: 2021-10-31 08:29:54
On Sun, Oct 31, 2021 at 12:32:26AM +0200, Ævar Arnfjörð Bjarmason wrote:
Remove the hardcoded lists of objects in favor of using
$(wildcard). This means that every time a built-in, test tool etc. is
added we won't need to patch the top-level Makefile, except for the
few remaining cases where the asset in question would make it onto one
of our list of exceptions.
Ever since 81b50f3ce40 (Move 'builtin-*' into a 'builtin/'
subdirectory, 2010-02-22) this has been relatively easy to do (and
even before that we could glob builtin-*.c). This pattern of
exhaustively enumerating files was then carried forward for
e.g. TEST_BUILTINS_OBJS in efd71f8913a (t/helper: add an empty
test-tool program, 2018-03-24).
One reason not to do this is that now a new *.c file at the top-level
will be immediately picked up, so if a new *.c file is being worked on
"make" will error if it doesn't compile, whereas before that file
would need to be explicitly listed in the Makefile. I think than small
trade-off is worth it.
A more general way of thinking about this is that we are switching from
"ignore source files by default" to "stick source files into LIB_OBJS by
default". So it's helpful if you were going to stick that file into
LIB_OBJS, but harmful otherwise.
Your "new *.c file" example is one case, because it wouldn't have been
added _yet_. And I agree it's probably not that big a deal in practice.
The other cases are ones similar to what you had to exclude from
LIB_OBJS manually here:
So if I wanted to add a new external program source but forgot to put it
into PROGRAM_OBJS, the default would now be to pick it up in LIB_OBJS.
That's weird and definitely not what you'd want, but presumably you'd
figure it out pretty quickly because we wouldn't have built the command
you expected to exist.
Likewise, there's an interesting tradeoff here for non-program object
files. The current Makefile does not need to mention unix-socket.o
outside of the NO_UNIX_SOCKETS ifdef block, because that's where we
stick it in LIB_OBJS. After your patch, it gets mentioned twice: in that
same spot, but also as an exception to the LIB_OBJS rule (via the
ALL_COMPAT_OBJS variable above).
So we're trading off having to remember to do one thing (add stuff to
LIB_OBJS) for another (add stuff to the exception list). Now one of
those happens a lot more than the other, which is why you get such a
nice diffstat. So it might be worth the tradeoff.
I don't have a very strong opinion either way on this. I felt like we'd
discussed this direction before, and came up with this thread from the
archive:
https://lore.kernel.org/git/20110222155637.GC27178@sigill.intra.peff.net/
There it was coupled with suggestions to actually change the file
layout. That could make some of those exceptions go away (say, if all of
LIB_OBJS was in "lib/"), but it's a bigger change overall. So I offer it
here mostly for historical context / interest.
I didn't see anything obviously wrong in the patch, but two comments:
- De-indent an "ifndef" block, we don't usually indent their
contents.
Quite a lot of existing conditional blocks are indented, but I think for
conditional inclusions of one entry in a larger list (where the rest of
the list isn't indented), this makes sense. And that's what you changed
here.
+# LIB_OBJS: compat/* objects that live at the top-level
+ALL_COMPAT_OBJS += unix-socket.o
+ALL_COMPAT_OBJS += unix-stream-server.o
+ALL_COMPAT_OBJS += sha1dc_git.o
I think "compat" is a misnomer here. For one thing, they're by
definition not "compat/*" objects, because they're not in that
directory. ;) But more importantly, the interesting thing about them is
not that they're compatibility layers, but that they're part of a
conditional compilation. I.e., we might or might not want them, which
will be determined elsewhere in the Makefile, so they must not be part
of the base LIB_OBJS set.
Probably CONDITIONAL_OBJS or something might be more descriptive. That
_could_ be used to include things like CURL_OBJS, but there's probably
value in keeping those in their own list anyway.
Likewise, they could go into a conditional-src/ directory (or some
less-horrible name) to keep them distinct without needing an explicit
list in the Makefile. That's sort of the flip-side of putting all the
other LIB_OBJS ones into lib/.
-Peff
On Sun, Oct 31, 2021 at 12:32:26AM +0200, Ævar Arnfjörð Bjarmason wrote:
[...]
quoted
+# LIB_OBJS: compat/* objects that live at the top-level
+ALL_COMPAT_OBJS += unix-socket.o
+ALL_COMPAT_OBJS += unix-stream-server.o
+ALL_COMPAT_OBJS += sha1dc_git.o
I think "compat" is a misnomer here. For one thing, they're by
definition not "compat/*" objects, because they're not in that
directory. ;) But more importantly, the interesting thing about them is
not that they're compatibility layers, but that they're part of a
conditional compilation. I.e., we might or might not want them, which
will be determined elsewhere in the Makefile, so they must not be part
of the base LIB_OBJS set.
Probably CONDITIONAL_OBJS or something might be more descriptive. That
_could_ be used to include things like CURL_OBJS, but there's probably
value in keeping those in their own list anyway.
Good point, will rename them.
Likewise, they could go into a conditional-src/ directory (or some
less-horrible name) to keep them distinct without needing an explicit
list in the Makefile. That's sort of the flip-side of putting all the
other LIB_OBJS ones into lib/.
The goal here was just to get us rid of tiresome merge conflicts when
two things are added to adjacent part of these lists going forward,
rather than some source-tree reorganization. I didn't search around and
didn't find that 2011-era thread.
I think overall just maintaining the list of the few exceptions is
better than any sort of general mass-move of these files.
Even if we carefully trickle those in at a rate that doesn't conflict
with anything in-flight, the end result will be that e.g.:
git log -- lib/grep.c
Will stop at that rename commit, similar to builtin/log.c, unless you
specify --follow etc. Just that doesn't make it worth it to me. Likewise
sha1_file.c to sha1-file.c to object-file.c, which is a case I run into
every time I get a "git log" pathspec glob wrong.
Also.
I didn't notice before submitting this but this patch breaks the
vs-build job, because the cmake build in "contrib" is screen-scraping
the Makefile[1].
What's the status of that code? It's rather tiresome to need to patch
two independent and incompatible build systems every time there's some
structural change in the Makefile.
I hadn't looked in any detail at that recipe before, but it the vs-build
job has a hard dependency on GNU make anyway, since we use it for "make
artifacts-tar".
So whatever cmake special-sauce is happening there I don't see why
vs-build couldn't call out "make" for most of the work it's doing, isn't
it just some replacement for what the "vcxproj" target in
config.mak.uname used to do?
1. https://github.com/avar/git/runs/4057171803?check_suite_focus=true
This series replaces the hardcoded lists *.o files in the top-level
Makefile with a reliance on globbing, e.g. built-ins are $(wildcard
builtin/*.c now, instead of an exhausitive list of every built-in in
the tree.
This makes future development easier as we won't be running into merge
conflicts with these lists, see 3/3 for some more disucssion.
As noted in the v1 discussion the v1 had changes that broke the
"cmake" integration. In going back and reading the discussions around
it it seems we've ended up with a state for that "contrib" component
that was never the intent, and which we tried to explicitly avoid when
integrating it.
There are more details in the updated commit message, but this v2 gets
it working with passing CI, and in such a way as to reduce future
maintenance burden related to that component.
Ævar Arnfjörð Bjarmason (3):
Makefile: rename $(SCRIPT_LIB) to $(SCRIPT_LIB_GEN)
Makefile: add a utility to dump variables
Makefile: replace most hardcoded object lists with $(wildcard)
Makefile | 510 ++++------------------------
contrib/buildsystems/CMakeLists.txt | 53 ++-
2 files changed, 91 insertions(+), 472 deletions(-)
Range-diff against v1:
-: ----------- > 1: 0b23b8395ec Makefile: rename $(SCRIPT_LIB) to $(SCRIPT_LIB_GEN)
-: ----------- > 2: 97738b056cf Makefile: add a utility to dump variables
1: bbacbed5c95 ! 3: cd62d8f92d1 Makefile: replace most hardcoded object lists with $(wildcard)
@@ Commit message
would need to be explicitly listed in the Makefile. I think than small
trade-off is worth it.
+ We could make this simpler still for the Makefile by moving
+ "unix-socket.c" etc. to e.g. a "conditional-src/" directory, likewise
+ for $(PROGRAM_OBJS) to e.g. "programs/". If we did that we would not
+ need the "$(filter-out)" for LIB_OBJS. I don't think that's worth it,
+ e.g. due to "git log -- <path>" on the files now needing a "--follow".
+
There's a few small "while we're at it" changes here, since I'm
touching the code in question:
@@ Commit message
- De-indent an "ifndef" block, we don't usually indent their
contents.
+ On the CMake changes here:
+
+ - When CMake support was introduced in was introduced
+ 061c2240b1b (Introduce CMake support for configuring Git, 2020-06-12)
+ there was a discussion about the maintenance burden of maintaining the
+ top-level Makefile in parallel with CMakeLists.txt[1] where reviewers
+ were assured that doing so would simply be a matter of adding something
+ to a list in the CMake recipe.
+
+ Between change and some recent changes of mine where the "vs-build"
+ job failed to a divergence between the Makefile and CMakeList.txt I
+ can confidently say that that doesn't at all match reality. Even
+ seemingly trivial changes to the Makefile like this one are forcing
+ us to do a deep-dive into CMake internals to make forward progress
+ with our main build system.
+
+ - The promised "We can add a (continue-on-error) to vs-build job to
+ make this process less of a hindrance." in [2] never materialized.
+ Since 4c2c38e800f (ci: modification of main.yml to use cmake for
+ vs-build job, 2020-06-26) got a hard dependency on CMake as far as
+ getting the CI to pass goes.
+
+ - The "vs-build" CI doesn't actually require that there be no GNU make
+ usage in the job, as it itself has a hard dependency on running a
+ "make -n artifacts-tar" command. So as far as any vs-specific special-sauce
+ goes we don't need a GNU-make free build system for vs-build.
+
+ - The stated goal in 061c2240b1b of avoiding a GNU make dependency
+ for developer because it requires an SDK that "occupies around two
+ gigabytes" and "three quarters of a gigabyte worth of Git objects"
+ hardly seems worthwhile trade-off given the above. Disk space is cheap,
+ developer time required to maintain two parallel build systems isn't.
+
+ My attempt to amend/revert 4c2c38e800f to have it use the
+ pre-4c2c38e800f "make" invocation as a fallback failed, partially
+ because I don't have a Windows development environment, so any attempt
+ to change it is a painfully slow round-trip to GitHub CI.
+
+ Let's instead have CMake call out to the Makefile asking it what the
+ definition of various variables lists is, rather than being forced to
+ maintain those lists in a way that CMake can parse with regexes (which
+ precludes anything but a giant hardcoded list).
+
+ I could familiarize myself enough with CMake to do this in some
+ CMake-native way, but that would take "just as long as adding it to
+ the Makefile"[2] (I think that took me <5 minutes, but I'm several
+ hours into fighting with CMake)
+
+ So I consider this both a bugfix to the stated aims of this CMake
+ integration, and a better way forward for having an alternate build
+ system. I.e. If someone really does care about a having a
+ GNU-make-less dependency for the "vs-build" I think this change offers
+ a much better way forward for that.
+
+ Once we invoke the Makefile to spew out e.g. its idea of "LIB_OBJS",
+ it's going to be trivial to do that via some wrapper script that lives
+ in "contrib/buildsystems". Such a script would either invoke "make
+ print-{var,list}-%", or alternatively use an in-tree committed text
+ file with the last known result of such a "make print-{var,list}-%"
+ run.
+
+ 1. https://lore.kernel.org/git/xmqq8sikblv2.fsf@gitster.c.googlers.com
+ 2. https://lore.kernel.org/git/CAKiG+9Xtof8Hj3npsS-M0SnT_dcjtHjP_+avWB4oOHkaMdnSbw@mail.gmail.com/
+
Signed-off-by: Ævar Arnfjörð Bjarmason [off-list ref]
## Makefile ##
@@ Makefile: TEST_OBJS =
+XDIFF_SRC =
+
+## Guard against env: objects
-+ALL_COMPAT_OBJS =
++CONDITIONAL_OBJS =
+CURL_OBJS =
+LIB_OBJS_DIRS =
+
- # Having this variable in your environment would break pipelines because
- # you cause "cd" to echo its destination to stdout. It can also take
- # scripts to unexpected places. If you like CDPATH, define it for your
+ # Utility to dump whatever variables are defined here
+ print-var-%:
+ @echo $($*)
@@ Makefile: X =
PROGRAMS += $(patsubst %.o,git-%$X,$(PROGRAM_OBJS))
@@ Makefile: LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!
-LIB_OBJS += commit-reach.o
-LIB_OBJS += commit.o
+# LIB_OBJS: compat/* objects that live at the top-level
-+ALL_COMPAT_OBJS += unix-socket.o
-+ALL_COMPAT_OBJS += unix-stream-server.o
-+ALL_COMPAT_OBJS += sha1dc_git.o
++CONDITIONAL_OBJS += unix-socket.o
++CONDITIONAL_OBJS += unix-stream-server.o
++CONDITIONAL_OBJS += sha1dc_git.o
+
+# LIB_OBJS: Mostly glob *.c at the top-level, with some exlusions
+LIB_OBJS += $(filter-out \
-+ $(ALL_COMPAT_OBJS) \
++ $(CONDITIONAL_OBJS) \
+ git.o common-main.o $(PROGRAM_OBJS) \
+ $(FUZZ_OBJS) $(CURL_OBJS),\
+ $(patsubst %.c,%.o,$(wildcard *.c)))
@@ Makefile: LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!
+LIB_OBJS_DIRS += negotiator
+LIB_OBJS_DIRS += refs
+LIB_OBJS_DIRS += trace2
-+LIB_OBJS += $(patsubst %.c,%.o,$(foreach dir,$(LIB_OBJS_DIRS),$(wildcard $(dir)/*.c)))
++LIB_OBJS += $(patsubst %.c,%.o,$(wildcard $(addsuffix /*.c,$(LIB_OBJS_DIRS))))
+
+# LIB_OBJS: unconditional compat/* objects
LIB_OBJS += compat/obstack.o
@@ Makefile: LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!
# THIRD_PARTY_SOURCES is a list of patterns compatible with the
# $(filter) and $(filter-out) family of functions. They specify source
+@@ Makefile: endif
+ LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
+
+ BASIC_CFLAGS += $(COMPAT_CFLAGS)
++LIB_OBJS_NO_COMPAT := $(LIB_OBJS)
+ LIB_OBJS += $(COMPAT_OBJS)
+
+ # Quote for C
@@ Makefile: reconfigure config.mak.autogen: config.status
.PHONY: reconfigure # This is a convenience target.
endif
@@ Makefile: perf: all
t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
+
+ ## contrib/buildsystems/CMakeLists.txt ##
+@@ contrib/buildsystems/CMakeLists.txt: project(git
+ #TODO Add pcre support
+
+ #macros for parsing the Makefile for sources and scripts
+-macro(parse_makefile_for_sources list_var regex)
+- file(STRINGS ${CMAKE_SOURCE_DIR}/Makefile ${list_var} REGEX "^${regex} \\+=(.*)")
+- string(REPLACE "${regex} +=" "" ${list_var} ${${list_var}})
+- string(REPLACE "$(COMPAT_OBJS)" "" ${list_var} ${${list_var}}) #remove "$(COMPAT_OBJS)" This is only for libgit.
+- string(STRIP ${${list_var}} ${list_var}) #remove trailing/leading whitespaces
+- string(REPLACE ".o" ".c;" ${list_var} ${${list_var}}) #change .o to .c, ; is for converting the string into a list
+- list(TRANSFORM ${list_var} STRIP) #remove trailing/leading whitespaces for each element in list
+- list(REMOVE_ITEM ${list_var} "") #remove empty list elements
+-endmacro()
+-
+-macro(parse_makefile_for_scripts list_var regex lang)
+- file(STRINGS ${CMAKE_SOURCE_DIR}/Makefile ${list_var} REGEX "^${regex} \\+=(.*)")
+- string(REPLACE "${regex} +=" "" ${list_var} ${${list_var}})
+- string(STRIP ${${list_var}} ${list_var}) #remove trailing/leading whitespaces
+- string(REPLACE " " ";" ${list_var} ${${list_var}}) #convert string to a list
+- if(NOT ${lang}) #exclude for SCRIPT_LIB
+- list(TRANSFORM ${list_var} REPLACE "${lang}" "") #do the replacement
+- endif()
+-endmacro()
+-
+-macro(parse_makefile_for_executables list_var regex)
+- file(STRINGS ${CMAKE_SOURCE_DIR}/Makefile ${list_var} REGEX "^${regex} \\+= git-(.*)")
+- string(REPLACE "${regex} +=" "" ${list_var} ${${list_var}})
++macro(ask_makefile_for_list list_var var_name)
++ execute_process(COMMAND make print-list-${var_name}
++ WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
++ OUTPUT_VARIABLE ${list_var})
++ string(REGEX REPLACE "\\.o\n" ".c\n" ${list_var} ${${list_var}}) #change .o to .c
+ string(STRIP ${${list_var}} ${list_var}) #remove trailing/leading whitespaces
+- string(REPLACE "git-" "" ${list_var} ${${list_var}}) #strip `git-` prefix
+- string(REPLACE "\$X" ";" ${list_var} ${${list_var}}) #strip $X, ; is for converting the string into a list
++ ## Parse the Makefile print-list-% format
++ string(REGEX REPLACE "${var_name} =\n" "" ${list_var} ${${list_var}})
++ string(REGEX REPLACE "${var_name} \\+?= ([^\n]+)" "\\1;" ${list_var} ${${list_var}})
+ list(TRANSFORM ${list_var} STRIP) #remove trailing/leading whitespaces for each element in list
+ list(REMOVE_ITEM ${list_var} "") #remove empty list elements
+ endmacro()
+@@ contrib/buildsystems/CMakeLists.txt: include_directories(${CMAKE_BINARY_DIR})
+
+ #build
+ #libgit
+-parse_makefile_for_sources(libgit_SOURCES "LIB_OBJS")
++ask_makefile_for_list(libgit_SOURCES "LIB_OBJS_NO_COMPAT")
+
+ list(TRANSFORM libgit_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
+ list(TRANSFORM compat_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
+ add_library(libgit ${libgit_SOURCES} ${compat_SOURCES})
+
+ #libxdiff
+-parse_makefile_for_sources(libxdiff_SOURCES "XDIFF_OBJS")
++ask_makefile_for_list(libxdiff_SOURCES "XDIFF_OBJS")
+
+ list(TRANSFORM libxdiff_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
+ add_library(xdiff STATIC ${libxdiff_SOURCES})
+@@ contrib/buildsystems/CMakeLists.txt: elseif(UNIX)
+ endif()
+
+ #git
+-parse_makefile_for_sources(git_SOURCES "BUILTIN_OBJS")
++ask_makefile_for_list(git_SOURCES "BUILTIN_OBJS")
+
+ list(TRANSFORM git_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
+ add_executable(git ${CMAKE_SOURCE_DIR}/git.c ${git_SOURCES})
+@@ contrib/buildsystems/CMakeLists.txt: if(CURL_FOUND)
+ endif()
+ endif()
+
+-parse_makefile_for_executables(git_builtin_extra "BUILT_INS")
++ask_makefile_for_list(git_builtin_extra "BUILT_INS")
++list(TRANSFORM git_builtin_extra REPLACE "^git-(.*)" "\\1") #strip `git-` prefix
++list(TRANSFORM git_builtin_extra REPLACE "\.exe$" "") #strip $X
+
+ option(SKIP_DASHED_BUILT_INS "Skip hardlinking the dashed versions of the built-ins")
+
+@@ contrib/buildsystems/CMakeLists.txt: set(GITWEBDIR ${FALLBACK_RUNTIME_PREFIX}/share/locale)
+ set(INSTLIBDIR ${FALLBACK_RUNTIME_PREFIX}/share/perl5)
+
+ #shell scripts
+-parse_makefile_for_scripts(git_sh_scripts "SCRIPT_SH" ".sh")
+-parse_makefile_for_scripts(git_shlib_scripts "SCRIPT_LIB" "")
++ask_makefile_for_list(git_sh_scripts "SCRIPT_SH_GEN")
++ask_makefile_for_list(git_shlib_scripts "SCRIPT_LIB_GEN")
+ set(git_shell_scripts
+ ${git_sh_scripts} ${git_shlib_scripts} git-instaweb)
+
+@@ contrib/buildsystems/CMakeLists.txt: foreach(script ${git_shell_scripts})
+ endforeach()
+
+ #perl scripts
+-parse_makefile_for_scripts(git_perl_scripts "SCRIPT_PERL" ".perl")
++ask_makefile_for_list(git_perl_scripts "SCRIPT_PERL_GEN")
+
+ #create perl header
+ file(STRINGS ${CMAKE_SOURCE_DIR}/perl/header_templates/fixed_prefix.template.pl perl_header )
+@@ contrib/buildsystems/CMakeLists.txt: add_executable(test-fake-ssh ${CMAKE_SOURCE_DIR}/t/helper/test-fake-ssh.c)
+ target_link_libraries(test-fake-ssh common-main)
+
+ #test-tool
+-parse_makefile_for_sources(test-tool_SOURCES "TEST_BUILTINS_OBJS")
++ask_makefile_for_list(test-tool_SOURCES "TEST_BUILTINS_OBJS")
+
+-list(TRANSFORM test-tool_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/t/helper/")
++list(TRANSFORM test-tool_SOURCES PREPEND "${CMAKE_SOURCE_DIR}/")
+ add_executable(test-tool ${CMAKE_SOURCE_DIR}/t/helper/test-tool.c ${test-tool_SOURCES})
+ target_link_libraries(test-tool common-main)
+
--
2.33.1.1570.g069344fdd45
Make the $(SCRIPT_LIB) variable like the rest of its siblings, where
we have an extension-less $(*_GEN) variable, but the main one has the
full file name. This will be used in subsequent commits to emit the
filenames in $(SCRIPT_LIB), $(SCRIPT_PERL) etc.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
Add handy "print-var-%" and "print-list-%" targets, these can both be
used for ad-hoc debugging, and to integrate the Makefile into some
other build system which needs to extract information from it.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 8 ++++++++
1 file changed, 8 insertions(+)
@@ -590,6 +590,14 @@ TEST_OBJS =TEST_PROGRAMS_NEED_X=THIRD_PARTY_SOURCES=+# Utility to dump whatever variables are defined here+print-var-%:+@echo$($*)++print-list-%:+@echo$*=+@forvin$($*);doecho$*+=$$v;done+# Having this variable in your environment would break pipelines because# you cause "cd" to echo its destination to stdout. It can also take# scripts to unexpected places. If you like CDPATH, define it for your
Remove the hardcoded lists of objects in favor of using
$(wildcard). This means that every time a built-in, test tool etc. is
added we won't need to patch the top-level Makefile, except for the
few remaining cases where the asset in question would make it onto one
of our list of exceptions.
Ever since 81b50f3ce40 (Move 'builtin-*' into a 'builtin/'
subdirectory, 2010-02-22) this has been relatively easy to do (and
even before that we could glob builtin-*.c). This pattern of
exhaustively enumerating files was then carried forward for
e.g. TEST_BUILTINS_OBJS in efd71f8913a (t/helper: add an empty
test-tool program, 2018-03-24).
One reason not to do this is that now a new *.c file at the top-level
will be immediately picked up, so if a new *.c file is being worked on
"make" will error if it doesn't compile, whereas before that file
would need to be explicitly listed in the Makefile. I think than small
trade-off is worth it.
We could make this simpler still for the Makefile by moving
"unix-socket.c" etc. to e.g. a "conditional-src/" directory, likewise
for $(PROGRAM_OBJS) to e.g. "programs/". If we did that we would not
need the "$(filter-out)" for LIB_OBJS. I don't think that's worth it,
e.g. due to "git log -- <path>" on the files now needing a "--follow".
There's a few small "while we're at it" changes here, since I'm
touching the code in question:
- Start splitting up the the "Guard against the environment" section
at the top, but don't move anything that exists there out to avoid
merge conflicts
- The $(TEST_BUILTINS_OBJS) variable was needlessly complex, because
it didn't have the full paths we'd pathsubst it back & forth.
- Introduce *_SRC in addition to *_OBJ for the variable I'm
touching. Eventually we'll want to do this for all the *.o files,
i.e. make the *.c list a source of truth for *.o, which means we can
e.g. use that exhaustive list for "make TAGS".
- Add a missing "curl-objs" target. See 029bac01a87 (Makefile: add
{program,xdiff,test,git,fuzz}-objs & objects targets, 2021-02-23)
for the commit that added the rest.
- De-indent an "ifndef" block, we don't usually indent their
contents.
On the CMake changes here:
- When CMake support was introduced in was introduced
061c2240b1b (Introduce CMake support for configuring Git, 2020-06-12)
there was a discussion about the maintenance burden of maintaining the
top-level Makefile in parallel with CMakeLists.txt[1] where reviewers
were assured that doing so would simply be a matter of adding something
to a list in the CMake recipe.
Between change and some recent changes of mine where the "vs-build"
job failed to a divergence between the Makefile and CMakeList.txt I
can confidently say that that doesn't at all match reality. Even
seemingly trivial changes to the Makefile like this one are forcing
us to do a deep-dive into CMake internals to make forward progress
with our main build system.
- The promised "We can add a (continue-on-error) to vs-build job to
make this process less of a hindrance." in [2] never materialized.
Since 4c2c38e800f (ci: modification of main.yml to use cmake for
vs-build job, 2020-06-26) got a hard dependency on CMake as far as
getting the CI to pass goes.
- The "vs-build" CI doesn't actually require that there be no GNU make
usage in the job, as it itself has a hard dependency on running a
"make -n artifacts-tar" command. So as far as any vs-specific special-sauce
goes we don't need a GNU-make free build system for vs-build.
- The stated goal in 061c2240b1b of avoiding a GNU make dependency
for developer because it requires an SDK that "occupies around two
gigabytes" and "three quarters of a gigabyte worth of Git objects"
hardly seems worthwhile trade-off given the above. Disk space is cheap,
developer time required to maintain two parallel build systems isn't.
My attempt to amend/revert 4c2c38e800f to have it use the
pre-4c2c38e800f "make" invocation as a fallback failed, partially
because I don't have a Windows development environment, so any attempt
to change it is a painfully slow round-trip to GitHub CI.
Let's instead have CMake call out to the Makefile asking it what the
definition of various variables lists is, rather than being forced to
maintain those lists in a way that CMake can parse with regexes (which
precludes anything but a giant hardcoded list).
I could familiarize myself enough with CMake to do this in some
CMake-native way, but that would take "just as long as adding it to
the Makefile"[2] (I think that took me <5 minutes, but I'm several
hours into fighting with CMake)
So I consider this both a bugfix to the stated aims of this CMake
integration, and a better way forward for having an alternate build
system. I.e. If someone really does care about a having a
GNU-make-less dependency for the "vs-build" I think this change offers
a much better way forward for that.
Once we invoke the Makefile to spew out e.g. its idea of "LIB_OBJS",
it's going to be trivial to do that via some wrapper script that lives
in "contrib/buildsystems". Such a script would either invoke "make
print-{var,list}-%", or alternatively use an in-tree committed text
file with the last known result of such a "make print-{var,list}-%"
run.
1. https://lore.kernel.org/git/xmqq8sikblv2.fsf@gitster.c.googlers.com
2. https://lore.kernel.org/git/CAKiG+9Xtof8Hj3npsS-M0SnT_dcjtHjP_+avWB4oOHkaMdnSbw@mail.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 485 ++++------------------------
contrib/buildsystems/CMakeLists.txt | 53 ++-
2 files changed, 74 insertions(+), 464 deletions(-)
@@ -590,6 +590,19 @@ TEST_OBJS =TEST_PROGRAMS_NEED_X=THIRD_PARTY_SOURCES=+## Guard against env: programs+TEST_PROGRAMS=++## Guard against env: sources+CURL_SRC=+TEST_PROGRAMS_NEED_X_SRC=+XDIFF_SRC=++## Guard against env: objects+CONDITIONAL_OBJS=+CURL_OBJS=+LIB_OBJS_DIRS=+# Utility to dump whatever variables are defined hereprint-var-%:@echo$($*)
@@ -697,87 +710,23 @@ X =PROGRAMS+=$(patsubst%.o,git-%$X,$(PROGRAM_OBJS))-TEST_BUILTINS_OBJS+=test-advise.o-TEST_BUILTINS_OBJS+=test-bitmap.o-TEST_BUILTINS_OBJS+=test-bloom.o-TEST_BUILTINS_OBJS+=test-chmtime.o-TEST_BUILTINS_OBJS+=test-config.o-TEST_BUILTINS_OBJS+=test-crontab.o-TEST_BUILTINS_OBJS+=test-ctype.o-TEST_BUILTINS_OBJS+=test-date.o-TEST_BUILTINS_OBJS+=test-delta.o-TEST_BUILTINS_OBJS+=test-dir-iterator.o-TEST_BUILTINS_OBJS+=test-drop-caches.o-TEST_BUILTINS_OBJS+=test-dump-cache-tree.o-TEST_BUILTINS_OBJS+=test-dump-fsmonitor.o-TEST_BUILTINS_OBJS+=test-dump-split-index.o-TEST_BUILTINS_OBJS+=test-dump-untracked-cache.o-TEST_BUILTINS_OBJS+=test-example-decorate.o-TEST_BUILTINS_OBJS+=test-fast-rebase.o-TEST_BUILTINS_OBJS+=test-genrandom.o-TEST_BUILTINS_OBJS+=test-genzeros.o-TEST_BUILTINS_OBJS+=test-getcwd.o-TEST_BUILTINS_OBJS+=test-hash-speed.o-TEST_BUILTINS_OBJS+=test-hash.o-TEST_BUILTINS_OBJS+=test-hashmap.o-TEST_BUILTINS_OBJS+=test-index-version.o-TEST_BUILTINS_OBJS+=test-json-writer.o-TEST_BUILTINS_OBJS+=test-lazy-init-name-hash.o-TEST_BUILTINS_OBJS+=test-match-trees.o-TEST_BUILTINS_OBJS+=test-mergesort.o-TEST_BUILTINS_OBJS+=test-mktemp.o-TEST_BUILTINS_OBJS+=test-oid-array.o-TEST_BUILTINS_OBJS+=test-oidmap.o-TEST_BUILTINS_OBJS+=test-oidtree.o-TEST_BUILTINS_OBJS+=test-online-cpus.o-TEST_BUILTINS_OBJS+=test-parse-options.o-TEST_BUILTINS_OBJS+=test-parse-pathspec-file.o-TEST_BUILTINS_OBJS+=test-partial-clone.o-TEST_BUILTINS_OBJS+=test-path-utils.o-TEST_BUILTINS_OBJS+=test-pcre2-config.o-TEST_BUILTINS_OBJS+=test-pkt-line.o-TEST_BUILTINS_OBJS+=test-prio-queue.o-TEST_BUILTINS_OBJS+=test-proc-receive.o-TEST_BUILTINS_OBJS+=test-progress.o-TEST_BUILTINS_OBJS+=test-reach.o-TEST_BUILTINS_OBJS+=test-read-cache.o-TEST_BUILTINS_OBJS+=test-read-graph.o-TEST_BUILTINS_OBJS+=test-read-midx.o-TEST_BUILTINS_OBJS+=test-ref-store.o-TEST_BUILTINS_OBJS+=test-regex.o-TEST_BUILTINS_OBJS+=test-repository.o-TEST_BUILTINS_OBJS+=test-revision-walking.o-TEST_BUILTINS_OBJS+=test-run-command.o-TEST_BUILTINS_OBJS+=test-scrap-cache-tree.o-TEST_BUILTINS_OBJS+=test-serve-v2.o-TEST_BUILTINS_OBJS+=test-sha1.o-TEST_BUILTINS_OBJS+=test-sha256.o-TEST_BUILTINS_OBJS+=test-sigchain.o-TEST_BUILTINS_OBJS+=test-simple-ipc.o-TEST_BUILTINS_OBJS+=test-strcmp-offset.o-TEST_BUILTINS_OBJS+=test-string-list.o-TEST_BUILTINS_OBJS+=test-submodule-config.o-TEST_BUILTINS_OBJS+=test-submodule-nested-repo-config.o-TEST_BUILTINS_OBJS+=test-subprocess.o-TEST_BUILTINS_OBJS+=test-trace2.o-TEST_BUILTINS_OBJS+=test-urlmatch-normalization.o-TEST_BUILTINS_OBJS+=test-userdiff.o-TEST_BUILTINS_OBJS+=test-wildmatch.o-TEST_BUILTINS_OBJS+=test-windows-named-pipe.o-TEST_BUILTINS_OBJS+=test-write-cache.o-TEST_BUILTINS_OBJS+=test-xml-encode.o-# Do not add more tests here unless they have extra dependencies. Add# them in TEST_BUILTINS_OBJS above.TEST_PROGRAMS_NEED_X+=test-fake-sshTEST_PROGRAMS_NEED_X+=test-tool-TEST_PROGRAMS=$(patsubst%,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))+TEST_PROGRAMS_NEED_X_SRC+=$(TEST_PROGRAMS_NEED_X:%=t/helper/%.c)+TEST_PROGRAMS+=$(TEST_PROGRAMS_NEED_X_SRC:%.c=%$X)+TEST_BUILTINS_SRC+=$(filter-out$(TEST_PROGRAMS_NEED_X_SRC),$(wildcardt/helper/*.c))+TEST_BUILTINS_OBJS+=$(TEST_BUILTINS_SRC:%.c=%.o)-# List built-in command $C whose implementation cmd_$C() is not in-# builtin/$C.o but is linked in as part of some other command.+# List built-in command $C whose implementation cmd_$C() is in+# builtin/$C.o+BUILTIN_OBJS=$(patsubst%.c,%.o,$(wildcardbuiltin/*.c))BUILT_INS+=$(patsubstbuiltin/%.o,git-%$X,$(BUILTIN_OBJS))+# List built-in command $C whose implementation cmd_$C() is not in+# builtin/$C.o but is linked in as part of some other command.BUILT_INS+=git-cherry$XBUILT_INS+=git-cherry-pick$XBUILT_INS+=git-format-patch$X
@@ -837,355 +786,28 @@ LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentat-nameDocumentation-prune-o\-name'*.h'-print)))-LIB_OBJS+=abspath.o-LIB_OBJS+=add-interactive.o-LIB_OBJS+=add-patch.o-LIB_OBJS+=advice.o-LIB_OBJS+=alias.o-LIB_OBJS+=alloc.o-LIB_OBJS+=apply.o-LIB_OBJS+=archive-tar.o-LIB_OBJS+=archive-zip.o-LIB_OBJS+=archive.o-LIB_OBJS+=attr.o-LIB_OBJS+=base85.o-LIB_OBJS+=bisect.o-LIB_OBJS+=blame.o-LIB_OBJS+=blob.o-LIB_OBJS+=bloom.o-LIB_OBJS+=branch.o-LIB_OBJS+=bulk-checkin.o-LIB_OBJS+=bundle.o-LIB_OBJS+=cache-tree.o-LIB_OBJS+=cbtree.o-LIB_OBJS+=chdir-notify.o-LIB_OBJS+=checkout.o-LIB_OBJS+=chunk-format.o-LIB_OBJS+=color.o-LIB_OBJS+=column.o-LIB_OBJS+=combine-diff.o-LIB_OBJS+=commit-graph.o-LIB_OBJS+=commit-reach.o-LIB_OBJS+=commit.o+# LIB_OBJS: compat/* objects that live at the top-level+CONDITIONAL_OBJS+=unix-socket.o+CONDITIONAL_OBJS+=unix-stream-server.o+CONDITIONAL_OBJS+=sha1dc_git.o++# LIB_OBJS: Mostly glob *.c at the top-level, with some exlusions+LIB_OBJS+=$(filter-out\+$(CONDITIONAL_OBJS)\+git.ocommon-main.o$(PROGRAM_OBJS)\+$(FUZZ_OBJS)$(CURL_OBJS),\+$(patsubst%.c,%.o,$(wildcard*.c)))++# LIB_OBJS: Directories that contain only LIB_OBJS+LIB_OBJS_DIRS+=ewah+LIB_OBJS_DIRS+=negotiator+LIB_OBJS_DIRS+=refs+LIB_OBJS_DIRS+=trace2+LIB_OBJS+=$(patsubst%.c,%.o,$(wildcard$(addsuffix/*.c,$(LIB_OBJS_DIRS))))++# LIB_OBJS: unconditional compat/* objectsLIB_OBJS+=compat/obstack.oLIB_OBJS+=compat/terminal.o-LIB_OBJS+=config.o-LIB_OBJS+=connect.o-LIB_OBJS+=connected.o-LIB_OBJS+=convert.o-LIB_OBJS+=copy.o-LIB_OBJS+=credential.o-LIB_OBJS+=csum-file.o-LIB_OBJS+=ctype.o-LIB_OBJS+=date.o-LIB_OBJS+=decorate.o-LIB_OBJS+=delta-islands.o-LIB_OBJS+=diff-delta.o-LIB_OBJS+=diff-merges.o-LIB_OBJS+=diff-lib.o-LIB_OBJS+=diff-no-index.o-LIB_OBJS+=diff.o-LIB_OBJS+=diffcore-break.o-LIB_OBJS+=diffcore-delta.o-LIB_OBJS+=diffcore-order.o-LIB_OBJS+=diffcore-pickaxe.o-LIB_OBJS+=diffcore-rename.o-LIB_OBJS+=diffcore-rotate.o-LIB_OBJS+=dir-iterator.o-LIB_OBJS+=dir.o-LIB_OBJS+=editor.o-LIB_OBJS+=entry.o-LIB_OBJS+=environment.o-LIB_OBJS+=ewah/bitmap.o-LIB_OBJS+=ewah/ewah_bitmap.o-LIB_OBJS+=ewah/ewah_io.o-LIB_OBJS+=ewah/ewah_rlw.o-LIB_OBJS+=exec-cmd.o-LIB_OBJS+=fetch-negotiator.o-LIB_OBJS+=fetch-pack.o-LIB_OBJS+=fmt-merge-msg.o-LIB_OBJS+=fsck.o-LIB_OBJS+=fsmonitor.o-LIB_OBJS+=gettext.o-LIB_OBJS+=gpg-interface.o-LIB_OBJS+=graph.o-LIB_OBJS+=grep.o-LIB_OBJS+=hash-lookup.o-LIB_OBJS+=hashmap.o-LIB_OBJS+=help.o-LIB_OBJS+=hex.o-LIB_OBJS+=hook.o-LIB_OBJS+=ident.o-LIB_OBJS+=json-writer.o-LIB_OBJS+=kwset.o-LIB_OBJS+=levenshtein.o-LIB_OBJS+=line-log.o-LIB_OBJS+=line-range.o-LIB_OBJS+=linear-assignment.o-LIB_OBJS+=list-objects-filter-options.o-LIB_OBJS+=list-objects-filter.o-LIB_OBJS+=list-objects.o-LIB_OBJS+=ll-merge.o-LIB_OBJS+=lockfile.o-LIB_OBJS+=log-tree.o-LIB_OBJS+=ls-refs.o-LIB_OBJS+=mailinfo.o-LIB_OBJS+=mailmap.o-LIB_OBJS+=match-trees.o-LIB_OBJS+=mem-pool.o-LIB_OBJS+=merge-blobs.o-LIB_OBJS+=merge-ort.o-LIB_OBJS+=merge-ort-wrappers.o-LIB_OBJS+=merge-recursive.o-LIB_OBJS+=merge.o-LIB_OBJS+=mergesort.o-LIB_OBJS+=midx.o-LIB_OBJS+=name-hash.o-LIB_OBJS+=negotiator/default.o-LIB_OBJS+=negotiator/noop.o-LIB_OBJS+=negotiator/skipping.o-LIB_OBJS+=notes-cache.o-LIB_OBJS+=notes-merge.o-LIB_OBJS+=notes-utils.o-LIB_OBJS+=notes.o-LIB_OBJS+=object-file.o-LIB_OBJS+=object-name.o-LIB_OBJS+=object.o-LIB_OBJS+=oid-array.o-LIB_OBJS+=oidmap.o-LIB_OBJS+=oidset.o-LIB_OBJS+=oidtree.o-LIB_OBJS+=pack-bitmap-write.o-LIB_OBJS+=pack-bitmap.o-LIB_OBJS+=pack-check.o-LIB_OBJS+=pack-objects.o-LIB_OBJS+=pack-revindex.o-LIB_OBJS+=pack-write.o-LIB_OBJS+=packfile.o-LIB_OBJS+=pager.o-LIB_OBJS+=parallel-checkout.o-LIB_OBJS+=parse-options-cb.o-LIB_OBJS+=parse-options.o-LIB_OBJS+=patch-delta.o-LIB_OBJS+=patch-ids.o-LIB_OBJS+=path.o-LIB_OBJS+=pathspec.o-LIB_OBJS+=pkt-line.o-LIB_OBJS+=preload-index.o-LIB_OBJS+=pretty.o-LIB_OBJS+=prio-queue.o-LIB_OBJS+=progress.o-LIB_OBJS+=promisor-remote.o-LIB_OBJS+=prompt.o-LIB_OBJS+=protocol.o-LIB_OBJS+=protocol-caps.o-LIB_OBJS+=prune-packed.o-LIB_OBJS+=quote.o-LIB_OBJS+=range-diff.o-LIB_OBJS+=reachable.o-LIB_OBJS+=read-cache.o-LIB_OBJS+=rebase-interactive.o-LIB_OBJS+=rebase.o-LIB_OBJS+=ref-filter.o-LIB_OBJS+=reflog-walk.o-LIB_OBJS+=refs.o-LIB_OBJS+=refs/debug.o-LIB_OBJS+=refs/files-backend.o-LIB_OBJS+=refs/iterator.o-LIB_OBJS+=refs/packed-backend.o-LIB_OBJS+=refs/ref-cache.o-LIB_OBJS+=refspec.o-LIB_OBJS+=remote.o-LIB_OBJS+=replace-object.o-LIB_OBJS+=repo-settings.o-LIB_OBJS+=repository.o-LIB_OBJS+=rerere.o-LIB_OBJS+=reset.o-LIB_OBJS+=resolve-undo.o-LIB_OBJS+=revision.o-LIB_OBJS+=run-command.o-LIB_OBJS+=send-pack.o-LIB_OBJS+=sequencer.o-LIB_OBJS+=serve.o-LIB_OBJS+=server-info.o-LIB_OBJS+=setup.o-LIB_OBJS+=shallow.o-LIB_OBJS+=sideband.o-LIB_OBJS+=sigchain.o-LIB_OBJS+=sparse-index.o-LIB_OBJS+=split-index.o-LIB_OBJS+=stable-qsort.o-LIB_OBJS+=strbuf.o-LIB_OBJS+=streaming.o-LIB_OBJS+=string-list.o-LIB_OBJS+=strmap.o-LIB_OBJS+=strvec.o-LIB_OBJS+=sub-process.o-LIB_OBJS+=submodule-config.o-LIB_OBJS+=submodule.o-LIB_OBJS+=symlinks.o-LIB_OBJS+=tag.o-LIB_OBJS+=tempfile.o-LIB_OBJS+=thread-utils.o-LIB_OBJS+=tmp-objdir.o-LIB_OBJS+=trace.o-LIB_OBJS+=trace2.o-LIB_OBJS+=trace2/tr2_cfg.o-LIB_OBJS+=trace2/tr2_cmd_name.o-LIB_OBJS+=trace2/tr2_dst.o-LIB_OBJS+=trace2/tr2_sid.o-LIB_OBJS+=trace2/tr2_sysenv.o-LIB_OBJS+=trace2/tr2_tbuf.o-LIB_OBJS+=trace2/tr2_tgt_event.o-LIB_OBJS+=trace2/tr2_tgt_normal.o-LIB_OBJS+=trace2/tr2_tgt_perf.o-LIB_OBJS+=trace2/tr2_tls.o-LIB_OBJS+=trailer.o-LIB_OBJS+=transport-helper.o-LIB_OBJS+=transport.o-LIB_OBJS+=tree-diff.o-LIB_OBJS+=tree-walk.o-LIB_OBJS+=tree.o-LIB_OBJS+=unpack-trees.o-LIB_OBJS+=upload-pack.o-LIB_OBJS+=url.o-LIB_OBJS+=urlmatch.o-LIB_OBJS+=usage.o-LIB_OBJS+=userdiff.o-LIB_OBJS+=utf8.o-LIB_OBJS+=varint.o-LIB_OBJS+=version.o-LIB_OBJS+=versioncmp.o-LIB_OBJS+=walker.o-LIB_OBJS+=wildmatch.o-LIB_OBJS+=worktree.o-LIB_OBJS+=wrapper.o-LIB_OBJS+=write-or-die.o-LIB_OBJS+=ws.o-LIB_OBJS+=wt-status.o-LIB_OBJS+=xdiff-interface.o-LIB_OBJS+=zlib.o--BUILTIN_OBJS+=builtin/add.o-BUILTIN_OBJS+=builtin/am.o-BUILTIN_OBJS+=builtin/annotate.o-BUILTIN_OBJS+=builtin/apply.o-BUILTIN_OBJS+=builtin/archive.o-BUILTIN_OBJS+=builtin/bisect--helper.o-BUILTIN_OBJS+=builtin/blame.o-BUILTIN_OBJS+=builtin/branch.o-BUILTIN_OBJS+=builtin/bugreport.o-BUILTIN_OBJS+=builtin/bundle.o-BUILTIN_OBJS+=builtin/cat-file.o-BUILTIN_OBJS+=builtin/check-attr.o-BUILTIN_OBJS+=builtin/check-ignore.o-BUILTIN_OBJS+=builtin/check-mailmap.o-BUILTIN_OBJS+=builtin/check-ref-format.o-BUILTIN_OBJS+=builtin/checkout--worker.o-BUILTIN_OBJS+=builtin/checkout-index.o-BUILTIN_OBJS+=builtin/checkout.o-BUILTIN_OBJS+=builtin/clean.o-BUILTIN_OBJS+=builtin/clone.o-BUILTIN_OBJS+=builtin/column.o-BUILTIN_OBJS+=builtin/commit-graph.o-BUILTIN_OBJS+=builtin/commit-tree.o-BUILTIN_OBJS+=builtin/commit.o-BUILTIN_OBJS+=builtin/config.o-BUILTIN_OBJS+=builtin/count-objects.o-BUILTIN_OBJS+=builtin/credential-cache--daemon.o-BUILTIN_OBJS+=builtin/credential-cache.o-BUILTIN_OBJS+=builtin/credential-store.o-BUILTIN_OBJS+=builtin/credential.o-BUILTIN_OBJS+=builtin/describe.o-BUILTIN_OBJS+=builtin/diff-files.o-BUILTIN_OBJS+=builtin/diff-index.o-BUILTIN_OBJS+=builtin/diff-tree.o-BUILTIN_OBJS+=builtin/diff.o-BUILTIN_OBJS+=builtin/difftool.o-BUILTIN_OBJS+=builtin/env--helper.o-BUILTIN_OBJS+=builtin/fast-export.o-BUILTIN_OBJS+=builtin/fast-import.o-BUILTIN_OBJS+=builtin/fetch-pack.o-BUILTIN_OBJS+=builtin/fetch.o-BUILTIN_OBJS+=builtin/fmt-merge-msg.o-BUILTIN_OBJS+=builtin/for-each-ref.o-BUILTIN_OBJS+=builtin/for-each-repo.o-BUILTIN_OBJS+=builtin/fsck.o-BUILTIN_OBJS+=builtin/gc.o-BUILTIN_OBJS+=builtin/get-tar-commit-id.o-BUILTIN_OBJS+=builtin/grep.o-BUILTIN_OBJS+=builtin/hash-object.o-BUILTIN_OBJS+=builtin/help.o-BUILTIN_OBJS+=builtin/index-pack.o-BUILTIN_OBJS+=builtin/init-db.o-BUILTIN_OBJS+=builtin/interpret-trailers.o-BUILTIN_OBJS+=builtin/log.o-BUILTIN_OBJS+=builtin/ls-files.o-BUILTIN_OBJS+=builtin/ls-remote.o-BUILTIN_OBJS+=builtin/ls-tree.o-BUILTIN_OBJS+=builtin/mailinfo.o-BUILTIN_OBJS+=builtin/mailsplit.o-BUILTIN_OBJS+=builtin/merge-base.o-BUILTIN_OBJS+=builtin/merge-file.o-BUILTIN_OBJS+=builtin/merge-index.o-BUILTIN_OBJS+=builtin/merge-ours.o-BUILTIN_OBJS+=builtin/merge-recursive.o-BUILTIN_OBJS+=builtin/merge-tree.o-BUILTIN_OBJS+=builtin/merge.o-BUILTIN_OBJS+=builtin/mktag.o-BUILTIN_OBJS+=builtin/mktree.o-BUILTIN_OBJS+=builtin/multi-pack-index.o-BUILTIN_OBJS+=builtin/mv.o-BUILTIN_OBJS+=builtin/name-rev.o-BUILTIN_OBJS+=builtin/notes.o-BUILTIN_OBJS+=builtin/pack-objects.o-BUILTIN_OBJS+=builtin/pack-redundant.o-BUILTIN_OBJS+=builtin/pack-refs.o-BUILTIN_OBJS+=builtin/patch-id.o-BUILTIN_OBJS+=builtin/prune-packed.o-BUILTIN_OBJS+=builtin/prune.o-BUILTIN_OBJS+=builtin/pull.o-BUILTIN_OBJS+=builtin/push.o-BUILTIN_OBJS+=builtin/range-diff.o-BUILTIN_OBJS+=builtin/read-tree.o-BUILTIN_OBJS+=builtin/rebase.o-BUILTIN_OBJS+=builtin/receive-pack.o-BUILTIN_OBJS+=builtin/reflog.o-BUILTIN_OBJS+=builtin/remote-ext.o-BUILTIN_OBJS+=builtin/remote-fd.o-BUILTIN_OBJS+=builtin/remote.o-BUILTIN_OBJS+=builtin/repack.o-BUILTIN_OBJS+=builtin/replace.o-BUILTIN_OBJS+=builtin/rerere.o-BUILTIN_OBJS+=builtin/reset.o-BUILTIN_OBJS+=builtin/rev-list.o-BUILTIN_OBJS+=builtin/rev-parse.o-BUILTIN_OBJS+=builtin/revert.o-BUILTIN_OBJS+=builtin/rm.o-BUILTIN_OBJS+=builtin/send-pack.o-BUILTIN_OBJS+=builtin/shortlog.o-BUILTIN_OBJS+=builtin/show-branch.o-BUILTIN_OBJS+=builtin/show-index.o-BUILTIN_OBJS+=builtin/show-ref.o-BUILTIN_OBJS+=builtin/sparse-checkout.o-BUILTIN_OBJS+=builtin/stash.o-BUILTIN_OBJS+=builtin/stripspace.o-BUILTIN_OBJS+=builtin/submodule--helper.o-BUILTIN_OBJS+=builtin/symbolic-ref.o-BUILTIN_OBJS+=builtin/tag.o-BUILTIN_OBJS+=builtin/unpack-file.o-BUILTIN_OBJS+=builtin/unpack-objects.o-BUILTIN_OBJS+=builtin/update-index.o-BUILTIN_OBJS+=builtin/update-ref.o-BUILTIN_OBJS+=builtin/update-server-info.o-BUILTIN_OBJS+=builtin/upload-archive.o-BUILTIN_OBJS+=builtin/upload-pack.o-BUILTIN_OBJS+=builtin/var.o-BUILTIN_OBJS+=builtin/verify-commit.o-BUILTIN_OBJS+=builtin/verify-pack.o-BUILTIN_OBJS+=builtin/verify-tag.o-BUILTIN_OBJS+=builtin/worktree.o-BUILTIN_OBJS+=builtin/write-tree.o# THIRD_PARTY_SOURCES is a list of patterns compatible with the# $(filter) and $(filter-out) family of functions. They specify source
@@ -2076,6 +1698,7 @@ endifLIBS=$(filter-out%.o,$(GITLIBS))$(EXTLIBS)BASIC_CFLAGS+=$(COMPAT_CFLAGS)+LIB_OBJS_NO_COMPAT:=$(LIB_OBJS)LIB_OBJS+=$(COMPAT_OBJS)# Quote for C
@@ -2436,17 +2059,12 @@ reconfigure config.mak.autogen: config.status.PHONY:reconfigure# This is a convenience target.endif-XDIFF_OBJS+=xdiff/xdiffi.o-XDIFF_OBJS+=xdiff/xemit.o-XDIFF_OBJS+=xdiff/xhistogram.o-XDIFF_OBJS+=xdiff/xmerge.o-XDIFF_OBJS+=xdiff/xpatience.o-XDIFF_OBJS+=xdiff/xprepare.o-XDIFF_OBJS+=xdiff/xutils.o+XDIFF_SRC+=$(wildcardxdiff/*.c)+XDIFF_OBJS+=$(XDIFF_SRC:.c=.o).PHONY:xdiff-objsxdiff-objs:$(XDIFF_OBJS)-TEST_OBJS:=$(patsubst%$X,%.o,$(TEST_PROGRAMS))$(patsubst%,t/helper/%,$(TEST_BUILTINS_OBJS))+TEST_OBJS:=$(patsubst%$X,%.o,$(TEST_PROGRAMS))$(TEST_BUILTINS_OBJS).PHONY:test-objstest-objs:$(TEST_OBJS)
@@ -111,32 +111,15 @@ project(git#TODO Add pcre support#macros for parsing the Makefile for sources and scripts-macro(parse_makefile_for_sourceslist_varregex)-file(STRINGS${CMAKE_SOURCE_DIR}/Makefile${list_var}REGEX"^${regex} \\+=(.*)")-string(REPLACE"${regex} +="""${list_var}${${list_var}})-string(REPLACE"$(COMPAT_OBJS)"""${list_var}${${list_var}})#remove "$(COMPAT_OBJS)" This is only for libgit.-string(STRIP${${list_var}}${list_var})#remove trailing/leading whitespaces-string(REPLACE".o"".c;"${list_var}${${list_var}})#change .o to .c, ; is for converting the string into a list-list(TRANSFORM${list_var}STRIP)#remove trailing/leading whitespaces for each element in list-list(REMOVE_ITEM${list_var}"")#remove empty list elements-endmacro()--macro(parse_makefile_for_scriptslist_varregexlang)-file(STRINGS${CMAKE_SOURCE_DIR}/Makefile${list_var}REGEX"^${regex} \\+=(.*)")-string(REPLACE"${regex} +="""${list_var}${${list_var}})-string(STRIP${${list_var}}${list_var})#remove trailing/leading whitespaces-string(REPLACE" "";"${list_var}${${list_var}})#convert string to a list-if(NOT${lang})#exclude for SCRIPT_LIB-list(TRANSFORM${list_var}REPLACE"${lang}""")#do the replacement-endif()-endmacro()--macro(parse_makefile_for_executableslist_varregex)-file(STRINGS${CMAKE_SOURCE_DIR}/Makefile${list_var}REGEX"^${regex} \\+= git-(.*)")-string(REPLACE"${regex} +="""${list_var}${${list_var}})+macro(ask_makefile_for_listlist_varvar_name)+execute_process(COMMANDmakeprint-list-${var_name}+WORKING_DIRECTORY${CMAKE_SOURCE_DIR}+OUTPUT_VARIABLE${list_var})+string(REGEXREPLACE"\\.o\n"".c\n"${list_var}${${list_var}})#change .o to .cstring(STRIP${${list_var}}${list_var})#remove trailing/leading whitespaces-string(REPLACE"git-"""${list_var}${${list_var}})#strip `git-` prefix-string(REPLACE"\$X"";"${list_var}${${list_var}})#strip $X, ; is for converting the string into a list+## Parse the Makefile print-list-% format+string(REGEXREPLACE"${var_name} =\n"""${list_var}${${list_var}})+string(REGEXREPLACE"${var_name} \\+?= ([^\n]+)""\\1;"${list_var}${${list_var}})list(TRANSFORM${list_var}STRIP)#remove trailing/leading whitespaces for each element in listlist(REMOVE_ITEM${list_var}"")#remove empty list elementsendmacro()
@@ -729,7 +712,9 @@ if(CURL_FOUND)endif()endif()-parse_makefile_for_executables(git_builtin_extra"BUILT_INS")+ask_makefile_for_list(git_builtin_extra"BUILT_INS")+list(TRANSFORMgit_builtin_extraREPLACE"^git-(.*)""\\1")#strip `git-` prefix+list(TRANSFORMgit_builtin_extraREPLACE"\.exe$""")#strip $Xoption(SKIP_DASHED_BUILT_INS"Skip hardlinking the dashed versions of the built-ins")
Another way to write this would be:
LIB_OBJS += $(patsubst %.c,%.o,$(wildcard $(addsuffix /*.c,$(LIB_OBJS_DIRS)))
I don't know that there's any reason to choose one over the other. I
don't think there's any real performance difference although one could
imagine this version to be VERY SLIGHTLY faster. Also this one is a
little more "Lisp-ish"... that might be a pro or a con depending :).
Just kibitzing while waiting for dinner to arrive...
From: Jeff King <hidden> Date: 2021-11-03 11:30:51
On Sun, Oct 31, 2021 at 02:00:42PM +0100, Ævar Arnfjörð Bjarmason wrote:
quoted
Likewise, they could go into a conditional-src/ directory (or some
less-horrible name) to keep them distinct without needing an explicit
list in the Makefile. That's sort of the flip-side of putting all the
other LIB_OBJS ones into lib/.
The goal here was just to get us rid of tiresome merge conflicts when
two things are added to adjacent part of these lists going forward,
rather than some source-tree reorganization. I didn't search around and
didn't find that 2011-era thread.
Right, sorry, I didn't mean to sidetrack. That was just the only
discussion I could find on the topic. I agree that it's worth
considering the two topics (moving files around vs Makefile changes)
separately.
Even if we carefully trickle those in at a rate that doesn't conflict
with anything in-flight, the end result will be that e.g.:
git log -- lib/grep.c
Will stop at that rename commit, similar to builtin/log.c, unless you
specify --follow etc. Just that doesn't make it worth it to me. Likewise
sha1_file.c to sha1-file.c to object-file.c, which is a case I run into
every time I get a "git log" pathspec glob wrong.
Agreed. One of the arguments back when we started to move around a few
files is that this dog-fooding would encourage us to make --follow mode
better. That hasn't really happened, though.
I didn't notice before submitting this but this patch breaks the
vs-build job, because the cmake build in "contrib" is screen-scraping
the Makefile[1].
What's the status of that code? It's rather tiresome to need to patch
two independent and incompatible build systems every time there's some
structural change in the Makefile.
My opinion when we took in the cmake topic was that it would be OK for
people working on the main Makefile to break cmake. It's an add-on and
the people who care about cmake are the ones who will do the work to
track the Makefile.
But since there's a CI job that will nag you if it fails, that kind of
makes it everybody's problem in practice. That doesn't change my opinion
on how things _should_ work, but I have done small fixups as necessary
to stop the nagging.
I hadn't looked in any detail at that recipe before, but it the vs-build
job has a hard dependency on GNU make anyway, since we use it for "make
artifacts-tar".
So whatever cmake special-sauce is happening there I don't see why
vs-build couldn't call out "make" for most of the work it's doing, isn't
it just some replacement for what the "vcxproj" target in
config.mak.uname used to do?
The big question for me is whether that really is a hard dependency.
Obviously "make artifacts-tar" is for the CI job, but is the cmake stuff
supposed to work for regular users without relying on having GNU make at
all? I have no clue.
-Peff
On Sun, Oct 31, 2021 at 02:00:42PM +0100, Ævar Arnfjörð Bjarmason wrote:
[...]
quoted
I didn't notice before submitting this but this patch breaks the
vs-build job, because the cmake build in "contrib" is screen-scraping
the Makefile[1].
What's the status of that code? It's rather tiresome to need to patch
two independent and incompatible build systems every time there's some
structural change in the Makefile.
My opinion when we took in the cmake topic was that it would be OK for
people working on the main Makefile to break cmake. It's an add-on and
the people who care about cmake are the ones who will do the work to
track the Makefile.
But since there's a CI job that will nag you if it fails, that kind of
makes it everybody's problem in practice. That doesn't change my opinion
on how things _should_ work, but I have done small fixups as necessary
to stop the nagging.
Yes, that was clearly the intent from reading the original discussion,
but we've crept towards it being an actual hard dependency. I'd also be
fine with some direction that just removed that vs-build/vs-test job to
something optional...
quoted
I hadn't looked in any detail at that recipe before, but it the vs-build
job has a hard dependency on GNU make anyway, since we use it for "make
artifacts-tar".
So whatever cmake special-sauce is happening there I don't see why
vs-build couldn't call out "make" for most of the work it's doing, isn't
it just some replacement for what the "vcxproj" target in
config.mak.uname used to do?
The big question for me is whether that really is a hard dependency.
Obviously "make artifacts-tar" is for the CI job, but is the cmake stuff
supposed to work for regular users without relying on having GNU make at
all? I have no clue.
It's a hard dependency for the job, since it tars up its built assets in
the first step, and those are then unpacked and used in subsequent
steps. It's being used to ferry the built binaries between CI phases.
But yes, the intent was clearly to not have a dependency on GNU make,
but as I argue in
[off-list ref] I think
having those developers simply install it is better than forcing us to
maintain two distinct and incompatible build systems.
The selling point was that it was going to be really easy to maintain
them in parallel, i.e. you'd just add a thing to a list here and a list
there, but that assumes that nothing will ever structurally change in
the Makefile.
I think the other X-Y problem being solved there was that cmake has some
better integration for Visual Studio somehow. I.e. it does what the
"vcxproj" target in config.mak.uname does/did.
I think that would be a fine use for cmake, and we can clearly
accomplish that by having our cmake file effectively be a mostly thin
wrapper for logic that lives in the Makefile.
I.e. it would ask the Makefile what's in this list or other, what the
CFLAGS are etc. etc., and feed that into relevant cmake variables.
My patch starts us moving in that direction (but doesn't get anywhere
close to that end-goal). I think if we did that the ~1k line
CMakeLists.txt would be maybe 100-300 lines.
From: Johannes Schindelin <hidden> Date: 2021-11-04 00:32:53
Hi Peff,
On Wed, 3 Nov 2021, Jeff King wrote:
On Sun, Oct 31, 2021 at 02:00:42PM +0100, Ævar Arnfjörð Bjarmason wrote:
quoted
I didn't notice before submitting this but this patch breaks the
vs-build job, because the cmake build in "contrib" is screen-scraping
the Makefile[1].
What's the status of that code? It's rather tiresome to need to patch
two independent and incompatible build systems every time there's some
structural change in the Makefile.
My opinion when we took in the cmake topic was that it would be OK for
people working on the main Makefile to break cmake. It's an add-on and
the people who care about cmake are the ones who will do the work to
track the Makefile.
I do try to have a look at breakages in `seen` when I have the time, but
lately I didn't. That's why you may have felt more of these CMake
headaches.
But since there's a CI job that will nag you if it fails, that kind of
makes it everybody's problem in practice. That doesn't change my opinion
on how things _should_ work, but I have done small fixups as necessary
to stop the nagging.
One very simple solution is to leave the Makefile alone unless it really,
really needs to be changed. There are costs to refactoring, and quite
honestly, it might be a good thing that something like a failing vs-build
job discourages refactoring for refactoring's sake.
quoted
I hadn't looked in any detail at that recipe before, but it the
vs-build job has a hard dependency on GNU make anyway, since we use it
for "make artifacts-tar".
So whatever cmake special-sauce is happening there I don't see why
vs-build couldn't call out "make" for most of the work it's doing,
isn't it just some replacement for what the "vcxproj" target in
config.mak.uname used to do?
The big question for me is whether that really is a hard dependency.
Obviously "make artifacts-tar" is for the CI job, but is the cmake stuff
supposed to work for regular users without relying on having GNU make at
all? I have no clue.
The entire point of the CMake configuration is to allow developers on
Windows to use the tools they are used to, to build Git. And believe it or
not, GNU make is not one of those tools! I know. Very hard to believe. :-)
So yeah, the vs-build/vs-test combo tries to pay attention to this
intended scenario, and avoids using GNU make or any other of those Unix
tools that much less ubiquitous than some might want to believe.
Ciao,
Dscho
On Sun, Oct 31, 2021 at 02:00:42PM +0100, Ævar Arnfjörð Bjarmason wrote:
quoted
I didn't notice before submitting this but this patch breaks the
vs-build job, because the cmake build in "contrib" is screen-scraping
the Makefile[1].
What's the status of that code? It's rather tiresome to need to patch
two independent and incompatible build systems every time there's some
structural change in the Makefile.
My opinion when we took in the cmake topic was that it would be OK for
people working on the main Makefile to break cmake. It's an add-on and
the people who care about cmake are the ones who will do the work to
track the Makefile.
I do try to have a look at breakages in `seen` when I have the time, but
lately I didn't. That's why you may have felt more of these CMake
headaches.
It's not only things that make it into "seen", as most will test their
topic in GitHub CI before submission in their own repos.
quoted
But since there's a CI job that will nag you if it fails, that kind of
makes it everybody's problem in practice. That doesn't change my opinion
on how things _should_ work, but I have done small fixups as necessary
to stop the nagging.
One very simple solution is to leave the Makefile alone unless it really,
really needs to be changed. There are costs to refactoring, and quite
honestly, it might be a good thing that something like a failing vs-build
job discourages refactoring for refactoring's sake.
Sure, but that's the case with any critical component we're using. A
question of "is it worth leaving it alone" is distinct from "is it
painful to touch it because you need to implement a fix twice in two
incompatible languages?".
In this case I do think the change is justified. I've personally got a
few local topics that I keep having to (even with rerere) solve
conflicts for due to this list of files, and Junio deals with the same.
Ditto for some of the changes I've made recently to make things
non-.PHONY. That's resulted in major workflow improvements for me,
But in any case, the selling point of the original cmake integration was
not something to the effect of:
"nobody should have to change this in anything but ever so this
re-implementation is a one-off"
But rather something like:
"This re-implementation is a one-off, but any updates to both should
be trivial."
As someone who's had a couple of recent run-ins with cmake I can tell
you it's really not trivial at all.
So given that the selling point of the original change didn't turn out
as was expected I think it's fair to re-visit whether we'd like to take
this path going forward, or to choose another trade-off.
quoted
quoted
I hadn't looked in any detail at that recipe before, but it the
vs-build job has a hard dependency on GNU make anyway, since we use it
for "make artifacts-tar".
So whatever cmake special-sauce is happening there I don't see why
vs-build couldn't call out "make" for most of the work it's doing,
isn't it just some replacement for what the "vcxproj" target in
config.mak.uname used to do?
The big question for me is whether that really is a hard dependency.
Obviously "make artifacts-tar" is for the CI job, but is the cmake stuff
supposed to work for regular users without relying on having GNU make at
all? I have no clue.
The entire point of the CMake configuration is to allow developers on
Windows to use the tools they are used to, to build Git. And believe it or
not, GNU make is not one of those tools! I know. Very hard to believe. :-)
I believe that, the question is why it isn't a better trade-off to just
ask those users to install that software. Our Windows CI is doing it
on-the-fly, so clearly it's not that hard to do it.
Note that I'm not saying that whatever integration those users get in VS
from the special-cause CMake integration should change. We're only
talking about it invoking "make" under the hood in a way that'll be
invisible to the user.
POSIX "sh" isn't native to Windows either, and that CMake file invokes
shellscripts we ship to e.g. build the generated headers, so this
workflow is clearly something that's OK for an end-user once the one-off
hassle of installing a package is over with.
From: Philip Oakley <hidden> Date: 2021-11-04 14:29:44
On 04/11/2021 09:46, Ævar Arnfjörð Bjarmason wrote:
quoted
he entire point of the CMake configuration is to allow developers on
Windows to use the tools they are used to, to build Git. And believe it or
not, GNU make is not one of those tools! I know. Very hard to believe. :-)
I believe that, the question is why it isn't a better trade-off to just
ask those users to install that software. Our Windows CI is doing it
on-the-fly, so clearly it's not that hard to do it.
Just to say that, while it is real easy to download and install the
Git-for-Windows SDK (https://gitforwindows.org/#download-sdk), for most
(Windows) users it's a foreign land, with few friends who understand
what things like `gdb` are all about. It's all doable, but the learning
curve can be hard. The CI doesn't need a learning curve ;-)
Being able to fire up a well 'trusted' tool like Visual Studio to
investigate the code does help contributors understand the code.
--
Philip
Hi Ævar
On 01/11/2021 19:19, Ævar Arnfjörð Bjarmason wrote:
Remove the hardcoded lists of objects in favor of using
$(wildcard). This means that every time a built-in, test tool etc. is
added we won't need to patch the top-level Makefile, except for the
few remaining cases where the asset in question would make it onto one
of our list of exceptions.
Ever since 81b50f3ce40 (Move 'builtin-*' into a 'builtin/'
subdirectory, 2010-02-22) this has been relatively easy to do (and
even before that we could glob builtin-*.c). This pattern of
exhaustively enumerating files was then carried forward for
e.g. TEST_BUILTINS_OBJS in efd71f8913a (t/helper: add an empty
test-tool program, 2018-03-24).
One reason not to do this is that now a new *.c file at the top-level
will be immediately picked up, so if a new *.c file is being worked on
"make" will error if it doesn't compile, whereas before that file
would need to be explicitly listed in the Makefile. I think than small
trade-off is worth it.
If I need to split up some uncommitted changes into several commits and
I know it is going to be fiddly to do so I will sometimes copy the
original file to foo.safe.c and then edit foo.c to create each commit.
Then I can easily compile and test each commit and editing the file
directly is often easier than using add -p and editing the hunks. With
this patch running make will fail in that case I think.
We could make this simpler still for the Makefile by moving
"unix-socket.c" etc. to e.g. a "conditional-src/" directory, likewise
for $(PROGRAM_OBJS) to e.g. "programs/". If we did that we would not
need the "$(filter-out)" for LIB_OBJS. I don't think that's worth it,
e.g. due to "git log -- <path>" on the files now needing a "--follow".
There's a few small "while we're at it" changes here, since I'm
touching the code in question:
- Start splitting up the the "Guard against the environment" section
at the top, but don't move anything that exists there out to avoid
merge conflicts
- The $(TEST_BUILTINS_OBJS) variable was needlessly complex, because
it didn't have the full paths we'd pathsubst it back & forth.
- Introduce *_SRC in addition to *_OBJ for the variable I'm
touching. Eventually we'll want to do this for all the *.o files,
i.e. make the *.c list a source of truth for *.o, which means we can
e.g. use that exhaustive list for "make TAGS".
- Add a missing "curl-objs" target. See 029bac01a87 (Makefile: add
{program,xdiff,test,git,fuzz}-objs & objects targets, 2021-02-23)
for the commit that added the rest.
- De-indent an "ifndef" block, we don't usually indent their
contents.
On the CMake changes here:
- When CMake support was introduced in was introduced
061c2240b1b (Introduce CMake support for configuring Git, 2020-06-12)
there was a discussion about the maintenance burden of maintaining the
top-level Makefile in parallel with CMakeLists.txt[1] where reviewers
were assured that doing so would simply be a matter of adding something
to a list in the CMake recipe.
Between change and some recent changes of mine where the "vs-build"
job failed to a divergence between the Makefile and CMakeList.txt I
can confidently say that that doesn't at all match reality. Even
seemingly trivial changes to the Makefile like this one are forcing
us to do a deep-dive into CMake internals to make forward progress
with our main build system.
My recollection is that the discussions were about not having to touch
CMakeList.txt when adding new files to the build and I think that
largely works. I don't think a lot of the changes you have been making
recently were anticipated in that discussion.
- The promised "We can add a (continue-on-error) to vs-build job to
make this process less of a hindrance." in [2] never materialized.
Since 4c2c38e800f (ci: modification of main.yml to use cmake for
vs-build job, 2020-06-26) got a hard dependency on CMake as far as
getting the CI to pass goes.
- The "vs-build" CI doesn't actually require that there be no GNU make
usage in the job, as it itself has a hard dependency on running a
"make -n artifacts-tar" command. So as far as any vs-specific special-sauce
goes we don't need a GNU-make free build system for vs-build.
We need GNU-make for the ci job but an individual developer using CMake
does not need GNU-make installed. On linux it is possible to build git
without having make installed by using cmake and ninja [1]
- The stated goal in 061c2240b1b of avoiding a GNU make dependency
for developer because it requires an SDK that "occupies around two
gigabytes" and "three quarters of a gigabyte worth of Git objects"
hardly seems worthwhile trade-off given the above. Disk space is cheap,
developer time required to maintain two parallel build systems isn't.
That rather assumes everyone has plenty of disk space and a decent
network connection.
My attempt to amend/revert 4c2c38e800f to have it use the
pre-4c2c38e800f "make" invocation as a fallback failed, partially
because I don't have a Windows development environment, so any attempt
to change it is a painfully slow round-trip to GitHub CI.
Let's instead have CMake call out to the Makefile asking it what the
definition of various variables lists is, rather than being forced to
maintain those lists in a way that CMake can parse with regexes (which
precludes anything but a giant hardcoded list).
I could familiarize myself enough with CMake to do this in some
CMake-native way, but that would take "just as long as adding it to
the Makefile"[2] (I think that took me <5 minutes, but I'm several
hours into fighting with CMake)
So I consider this both a bugfix to the stated aims of this CMake
integration, and a better way forward for having an alternate build
system. I.e. If someone really does care about a having a
GNU-make-less dependency for the "vs-build" I think this change offers
a much better way forward for that.
I don't see how relying on GNU-make is a step forward for the CMake
integration when it works without it now.
Overall I'm don't think that moving from a known set of dependencies to
"build whatever C files are lying around in this directory" is an
improvement.
Best Wishes
Phillip
[1] The CMake integration is currently broken for non-windows builds,
I've got some fixes at
https://github.com/phillipwood/git/tree/wip/cmake-fixes
quoted hunk
Once we invoke the Makefile to spew out e.g. its idea of "LIB_OBJS",
it's going to be trivial to do that via some wrapper script that lives
in "contrib/buildsystems". Such a script would either invoke "make
print-{var,list}-%", or alternatively use an in-tree committed text
file with the last known result of such a "make print-{var,list}-%"
run.
1. https://lore.kernel.org/git/xmqq8sikblv2.fsf@gitster.c.googlers.com
2. https://lore.kernel.org/git/CAKiG+9Xtof8Hj3npsS-M0SnT_dcjtHjP_+avWB4oOHkaMdnSbw@mail.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 485 ++++------------------------
contrib/buildsystems/CMakeLists.txt | 53 ++-
2 files changed, 74 insertions(+), 464 deletions(-)
@@ -590,6 +590,19 @@ TEST_OBJS =TEST_PROGRAMS_NEED_X=THIRD_PARTY_SOURCES=+## Guard against env: programs+TEST_PROGRAMS=++## Guard against env: sources+CURL_SRC=+TEST_PROGRAMS_NEED_X_SRC=+XDIFF_SRC=++## Guard against env: objects+CONDITIONAL_OBJS=+CURL_OBJS=+LIB_OBJS_DIRS=+ # Utility to dump whatever variables are defined hereprint-var-%:@echo$($*)
@@ -697,87 +710,23 @@ X =PROGRAMS+=$(patsubst%.o,git-%$X,$(PROGRAM_OBJS))-TEST_BUILTINS_OBJS+=test-advise.o-TEST_BUILTINS_OBJS+=test-bitmap.o-TEST_BUILTINS_OBJS+=test-bloom.o-TEST_BUILTINS_OBJS+=test-chmtime.o-TEST_BUILTINS_OBJS+=test-config.o-TEST_BUILTINS_OBJS+=test-crontab.o-TEST_BUILTINS_OBJS+=test-ctype.o-TEST_BUILTINS_OBJS+=test-date.o-TEST_BUILTINS_OBJS+=test-delta.o-TEST_BUILTINS_OBJS+=test-dir-iterator.o-TEST_BUILTINS_OBJS+=test-drop-caches.o-TEST_BUILTINS_OBJS+=test-dump-cache-tree.o-TEST_BUILTINS_OBJS+=test-dump-fsmonitor.o-TEST_BUILTINS_OBJS+=test-dump-split-index.o-TEST_BUILTINS_OBJS+=test-dump-untracked-cache.o-TEST_BUILTINS_OBJS+=test-example-decorate.o-TEST_BUILTINS_OBJS+=test-fast-rebase.o-TEST_BUILTINS_OBJS+=test-genrandom.o-TEST_BUILTINS_OBJS+=test-genzeros.o-TEST_BUILTINS_OBJS+=test-getcwd.o-TEST_BUILTINS_OBJS+=test-hash-speed.o-TEST_BUILTINS_OBJS+=test-hash.o-TEST_BUILTINS_OBJS+=test-hashmap.o-TEST_BUILTINS_OBJS+=test-index-version.o-TEST_BUILTINS_OBJS+=test-json-writer.o-TEST_BUILTINS_OBJS+=test-lazy-init-name-hash.o-TEST_BUILTINS_OBJS+=test-match-trees.o-TEST_BUILTINS_OBJS+=test-mergesort.o-TEST_BUILTINS_OBJS+=test-mktemp.o-TEST_BUILTINS_OBJS+=test-oid-array.o-TEST_BUILTINS_OBJS+=test-oidmap.o-TEST_BUILTINS_OBJS+=test-oidtree.o-TEST_BUILTINS_OBJS+=test-online-cpus.o-TEST_BUILTINS_OBJS+=test-parse-options.o-TEST_BUILTINS_OBJS+=test-parse-pathspec-file.o-TEST_BUILTINS_OBJS+=test-partial-clone.o-TEST_BUILTINS_OBJS+=test-path-utils.o-TEST_BUILTINS_OBJS+=test-pcre2-config.o-TEST_BUILTINS_OBJS+=test-pkt-line.o-TEST_BUILTINS_OBJS+=test-prio-queue.o-TEST_BUILTINS_OBJS+=test-proc-receive.o-TEST_BUILTINS_OBJS+=test-progress.o-TEST_BUILTINS_OBJS+=test-reach.o-TEST_BUILTINS_OBJS+=test-read-cache.o-TEST_BUILTINS_OBJS+=test-read-graph.o-TEST_BUILTINS_OBJS+=test-read-midx.o-TEST_BUILTINS_OBJS+=test-ref-store.o-TEST_BUILTINS_OBJS+=test-regex.o-TEST_BUILTINS_OBJS+=test-repository.o-TEST_BUILTINS_OBJS+=test-revision-walking.o-TEST_BUILTINS_OBJS+=test-run-command.o-TEST_BUILTINS_OBJS+=test-scrap-cache-tree.o-TEST_BUILTINS_OBJS+=test-serve-v2.o-TEST_BUILTINS_OBJS+=test-sha1.o-TEST_BUILTINS_OBJS+=test-sha256.o-TEST_BUILTINS_OBJS+=test-sigchain.o-TEST_BUILTINS_OBJS+=test-simple-ipc.o-TEST_BUILTINS_OBJS+=test-strcmp-offset.o-TEST_BUILTINS_OBJS+=test-string-list.o-TEST_BUILTINS_OBJS+=test-submodule-config.o-TEST_BUILTINS_OBJS+=test-submodule-nested-repo-config.o-TEST_BUILTINS_OBJS+=test-subprocess.o-TEST_BUILTINS_OBJS+=test-trace2.o-TEST_BUILTINS_OBJS+=test-urlmatch-normalization.o-TEST_BUILTINS_OBJS+=test-userdiff.o-TEST_BUILTINS_OBJS+=test-wildmatch.o-TEST_BUILTINS_OBJS+=test-windows-named-pipe.o-TEST_BUILTINS_OBJS+=test-write-cache.o-TEST_BUILTINS_OBJS+=test-xml-encode.o- # Do not add more tests here unless they have extra dependencies. Add # them in TEST_BUILTINS_OBJS above.TEST_PROGRAMS_NEED_X+=test-fake-sshTEST_PROGRAMS_NEED_X+=test-tool-TEST_PROGRAMS=$(patsubst%,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))+TEST_PROGRAMS_NEED_X_SRC+=$(TEST_PROGRAMS_NEED_X:%=t/helper/%.c)+TEST_PROGRAMS+=$(TEST_PROGRAMS_NEED_X_SRC:%.c=%$X)+TEST_BUILTINS_SRC+=$(filter-out$(TEST_PROGRAMS_NEED_X_SRC),$(wildcardt/helper/*.c))+TEST_BUILTINS_OBJS+=$(TEST_BUILTINS_SRC:%.c=%.o)-# List built-in command $C whose implementation cmd_$C() is not in-# builtin/$C.o but is linked in as part of some other command.+# List built-in command $C whose implementation cmd_$C() is in+# builtin/$C.o+BUILTIN_OBJS=$(patsubst%.c,%.o,$(wildcardbuiltin/*.c))BUILT_INS+=$(patsubstbuiltin/%.o,git-%$X,$(BUILTIN_OBJS))+# List built-in command $C whose implementation cmd_$C() is not in+# builtin/$C.o but is linked in as part of some other command.BUILT_INS+=git-cherry$XBUILT_INS+=git-cherry-pick$XBUILT_INS+=git-format-patch$X
@@ -837,355 +786,28 @@ LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentat-nameDocumentation-prune-o\-name'*.h'-print)))-LIB_OBJS+=abspath.o-LIB_OBJS+=add-interactive.o-LIB_OBJS+=add-patch.o-LIB_OBJS+=advice.o-LIB_OBJS+=alias.o-LIB_OBJS+=alloc.o-LIB_OBJS+=apply.o-LIB_OBJS+=archive-tar.o-LIB_OBJS+=archive-zip.o-LIB_OBJS+=archive.o-LIB_OBJS+=attr.o-LIB_OBJS+=base85.o-LIB_OBJS+=bisect.o-LIB_OBJS+=blame.o-LIB_OBJS+=blob.o-LIB_OBJS+=bloom.o-LIB_OBJS+=branch.o-LIB_OBJS+=bulk-checkin.o-LIB_OBJS+=bundle.o-LIB_OBJS+=cache-tree.o-LIB_OBJS+=cbtree.o-LIB_OBJS+=chdir-notify.o-LIB_OBJS+=checkout.o-LIB_OBJS+=chunk-format.o-LIB_OBJS+=color.o-LIB_OBJS+=column.o-LIB_OBJS+=combine-diff.o-LIB_OBJS+=commit-graph.o-LIB_OBJS+=commit-reach.o-LIB_OBJS+=commit.o+# LIB_OBJS: compat/* objects that live at the top-level+CONDITIONAL_OBJS+=unix-socket.o+CONDITIONAL_OBJS+=unix-stream-server.o+CONDITIONAL_OBJS+=sha1dc_git.o++# LIB_OBJS: Mostly glob *.c at the top-level, with some exlusions+LIB_OBJS+=$(filter-out\+$(CONDITIONAL_OBJS)\+git.ocommon-main.o$(PROGRAM_OBJS)\+$(FUZZ_OBJS)$(CURL_OBJS),\+$(patsubst%.c,%.o,$(wildcard*.c)))++# LIB_OBJS: Directories that contain only LIB_OBJS+LIB_OBJS_DIRS+=ewah+LIB_OBJS_DIRS+=negotiator+LIB_OBJS_DIRS+=refs+LIB_OBJS_DIRS+=trace2+LIB_OBJS+=$(patsubst%.c,%.o,$(wildcard$(addsuffix/*.c,$(LIB_OBJS_DIRS))))++# LIB_OBJS: unconditional compat/* objectsLIB_OBJS+=compat/obstack.oLIB_OBJS+=compat/terminal.o-LIB_OBJS+=config.o-LIB_OBJS+=connect.o-LIB_OBJS+=connected.o-LIB_OBJS+=convert.o-LIB_OBJS+=copy.o-LIB_OBJS+=credential.o-LIB_OBJS+=csum-file.o-LIB_OBJS+=ctype.o-LIB_OBJS+=date.o-LIB_OBJS+=decorate.o-LIB_OBJS+=delta-islands.o-LIB_OBJS+=diff-delta.o-LIB_OBJS+=diff-merges.o-LIB_OBJS+=diff-lib.o-LIB_OBJS+=diff-no-index.o-LIB_OBJS+=diff.o-LIB_OBJS+=diffcore-break.o-LIB_OBJS+=diffcore-delta.o-LIB_OBJS+=diffcore-order.o-LIB_OBJS+=diffcore-pickaxe.o-LIB_OBJS+=diffcore-rename.o-LIB_OBJS+=diffcore-rotate.o-LIB_OBJS+=dir-iterator.o-LIB_OBJS+=dir.o-LIB_OBJS+=editor.o-LIB_OBJS+=entry.o-LIB_OBJS+=environment.o-LIB_OBJS+=ewah/bitmap.o-LIB_OBJS+=ewah/ewah_bitmap.o-LIB_OBJS+=ewah/ewah_io.o-LIB_OBJS+=ewah/ewah_rlw.o-LIB_OBJS+=exec-cmd.o-LIB_OBJS+=fetch-negotiator.o-LIB_OBJS+=fetch-pack.o-LIB_OBJS+=fmt-merge-msg.o-LIB_OBJS+=fsck.o-LIB_OBJS+=fsmonitor.o-LIB_OBJS+=gettext.o-LIB_OBJS+=gpg-interface.o-LIB_OBJS+=graph.o-LIB_OBJS+=grep.o-LIB_OBJS+=hash-lookup.o-LIB_OBJS+=hashmap.o-LIB_OBJS+=help.o-LIB_OBJS+=hex.o-LIB_OBJS+=hook.o-LIB_OBJS+=ident.o-LIB_OBJS+=json-writer.o-LIB_OBJS+=kwset.o-LIB_OBJS+=levenshtein.o-LIB_OBJS+=line-log.o-LIB_OBJS+=line-range.o-LIB_OBJS+=linear-assignment.o-LIB_OBJS+=list-objects-filter-options.o-LIB_OBJS+=list-objects-filter.o-LIB_OBJS+=list-objects.o-LIB_OBJS+=ll-merge.o-LIB_OBJS+=lockfile.o-LIB_OBJS+=log-tree.o-LIB_OBJS+=ls-refs.o-LIB_OBJS+=mailinfo.o-LIB_OBJS+=mailmap.o-LIB_OBJS+=match-trees.o-LIB_OBJS+=mem-pool.o-LIB_OBJS+=merge-blobs.o-LIB_OBJS+=merge-ort.o-LIB_OBJS+=merge-ort-wrappers.o-LIB_OBJS+=merge-recursive.o-LIB_OBJS+=merge.o-LIB_OBJS+=mergesort.o-LIB_OBJS+=midx.o-LIB_OBJS+=name-hash.o-LIB_OBJS+=negotiator/default.o-LIB_OBJS+=negotiator/noop.o-LIB_OBJS+=negotiator/skipping.o-LIB_OBJS+=notes-cache.o-LIB_OBJS+=notes-merge.o-LIB_OBJS+=notes-utils.o-LIB_OBJS+=notes.o-LIB_OBJS+=object-file.o-LIB_OBJS+=object-name.o-LIB_OBJS+=object.o-LIB_OBJS+=oid-array.o-LIB_OBJS+=oidmap.o-LIB_OBJS+=oidset.o-LIB_OBJS+=oidtree.o-LIB_OBJS+=pack-bitmap-write.o-LIB_OBJS+=pack-bitmap.o-LIB_OBJS+=pack-check.o-LIB_OBJS+=pack-objects.o-LIB_OBJS+=pack-revindex.o-LIB_OBJS+=pack-write.o-LIB_OBJS+=packfile.o-LIB_OBJS+=pager.o-LIB_OBJS+=parallel-checkout.o-LIB_OBJS+=parse-options-cb.o-LIB_OBJS+=parse-options.o-LIB_OBJS+=patch-delta.o-LIB_OBJS+=patch-ids.o-LIB_OBJS+=path.o-LIB_OBJS+=pathspec.o-LIB_OBJS+=pkt-line.o-LIB_OBJS+=preload-index.o-LIB_OBJS+=pretty.o-LIB_OBJS+=prio-queue.o-LIB_OBJS+=progress.o-LIB_OBJS+=promisor-remote.o-LIB_OBJS+=prompt.o-LIB_OBJS+=protocol.o-LIB_OBJS+=protocol-caps.o-LIB_OBJS+=prune-packed.o-LIB_OBJS+=quote.o-LIB_OBJS+=range-diff.o-LIB_OBJS+=reachable.o-LIB_OBJS+=read-cache.o-LIB_OBJS+=rebase-interactive.o-LIB_OBJS+=rebase.o-LIB_OBJS+=ref-filter.o-LIB_OBJS+=reflog-walk.o-LIB_OBJS+=refs.o-LIB_OBJS+=refs/debug.o-LIB_OBJS+=refs/files-backend.o-LIB_OBJS+=refs/iterator.o-LIB_OBJS+=refs/packed-backend.o-LIB_OBJS+=refs/ref-cache.o-LIB_OBJS+=refspec.o-LIB_OBJS+=remote.o-LIB_OBJS+=replace-object.o-LIB_OBJS+=repo-settings.o-LIB_OBJS+=repository.o-LIB_OBJS+=rerere.o-LIB_OBJS+=reset.o-LIB_OBJS+=resolve-undo.o-LIB_OBJS+=revision.o-LIB_OBJS+=run-command.o-LIB_OBJS+=send-pack.o-LIB_OBJS+=sequencer.o-LIB_OBJS+=serve.o-LIB_OBJS+=server-info.o-LIB_OBJS+=setup.o-LIB_OBJS+=shallow.o-LIB_OBJS+=sideband.o-LIB_OBJS+=sigchain.o-LIB_OBJS+=sparse-index.o-LIB_OBJS+=split-index.o-LIB_OBJS+=stable-qsort.o-LIB_OBJS+=strbuf.o-LIB_OBJS+=streaming.o-LIB_OBJS+=string-list.o-LIB_OBJS+=strmap.o-LIB_OBJS+=strvec.o-LIB_OBJS+=sub-process.o-LIB_OBJS+=submodule-config.o-LIB_OBJS+=submodule.o-LIB_OBJS+=symlinks.o-LIB_OBJS+=tag.o-LIB_OBJS+=tempfile.o-LIB_OBJS+=thread-utils.o-LIB_OBJS+=tmp-objdir.o-LIB_OBJS+=trace.o-LIB_OBJS+=trace2.o-LIB_OBJS+=trace2/tr2_cfg.o-LIB_OBJS+=trace2/tr2_cmd_name.o-LIB_OBJS+=trace2/tr2_dst.o-LIB_OBJS+=trace2/tr2_sid.o-LIB_OBJS+=trace2/tr2_sysenv.o-LIB_OBJS+=trace2/tr2_tbuf.o-LIB_OBJS+=trace2/tr2_tgt_event.o-LIB_OBJS+=trace2/tr2_tgt_normal.o-LIB_OBJS+=trace2/tr2_tgt_perf.o-LIB_OBJS+=trace2/tr2_tls.o-LIB_OBJS+=trailer.o-LIB_OBJS+=transport-helper.o-LIB_OBJS+=transport.o-LIB_OBJS+=tree-diff.o-LIB_OBJS+=tree-walk.o-LIB_OBJS+=tree.o-LIB_OBJS+=unpack-trees.o-LIB_OBJS+=upload-pack.o-LIB_OBJS+=url.o-LIB_OBJS+=urlmatch.o-LIB_OBJS+=usage.o-LIB_OBJS+=userdiff.o-LIB_OBJS+=utf8.o-LIB_OBJS+=varint.o-LIB_OBJS+=version.o-LIB_OBJS+=versioncmp.o-LIB_OBJS+=walker.o-LIB_OBJS+=wildmatch.o-LIB_OBJS+=worktree.o-LIB_OBJS+=wrapper.o-LIB_OBJS+=write-or-die.o-LIB_OBJS+=ws.o-LIB_OBJS+=wt-status.o-LIB_OBJS+=xdiff-interface.o-LIB_OBJS+=zlib.o--BUILTIN_OBJS+=builtin/add.o-BUILTIN_OBJS+=builtin/am.o-BUILTIN_OBJS+=builtin/annotate.o-BUILTIN_OBJS+=builtin/apply.o-BUILTIN_OBJS+=builtin/archive.o-BUILTIN_OBJS+=builtin/bisect--helper.o-BUILTIN_OBJS+=builtin/blame.o-BUILTIN_OBJS+=builtin/branch.o-BUILTIN_OBJS+=builtin/bugreport.o-BUILTIN_OBJS+=builtin/bundle.o-BUILTIN_OBJS+=builtin/cat-file.o-BUILTIN_OBJS+=builtin/check-attr.o-BUILTIN_OBJS+=builtin/check-ignore.o-BUILTIN_OBJS+=builtin/check-mailmap.o-BUILTIN_OBJS+=builtin/check-ref-format.o-BUILTIN_OBJS+=builtin/checkout--worker.o-BUILTIN_OBJS+=builtin/checkout-index.o-BUILTIN_OBJS+=builtin/checkout.o-BUILTIN_OBJS+=builtin/clean.o-BUILTIN_OBJS+=builtin/clone.o-BUILTIN_OBJS+=builtin/column.o-BUILTIN_OBJS+=builtin/commit-graph.o-BUILTIN_OBJS+=builtin/commit-tree.o-BUILTIN_OBJS+=builtin/commit.o-BUILTIN_OBJS+=builtin/config.o-BUILTIN_OBJS+=builtin/count-objects.o-BUILTIN_OBJS+=builtin/credential-cache--daemon.o-BUILTIN_OBJS+=builtin/credential-cache.o-BUILTIN_OBJS+=builtin/credential-store.o-BUILTIN_OBJS+=builtin/credential.o-BUILTIN_OBJS+=builtin/describe.o-BUILTIN_OBJS+=builtin/diff-files.o-BUILTIN_OBJS+=builtin/diff-index.o-BUILTIN_OBJS+=builtin/diff-tree.o-BUILTIN_OBJS+=builtin/diff.o-BUILTIN_OBJS+=builtin/difftool.o-BUILTIN_OBJS+=builtin/env--helper.o-BUILTIN_OBJS+=builtin/fast-export.o-BUILTIN_OBJS+=builtin/fast-import.o-BUILTIN_OBJS+=builtin/fetch-pack.o-BUILTIN_OBJS+=builtin/fetch.o-BUILTIN_OBJS+=builtin/fmt-merge-msg.o-BUILTIN_OBJS+=builtin/for-each-ref.o-BUILTIN_OBJS+=builtin/for-each-repo.o-BUILTIN_OBJS+=builtin/fsck.o-BUILTIN_OBJS+=builtin/gc.o-BUILTIN_OBJS+=builtin/get-tar-commit-id.o-BUILTIN_OBJS+=builtin/grep.o-BUILTIN_OBJS+=builtin/hash-object.o-BUILTIN_OBJS+=builtin/help.o-BUILTIN_OBJS+=builtin/index-pack.o-BUILTIN_OBJS+=builtin/init-db.o-BUILTIN_OBJS+=builtin/interpret-trailers.o-BUILTIN_OBJS+=builtin/log.o-BUILTIN_OBJS+=builtin/ls-files.o-BUILTIN_OBJS+=builtin/ls-remote.o-BUILTIN_OBJS+=builtin/ls-tree.o-BUILTIN_OBJS+=builtin/mailinfo.o-BUILTIN_OBJS+=builtin/mailsplit.o-BUILTIN_OBJS+=builtin/merge-base.o-BUILTIN_OBJS+=builtin/merge-file.o-BUILTIN_OBJS+=builtin/merge-index.o-BUILTIN_OBJS+=builtin/merge-ours.o-BUILTIN_OBJS+=builtin/merge-recursive.o-BUILTIN_OBJS+=builtin/merge-tree.o-BUILTIN_OBJS+=builtin/merge.o-BUILTIN_OBJS+=builtin/mktag.o-BUILTIN_OBJS+=builtin/mktree.o-BUILTIN_OBJS+=builtin/multi-pack-index.o-BUILTIN_OBJS+=builtin/mv.o-BUILTIN_OBJS+=builtin/name-rev.o-BUILTIN_OBJS+=builtin/notes.o-BUILTIN_OBJS+=builtin/pack-objects.o-BUILTIN_OBJS+=builtin/pack-redundant.o-BUILTIN_OBJS+=builtin/pack-refs.o-BUILTIN_OBJS+=builtin/patch-id.o-BUILTIN_OBJS+=builtin/prune-packed.o-BUILTIN_OBJS+=builtin/prune.o-BUILTIN_OBJS+=builtin/pull.o-BUILTIN_OBJS+=builtin/push.o-BUILTIN_OBJS+=builtin/range-diff.o-BUILTIN_OBJS+=builtin/read-tree.o-BUILTIN_OBJS+=builtin/rebase.o-BUILTIN_OBJS+=builtin/receive-pack.o-BUILTIN_OBJS+=builtin/reflog.o-BUILTIN_OBJS+=builtin/remote-ext.o-BUILTIN_OBJS+=builtin/remote-fd.o-BUILTIN_OBJS+=builtin/remote.o-BUILTIN_OBJS+=builtin/repack.o-BUILTIN_OBJS+=builtin/replace.o-BUILTIN_OBJS+=builtin/rerere.o-BUILTIN_OBJS+=builtin/reset.o-BUILTIN_OBJS+=builtin/rev-list.o-BUILTIN_OBJS+=builtin/rev-parse.o-BUILTIN_OBJS+=builtin/revert.o-BUILTIN_OBJS+=builtin/rm.o-BUILTIN_OBJS+=builtin/send-pack.o-BUILTIN_OBJS+=builtin/shortlog.o-BUILTIN_OBJS+=builtin/show-branch.o-BUILTIN_OBJS+=builtin/show-index.o-BUILTIN_OBJS+=builtin/show-ref.o-BUILTIN_OBJS+=builtin/sparse-checkout.o-BUILTIN_OBJS+=builtin/stash.o-BUILTIN_OBJS+=builtin/stripspace.o-BUILTIN_OBJS+=builtin/submodule--helper.o-BUILTIN_OBJS+=builtin/symbolic-ref.o-BUILTIN_OBJS+=builtin/tag.o-BUILTIN_OBJS+=builtin/unpack-file.o-BUILTIN_OBJS+=builtin/unpack-objects.o-BUILTIN_OBJS+=builtin/update-index.o-BUILTIN_OBJS+=builtin/update-ref.o-BUILTIN_OBJS+=builtin/update-server-info.o-BUILTIN_OBJS+=builtin/upload-archive.o-BUILTIN_OBJS+=builtin/upload-pack.o-BUILTIN_OBJS+=builtin/var.o-BUILTIN_OBJS+=builtin/verify-commit.o-BUILTIN_OBJS+=builtin/verify-pack.o-BUILTIN_OBJS+=builtin/verify-tag.o-BUILTIN_OBJS+=builtin/worktree.o-BUILTIN_OBJS+=builtin/write-tree.o # THIRD_PARTY_SOURCES is a list of patterns compatible with the # $(filter) and $(filter-out) family of functions. They specify source
@@ -2076,6 +1698,7 @@ endifLIBS=$(filter-out%.o,$(GITLIBS))$(EXTLIBS)BASIC_CFLAGS+=$(COMPAT_CFLAGS)+LIB_OBJS_NO_COMPAT:=$(LIB_OBJS)LIB_OBJS+=$(COMPAT_OBJS) # Quote for C
@@ -2436,17 +2059,12 @@ reconfigure config.mak.autogen: config.status.PHONY:reconfigure# This is a convenience target. endif-XDIFF_OBJS+=xdiff/xdiffi.o-XDIFF_OBJS+=xdiff/xemit.o-XDIFF_OBJS+=xdiff/xhistogram.o-XDIFF_OBJS+=xdiff/xmerge.o-XDIFF_OBJS+=xdiff/xpatience.o-XDIFF_OBJS+=xdiff/xprepare.o-XDIFF_OBJS+=xdiff/xutils.o+XDIFF_SRC+=$(wildcardxdiff/*.c)+XDIFF_OBJS+=$(XDIFF_SRC:.c=.o).PHONY:xdiff-objsxdiff-objs:$(XDIFF_OBJS)-TEST_OBJS:=$(patsubst%$X,%.o,$(TEST_PROGRAMS))$(patsubst%,t/helper/%,$(TEST_BUILTINS_OBJS))+TEST_OBJS:=$(patsubst%$X,%.o,$(TEST_PROGRAMS))$(TEST_BUILTINS_OBJS).PHONY:test-objstest-objs:$(TEST_OBJS)
@@ -111,32 +111,15 @@ project(git#TODO Add pcre support#macros for parsing the Makefile for sources and scripts-macro(parse_makefile_for_sourceslist_varregex)-file(STRINGS${CMAKE_SOURCE_DIR}/Makefile${list_var}REGEX"^${regex} \\+=(.*)")-string(REPLACE"${regex} +="""${list_var}${${list_var}})-string(REPLACE"$(COMPAT_OBJS)"""${list_var}${${list_var}})#remove "$(COMPAT_OBJS)" This is only for libgit.-string(STRIP${${list_var}}${list_var})#remove trailing/leading whitespaces-string(REPLACE".o"".c;"${list_var}${${list_var}})#change .o to .c, ; is for converting the string into a list-list(TRANSFORM${list_var}STRIP)#remove trailing/leading whitespaces for each element in list-list(REMOVE_ITEM${list_var}"")#remove empty list elements-endmacro()--macro(parse_makefile_for_scriptslist_varregexlang)-file(STRINGS${CMAKE_SOURCE_DIR}/Makefile${list_var}REGEX"^${regex} \\+=(.*)")-string(REPLACE"${regex} +="""${list_var}${${list_var}})-string(STRIP${${list_var}}${list_var})#remove trailing/leading whitespaces-string(REPLACE" "";"${list_var}${${list_var}})#convert string to a list-if(NOT${lang})#exclude for SCRIPT_LIB-list(TRANSFORM${list_var}REPLACE"${lang}""")#do the replacement-endif()-endmacro()--macro(parse_makefile_for_executableslist_varregex)-file(STRINGS${CMAKE_SOURCE_DIR}/Makefile${list_var}REGEX"^${regex} \\+= git-(.*)")-string(REPLACE"${regex} +="""${list_var}${${list_var}})+macro(ask_makefile_for_listlist_varvar_name)+execute_process(COMMANDmakeprint-list-${var_name}+WORKING_DIRECTORY${CMAKE_SOURCE_DIR}+OUTPUT_VARIABLE${list_var})+string(REGEXREPLACE"\\.o\n"".c\n"${list_var}${${list_var}})#change .o to .cstring(STRIP${${list_var}}${list_var})#remove trailing/leading whitespaces-string(REPLACE"git-"""${list_var}${${list_var}})#strip `git-` prefix-string(REPLACE"\$X"";"${list_var}${${list_var}})#strip $X, ; is for converting the string into a list+## Parse the Makefile print-list-% format+string(REGEXREPLACE"${var_name} =\n"""${list_var}${${list_var}})+string(REGEXREPLACE"${var_name} \\+?= ([^\n]+)""\\1;"${list_var}${${list_var}})list(TRANSFORM${list_var}STRIP)#remove trailing/leading whitespaces for each element in listlist(REMOVE_ITEM${list_var}"")#remove empty list elementsendmacro()
@@ -729,7 +712,9 @@ if(CURL_FOUND)endif()endif()-parse_makefile_for_executables(git_builtin_extra"BUILT_INS")+ask_makefile_for_list(git_builtin_extra"BUILT_INS")+list(TRANSFORMgit_builtin_extraREPLACE"^git-(.*)""\\1")#strip `git-` prefix+list(TRANSFORMgit_builtin_extraREPLACE"\.exe$""")#strip $Xoption(SKIP_DASHED_BUILT_INS"Skip hardlinking the dashed versions of the built-ins")
Hi Ævar
On 01/11/2021 19:19, Ævar Arnfjörð Bjarmason wrote:
quoted
Remove the hardcoded lists of objects in favor of using
$(wildcard). This means that every time a built-in, test tool etc. is
added we won't need to patch the top-level Makefile, except for the
few remaining cases where the asset in question would make it onto one
of our list of exceptions.
Ever since 81b50f3ce40 (Move 'builtin-*' into a 'builtin/'
subdirectory, 2010-02-22) this has been relatively easy to do (and
even before that we could glob builtin-*.c). This pattern of
exhaustively enumerating files was then carried forward for
e.g. TEST_BUILTINS_OBJS in efd71f8913a (t/helper: add an empty
test-tool program, 2018-03-24).
One reason not to do this is that now a new *.c file at the
top-level
will be immediately picked up, so if a new *.c file is being worked on
"make" will error if it doesn't compile, whereas before that file
would need to be explicitly listed in the Makefile. I think than small
trade-off is worth it.
If I need to split up some uncommitted changes into several commits
and I know it is going to be fiddly to do so I will sometimes copy the
original file to foo.safe.c and then edit foo.c to create each
commit. Then I can easily compile and test each commit and editing the
file directly is often easier than using add -p and editing the
hunks. With this patch running make will fail in that case I think.
Would being able to do something like:
make EXCLUDE_WILDCARD=foo.safe.c
Satisfy this use-case?
We can also have some DEVOPTS knob so we'll prune out files found if a
$(shell)-out to "git status" tells us they're untracked. I wouldn't mind
that (and could implement it) if it was optional.
Also note that you've got some of this already, e.g. we'll pick up *.h
files via a glob for "make TAGS", the dependency graph etc.
quoted
We could make this simpler still for the Makefile by moving
"unix-socket.c" etc. to e.g. a "conditional-src/" directory, likewise
for $(PROGRAM_OBJS) to e.g. "programs/". If we did that we would not
need the "$(filter-out)" for LIB_OBJS. I don't think that's worth it,
e.g. due to "git log -- <path>" on the files now needing a "--follow".
There's a few small "while we're at it" changes here, since I'm
touching the code in question:
- Start splitting up the the "Guard against the environment"
section
at the top, but don't move anything that exists there out to avoid
merge conflicts
- The $(TEST_BUILTINS_OBJS) variable was needlessly complex,
because
it didn't have the full paths we'd pathsubst it back & forth.
- Introduce *_SRC in addition to *_OBJ for the variable I'm
touching. Eventually we'll want to do this for all the *.o files,
i.e. make the *.c list a source of truth for *.o, which means we can
e.g. use that exhaustive list for "make TAGS".
- Add a missing "curl-objs" target. See 029bac01a87 (Makefile: add
{program,xdiff,test,git,fuzz}-objs & objects targets, 2021-02-23)
for the commit that added the rest.
- De-indent an "ifndef" block, we don't usually indent their
contents.
On the CMake changes here:
- When CMake support was introduced in was introduced
061c2240b1b (Introduce CMake support for configuring Git, 2020-06-12)
there was a discussion about the maintenance burden of maintaining the
top-level Makefile in parallel with CMakeLists.txt[1] where reviewers
were assured that doing so would simply be a matter of adding something
to a list in the CMake recipe.
Between change and some recent changes of mine where the
"vs-build"
job failed to a divergence between the Makefile and CMakeList.txt I
can confidently say that that doesn't at all match reality. Even
seemingly trivial changes to the Makefile like this one are forcing
us to do a deep-dive into CMake internals to make forward progress
with our main build system.
My recollection is that the discussions were about not having to touch
CMakeList.txt when adding new files to the build and I think that
largely works. I don't think a lot of the changes you have been making
recently were anticipated in that discussion.
That's part of it, but the concern about needing to maintain two systems
in perpetuity was also brought up, and it not being a hard dependency,
having that vs-{build,test} job soft-fail on it etc. were brought up but
that's not what we've got now.
In any case, having to maintain two build systems and the maintainer(s)
of the CMake being inactive one is the situation we're in now.
quoted
- The promised "We can add a (continue-on-error) to vs-build job to
make this process less of a hindrance." in [2] never materialized.
Since 4c2c38e800f (ci: modification of main.yml to use cmake for
vs-build job, 2020-06-26) got a hard dependency on CMake as far as
getting the CI to pass goes.
- The "vs-build" CI doesn't actually require that there be no GNU
make
usage in the job, as it itself has a hard dependency on running a
"make -n artifacts-tar" command. So as far as any vs-specific special-sauce
goes we don't need a GNU-make free build system for vs-build.
We need GNU-make for the ci job but an individual developer using
CMake does not need GNU-make installed. On linux it is possible to
build git without having make installed by using cmake and ninja [1]
I'd tried that, but didn't know about your [1] topic. If you or someone
is actively willing to fix things up in CMake....
quoted
- The stated goal in 061c2240b1b of avoiding a GNU make dependency
for developer because it requires an SDK that "occupies around two
gigabytes" and "three quarters of a gigabyte worth of Git objects"
hardly seems worthwhile trade-off given the above. Disk space is cheap,
developer time required to maintain two parallel build systems isn't.
That rather assumes everyone has plenty of disk space and a decent
network connection.
I realize that there's going to be a person who's got 1.5GB of disk
space and needs to delete a movie they've downloaded or whatever. I just
think that's worth it v.s. maintenance trade-off.
Doesn't just a Windows base installation need something in the tens of
GB of disk space these days? We're not really talking about embedded
systems.
Just the gcc etc. I've got on my Debian box is approaching a
GB. Whatever we think about other trade-offs optimizing for disk space
doesn't seem very compelling.
quoted
My attempt to amend/revert 4c2c38e800f to have it use the
pre-4c2c38e800f "make" invocation as a fallback failed, partially
because I don't have a Windows development environment, so any attempt
to change it is a painfully slow round-trip to GitHub CI.
Let's instead have CMake call out to the Makefile asking it what the
definition of various variables lists is, rather than being forced to
maintain those lists in a way that CMake can parse with regexes (which
precludes anything but a giant hardcoded list).
I could familiarize myself enough with CMake to do this in some
CMake-native way, but that would take "just as long as adding it to
the Makefile"[2] (I think that took me <5 minutes, but I'm several
hours into fighting with CMake)
So I consider this both a bugfix to the stated aims of this CMake
integration, and a better way forward for having an alternate build
system. I.e. If someone really does care about a having a
GNU-make-less dependency for the "vs-build" I think this change offers
a much better way forward for that.
I don't see how relying on GNU-make is a step forward for the CMake
integration when it works without it now.
It's not if we assume we've got infinite man hours to maintain these
systems, but we don't.
I have some pending patches to make various common cases when using make
much better, mainly speeding up no-op runs so things in rebase --exec go
faster.
So far I've been submitting the parts of that that don't step on the
toes of this cmake integration, and realistically if I've got to
implement everything in lockstep in two systems I'll probably just give
up on it.
Hence asking if there's some middle ground we can find here.
So you don't want to install "make" on Windows, but how about if we had
a script in contrib/ that generated these extractions of lists from the
Makefile instead of doing it on the fly, we could even commit those to
the repo.
Then I'd effectively get what I'm aiming for here, and cmake users could
just re-run that script, and if one of them did they could push the
result somewhere, and others could just fetch the generated assets.
Overall I'm don't think that moving from a known set of dependencies
to "build whatever C files are lying around in this directory" is an
improvement.
The caveat you note above with "foo.safe.c" is something we've got
already, see various "$(wildcard)", "find" and "git ls-files" in the
Makefile. So this way we'll at least be consistent. Now we'll add stray
files to TAGS, apply "coccicheck" to them etc.
So one thing I was aiming for here was closing that gap.
I do tihnk having a well understood hierarchy does help a lot, since you
can know that t/helpers/*.c is always one sort of thing etc.
On 06/11/2021 14:27, Ævar Arnfjörð Bjarmason wrote:
On Sat, Nov 06 2021, Phillip Wood wrote:
quoted
Hi Ævar
On 01/11/2021 19:19, Ævar Arnfjörð Bjarmason wrote:
quoted
Remove the hardcoded lists of objects in favor of using
$(wildcard). This means that every time a built-in, test tool etc. is
added we won't need to patch the top-level Makefile, except for the
few remaining cases where the asset in question would make it onto one
of our list of exceptions.
Ever since 81b50f3ce40 (Move 'builtin-*' into a 'builtin/'
subdirectory, 2010-02-22) this has been relatively easy to do (and
even before that we could glob builtin-*.c). This pattern of
exhaustively enumerating files was then carried forward for
e.g. TEST_BUILTINS_OBJS in efd71f8913a (t/helper: add an empty
test-tool program, 2018-03-24).
One reason not to do this is that now a new *.c file at the
top-level
will be immediately picked up, so if a new *.c file is being worked on
"make" will error if it doesn't compile, whereas before that file
would need to be explicitly listed in the Makefile. I think than small
trade-off is worth it.
If I need to split up some uncommitted changes into several commits
and I know it is going to be fiddly to do so I will sometimes copy the
original file to foo.safe.c and then edit foo.c to create each
commit. Then I can easily compile and test each commit and editing the
file directly is often easier than using add -p and editing the
hunks. With this patch running make will fail in that case I think.
Would being able to do something like:
make EXCLUDE_WILDCARD=foo.safe.c
Satisfy this use-case?
We can also have some DEVOPTS knob so we'll prune out files found if a
$(shell)-out to "git status" tells us they're untracked. I wouldn't mind
that (and could implement it) if it was optional.
Also note that you've got some of this already, e.g. we'll pick up *.h
files via a glob for "make TAGS", the dependency graph etc.
I'd be happier using 'git ls-files' with a glob if we need to move away
from listing the files explicitly rather than having to pass some
exclude list when running make. Having seen your comments below about
ls-files/find I had a look at the Makefile and they always seem to be
used together as "git ls-files ... || find ...". Doing that would mean
we wouldn't try to build any untracked files but still find everything
in a tarball.
quoted
quoted
We could make this simpler still for the Makefile by moving
"unix-socket.c" etc. to e.g. a "conditional-src/" directory, likewise
for $(PROGRAM_OBJS) to e.g. "programs/". If we did that we would not
need the "$(filter-out)" for LIB_OBJS. I don't think that's worth it,
e.g. due to "git log -- <path>" on the files now needing a "--follow".
There's a few small "while we're at it" changes here, since I'm
touching the code in question:
- Start splitting up the the "Guard against the environment"
section
at the top, but don't move anything that exists there out to avoid
merge conflicts
- The $(TEST_BUILTINS_OBJS) variable was needlessly complex,
because
it didn't have the full paths we'd pathsubst it back & forth.
- Introduce *_SRC in addition to *_OBJ for the variable I'm
touching. Eventually we'll want to do this for all the *.o files,
i.e. make the *.c list a source of truth for *.o, which means we can
e.g. use that exhaustive list for "make TAGS".
- Add a missing "curl-objs" target. See 029bac01a87 (Makefile: add
{program,xdiff,test,git,fuzz}-objs & objects targets, 2021-02-23)
for the commit that added the rest.
- De-indent an "ifndef" block, we don't usually indent their
contents.
On the CMake changes here:
- When CMake support was introduced in was introduced
061c2240b1b (Introduce CMake support for configuring Git, 2020-06-12)
there was a discussion about the maintenance burden of maintaining the
top-level Makefile in parallel with CMakeLists.txt[1] where reviewers
were assured that doing so would simply be a matter of adding something
to a list in the CMake recipe.
Between change and some recent changes of mine where the
"vs-build"
job failed to a divergence between the Makefile and CMakeList.txt I
can confidently say that that doesn't at all match reality. Even
seemingly trivial changes to the Makefile like this one are forcing
us to do a deep-dive into CMake internals to make forward progress
with our main build system.
My recollection is that the discussions were about not having to touch
CMakeList.txt when adding new files to the build and I think that
largely works. I don't think a lot of the changes you have been making
recently were anticipated in that discussion.
That's part of it, but the concern about needing to maintain two systems
in perpetuity was also brought up, and it not being a hard dependency,
having that vs-{build,test} job soft-fail on it etc. were brought up but
that's not what we've got now.
In any case, having to maintain two build systems and the maintainer(s)
of the CMake being inactive one is the situation we're in now.
quoted
quoted
- The promised "We can add a (continue-on-error) to vs-build job to
make this process less of a hindrance." in [2] never materialized.
Since 4c2c38e800f (ci: modification of main.yml to use cmake for
vs-build job, 2020-06-26) got a hard dependency on CMake as far as
getting the CI to pass goes.
- The "vs-build" CI doesn't actually require that there be no GNU
make
usage in the job, as it itself has a hard dependency on running a
"make -n artifacts-tar" command. So as far as any vs-specific special-sauce
goes we don't need a GNU-make free build system for vs-build.
We need GNU-make for the ci job but an individual developer using
CMake does not need GNU-make installed. On linux it is possible to
build git without having make installed by using cmake and ninja [1]
I'd tried that, but didn't know about your [1] topic. If you or someone
is actively willing to fix things up in CMake....
I only started working on that a few days ago. I was interested to see
if using ninja was faster for building when just a few files have been
changed but it doesn't make much difference unless absolutely nothing
has changed - I guess we don't have that many files in the grand scheme
of things. The CMake support still lags the Makefile as currently things
like GIT-VERSION-FILE and git-hooks.h are not regenerated when their
inputs change. I'm not sure if I want to commit to doing much more on
that at the moment.
quoted
quoted
- The stated goal in 061c2240b1b of avoiding a GNU make dependency
for developer because it requires an SDK that "occupies around two
gigabytes" and "three quarters of a gigabyte worth of Git objects"
hardly seems worthwhile trade-off given the above. Disk space is cheap,
developer time required to maintain two parallel build systems isn't.
That rather assumes everyone has plenty of disk space and a decent
network connection.
I realize that there's going to be a person who's got 1.5GB of disk
space and needs to delete a movie they've downloaded or whatever. I just
think that's worth it v.s. maintenance trade-off.
Doesn't just a Windows base installation need something in the tens of
GB of disk space these days? We're not really talking about embedded
systems.
Just the gcc etc. I've got on my Debian box is approaching a
GB. Whatever we think about other trade-offs optimizing for disk space
doesn't seem very compelling.
quoted
quoted
My attempt to amend/revert 4c2c38e800f to have it use the
pre-4c2c38e800f "make" invocation as a fallback failed, partially
because I don't have a Windows development environment, so any attempt
to change it is a painfully slow round-trip to GitHub CI.
Let's instead have CMake call out to the Makefile asking it what the
definition of various variables lists is, rather than being forced to
maintain those lists in a way that CMake can parse with regexes (which
precludes anything but a giant hardcoded list).
I could familiarize myself enough with CMake to do this in some
CMake-native way, but that would take "just as long as adding it to
the Makefile"[2] (I think that took me <5 minutes, but I'm several
hours into fighting with CMake)
So I consider this both a bugfix to the stated aims of this CMake
integration, and a better way forward for having an alternate build
system. I.e. If someone really does care about a having a
GNU-make-less dependency for the "vs-build" I think this change offers
a much better way forward for that.
I don't see how relying on GNU-make is a step forward for the CMake
integration when it works without it now.
It's not if we assume we've got infinite man hours to maintain these
systems, but we don't.
I have some pending patches to make various common cases when using make
much better, mainly speeding up no-op runs so things in rebase --exec go
faster.
So far I've been submitting the parts of that that don't step on the
toes of this cmake integration, and realistically if I've got to
implement everything in lockstep in two systems I'll probably just give
up on it.
Hence asking if there's some middle ground we can find here.
What is it about listing the input files explicitly that slows things
down? Surely it's faster than globbing the filesystem.
So you don't want to install "make" on Windows, but how about if we had
a script in contrib/ that generated these extractions of lists from the
Makefile instead of doing it on the fly, we could even commit those to
the repo.
If I understand there would be some make rule that generates a list of
dependencies and we'd commit that list to the repo and consume it in
CMakeLists.txt. I'd be fine with that
Then I'd effectively get what I'm aiming for here, and cmake users could
just re-run that script, and if one of them did they could push the
result somewhere, and others could just fetch the generated assets.
quoted
Overall I'm don't think that moving from a known set of dependencies
to "build whatever C files are lying around in this directory" is an
improvement.
The caveat you note above with "foo.safe.c" is something we've got
already, see various "$(wildcard)", "find" and "git ls-files" in the
Makefile. So this way we'll at least be consistent. Now we'll add stray
files to TAGS, apply "coccicheck" to them etc.
I'm not so worried about those other targets, but being able to reliably
build and test git with some cruft lying around is useful though. I'm
still not entirely sure what the motivation for this change is (adding
new files is not that common) but I think using the established "git
ls-files || find" pattern would be a good way of globbing without
picking up rubbish if there is a compelling reason to drop the lists.
Best Wishes
Phillip
So one thing I was aiming for here was closing that gap.
I do tihnk having a well understood hierarchy does help a lot, since you
can know that t/helpers/*.c is always one sort of thing etc.
On 06/11/2021 14:27, Ævar Arnfjörð Bjarmason wrote:
quoted
On Sat, Nov 06 2021, Phillip Wood wrote:
quoted
Hi Ævar
On 01/11/2021 19:19, Ævar Arnfjörð Bjarmason wrote:
quoted
Remove the hardcoded lists of objects in favor of using
$(wildcard). This means that every time a built-in, test tool etc. is
added we won't need to patch the top-level Makefile, except for the
few remaining cases where the asset in question would make it onto one
of our list of exceptions.
Ever since 81b50f3ce40 (Move 'builtin-*' into a 'builtin/'
subdirectory, 2010-02-22) this has been relatively easy to do (and
even before that we could glob builtin-*.c). This pattern of
exhaustively enumerating files was then carried forward for
e.g. TEST_BUILTINS_OBJS in efd71f8913a (t/helper: add an empty
test-tool program, 2018-03-24).
One reason not to do this is that now a new *.c file at the
top-level
will be immediately picked up, so if a new *.c file is being worked on
"make" will error if it doesn't compile, whereas before that file
would need to be explicitly listed in the Makefile. I think than small
trade-off is worth it.
If I need to split up some uncommitted changes into several commits
and I know it is going to be fiddly to do so I will sometimes copy the
original file to foo.safe.c and then edit foo.c to create each
commit. Then I can easily compile and test each commit and editing the
file directly is often easier than using add -p and editing the
hunks. With this patch running make will fail in that case I think.
Would being able to do something like:
make EXCLUDE_WILDCARD=foo.safe.c
Satisfy this use-case?
We can also have some DEVOPTS knob so we'll prune out files found if
a
$(shell)-out to "git status" tells us they're untracked. I wouldn't mind
that (and could implement it) if it was optional.
Also note that you've got some of this already, e.g. we'll pick up
*.h
files via a glob for "make TAGS", the dependency graph etc.
I'd be happier using 'git ls-files' with a glob if we need to move
away from listing the files explicitly rather than having to pass some
exclude list when running make. Having seen your comments below about
ls-files/find I had a look at the Makefile and they always seem to be
used together as "git ls-files ... || find ...". Doing that would mean
we wouldn't try to build any untracked files but still find everything
in a tarball.
Yes we could use the $(wildcard) in a tarball, the issue of picking up
untracked files is a dev-checkout-only issue, no?
quoted
quoted
[...]
My recollection is that the discussions were about not having to touch
CMakeList.txt when adding new files to the build and I think that
largely works. I don't think a lot of the changes you have been making
recently were anticipated in that discussion.
That's part of it, but the concern about needing to maintain two
systems
in perpetuity was also brought up, and it not being a hard dependency,
having that vs-{build,test} job soft-fail on it etc. were brought up but
that's not what we've got now.
In any case, having to maintain two build systems and the
maintainer(s)
of the CMake being inactive one is the situation we're in now.
quoted
quoted
- The promised "We can add a (continue-on-error) to vs-build job to
make this process less of a hindrance." in [2] never materialized.
Since 4c2c38e800f (ci: modification of main.yml to use cmake for
vs-build job, 2020-06-26) got a hard dependency on CMake as far as
getting the CI to pass goes.
- The "vs-build" CI doesn't actually require that there be no GNU
make
usage in the job, as it itself has a hard dependency on running a
"make -n artifacts-tar" command. So as far as any vs-specific special-sauce
goes we don't need a GNU-make free build system for vs-build.
We need GNU-make for the ci job but an individual developer using
CMake does not need GNU-make installed. On linux it is possible to
build git without having make installed by using cmake and ninja [1]
I'd tried that, but didn't know about your [1] topic. If you or
someone
is actively willing to fix things up in CMake....
I only started working on that a few days ago. I was interested to see
if using ninja was faster for building when just a few files have been
changed but it doesn't make much difference unless absolutely nothing
has changed - I guess we don't have that many files in the grand
scheme of things. The CMake support still lags the Makefile as
currently things like GIT-VERSION-FILE and git-hooks.h are not
regenerated when their inputs change. I'm not sure if I want to commit
to doing much more on that at the moment.
I suspect that any other build system is faster because it doesn't
actually do what our Makefile does, which is something I've been trying
to slowly fix.
E.g. getting rid of the FORCE dependencies we've got. I submitted a
series just now to do some prep work for that:
https://lore.kernel.org/git/cover-00.16-00000000000-20211106T205717Z-avarab@gmail.com/
The performance of that:
0 $ hyperfine --warmup 3 -L v master,next,avar/Makefile-perl-python-shell-quote-various-fixes,avar/Makefile-get-rid-of-FORCE-rules-for-gmake-4.2-dependency -p 'git checkout {v} && make -j8 all' 'echo {v} && make -j8 all'
Benchmark #1: echo master && make -j8 all
Time (mean ± σ): 127.7 ms ± 40.8 ms [User: 111.5 ms, System: 17.4 ms]
Range (min … max): 113.2 ms … 243.8 ms 10 runs
options.
Benchmark #2: echo next && make -j8 all
Time (mean ± σ): 114.9 ms ± 0.9 ms [User: 105.6 ms, System: 23.1 ms]
Range (min … max): 113.5 ms … 116.3 ms 11 runs
Benchmark #3: echo avar/Makefile-perl-python-shell-quote-various-fixes && make -j8 all
Time (mean ± σ): 121.6 ms ± 16.8 ms [User: 107.9 ms, System: 22.2 ms]
Range (min … max): 113.6 ms … 167.4 ms 10 runs
Benchmark #4: echo avar/Makefile-get-rid-of-FORCE-rules-for-gmake-4.2-dependency && make -j8 all
Time (mean ± σ): 71.4 ms ± 17.2 ms [User: 61.7 ms, System: 11.1 ms]
Range (min … max): 64.2 ms … 131.9 ms 15 runs
Summary
'echo avar/Makefile-get-rid-of-FORCE-rules-for-gmake-4.2-dependency && make -j8 all' ran
1.61 ± 0.39 times faster than 'echo next && make -j8 all'
1.70 ± 0.47 times faster than 'echo avar/Makefile-perl-python-shell-quote-various-fixes && make -j8 all'
1.79 ± 0.72 times faster than 'echo master && make -j8 all'
I.e. so far I haven't really optimized anything much, but the next step
after that series above speeds it up by almost 2x for a noop run, and
there's other low-hanging fruit after that that I've got in some WIP
code somewhere. I think I managed to get it down to the 10-20ms range
for noop runs in one of those.
quoted
quoted
[...]
quoted
My attempt to amend/revert 4c2c38e800f to have it use the
pre-4c2c38e800f "make" invocation as a fallback failed, partially
because I don't have a Windows development environment, so any attempt
to change it is a painfully slow round-trip to GitHub CI.
Let's instead have CMake call out to the Makefile asking it what the
definition of various variables lists is, rather than being forced to
maintain those lists in a way that CMake can parse with regexes (which
precludes anything but a giant hardcoded list).
I could familiarize myself enough with CMake to do this in some
CMake-native way, but that would take "just as long as adding it to
the Makefile"[2] (I think that took me <5 minutes, but I'm several
hours into fighting with CMake)
So I consider this both a bugfix to the stated aims of this CMake
integration, and a better way forward for having an alternate build
system. I.e. If someone really does care about a having a
GNU-make-less dependency for the "vs-build" I think this change offers
a much better way forward for that.
I don't see how relying on GNU-make is a step forward for the CMake
integration when it works without it now.
It's not if we assume we've got infinite man hours to maintain these
systems, but we don't.
I have some pending patches to make various common cases when using
make
much better, mainly speeding up no-op runs so things in rebase --exec go
faster.
So far I've been submitting the parts of that that don't step on the
toes of this cmake integration, and realistically if I've got to
implement everything in lockstep in two systems I'll probably just give
up on it.
Hence asking if there's some middle ground we can find here.
What is it about listing the input files explicitly that slows things
down? Surely it's faster than globbing the filesystem.
It doesn't slow things down. I'd like to get rid of these big hardcoded
lists to make it easier to add new [test] commands, anything that cuts
down on the "modify these N files, copy/pasting the last similar commit"
helps with that.
And we've got churn related to these lists being unsorted, being
re-sorted, and occasional tiresome merge conflicts etc.
The initial reason I finished this up was because I had a WIP patch to
add a "sort assertion" for these lists, per the discussion here:
https://lore.kernel.org/git/87mtv2dk18.fsf@evledraar.gmail.com/
But I realized instead of doing that we could just use $(wildcard)
instead.
quoted
So you don't want to install "make" on Windows, but how about if we had
a script in contrib/ that generated these extractions of lists from the
Makefile instead of doing it on the fly, we could even commit those to
the repo.
If I understand there would be some make rule that generates a list of
dependencies and we'd commit that list to the repo and consume it in
CMakeLists.txt. I'd be fine with that
Ok that certainly helps & is a viable workaround.
quoted
Then I'd effectively get what I'm aiming for here, and cmake users could
just re-run that script, and if one of them did they could push the
result somewhere, and others could just fetch the generated assets.
quoted
Overall I'm don't think that moving from a known set of dependencies
to "build whatever C files are lying around in this directory" is an
improvement.
The caveat you note above with "foo.safe.c" is something we've got
already, see various "$(wildcard)", "find" and "git ls-files" in the
Makefile. So this way we'll at least be consistent. Now we'll add stray
files to TAGS, apply "coccicheck" to them etc.
I'm not so worried about those other targets, but being able to
reliably build and test git with some cruft lying around is useful
though. I'm still not entirely sure what the motivation for this
change is (adding new files is not that common) but I think using the
established "git ls-files || find" pattern would be a good way of
globbing without picking up rubbish if there is a compelling reason to
drop the lists.
I would like to get rid of those ls-files or whatever globs entirely,
whether that's by moving everything to wildcards or having them consume
the hardcoded lists doesn't really matter.
But it's a a wart that we're e.g. running coccicheck on one globbed set
of files, and compile a hardcoded list of files. Partially it's because
we conflate "list of known files" with "here's what we'd like to compile
on this platform".
From: Junio C Hamano <hidden> Date: 2021-11-09 21:38:49
Phillip Wood [off-list ref] writes:
quoted
We can also have some DEVOPTS knob so we'll prune out files found if
a
$(shell)-out to "git status" tells us they're untracked. I wouldn't mind
that (and could implement it) if it was optional.
Also note that you've got some of this already, e.g. we'll pick up
*.h
files via a glob for "make TAGS", the dependency graph etc.
I'd be happier using 'git ls-files' with a glob if we need to move
away from listing the files explicitly rather than having to pass some
exclude list when running make. Having seen your comments below about
ls-files/find I had a look at the Makefile and they always seem to be
used together as "git ls-files ... || find ...". Doing that would mean
we wouldn't try to build any untracked files but still find everything
in a tarball.
I've been quiet on this topic because honestly I do not find the
pros-and-cons favourable for more use of wildcards [*]. Tools like
git (especially .gitignore) and Makefile are to help users to be
safely sloppy by ensuring that random crufts the users may create in
the working tree for their convenience are not picked up by default
unless the project to consciously expresses the desire to use them.
Allowing to be sloppy while maintaining Makefile feels like a false
economy, and having to paper it over by adding exceptions and
forcing developers to learn such ad-hoc rules even more so.
Side note: TAGS generation and some other minor things may use
$(wildcard) and can throw tokens in cruft files in the output,
which is not ideal, but the damage is local. We cannot treat
that the same as building binaries and tarballs.
If we could use "git ls-files" consistently, that may make it
somewhat safer; you'd at least need to "git add" a new file before
it gets into the picture. But it would be impossible, because we
need to be able to bootstrap Git from a tarball extract.
quoted
quoted
quoted
We could make this simpler still for the Makefile by moving
"unix-socket.c" etc. to e.g. a "conditional-src/" directory, likewise
for $(PROGRAM_OBJS) to e.g. "programs/". If we did that we would not
need the "$(filter-out)" for LIB_OBJS. I don't think that's worth it,
e.g. due to "git log -- <path>" on the files now needing a "--follow".
And it is not quite clear to me why we want to even more pile
workaround like this on top. This also is to paper over the mistake
of being sloppy and using $(wildcard), which makes it unable to
distinguish, among the ones that match a pattern, between FOO_OBJS
and BAR_OBJS, no? Moving files around in the working tree to group
related things together is a good thing, and it has been a good move
to separate built-ins and library-ish parts into different
directories. But the above does not sound like it.
Other than "these source files may or may not be compiled
depending", what trait do files in conditional-src/ share, other
than "dividing them into a separate category makes it simpler to
write Makefile using $(wildcard)"? I do not think of a good one.
The only time I found that the large list of files in Makefile was
problematic was *NOT* when multiple topics added, renamed or removed
the files (it is pretty much bog standard merge conflicts that do
not happen very often to begin with). It is when this kind of
"large scale refactoring" for the sake of refactoring happens.
I'm not so worried about those other targets, but being able to
reliably build and test git with some cruft lying around is useful
though. I'm still not entirely sure what the motivation for this
change is (adding new files is not that common) but I think using the
established "git ls-files || find" pattern would be a good way of
globbing without picking up rubbish if there is a compelling reason to
drop the lists.
From: Johannes Schindelin <hidden> Date: 2021-11-10 12:40:04
Hi Junio,
On Tue, 9 Nov 2021, Junio C Hamano wrote:
Allowing to be sloppy while maintaining Makefile feels like a false
economy, and having to paper it over by adding exceptions and
forcing developers to learn such ad-hoc rules even more so.
If you ever needed another opinion to back you up on this: I fully agree.
If we could use "git ls-files" consistently, that may make it
somewhat safer; you'd at least need to "git add" a new file before
it gets into the picture. But it would be impossible, because we
need to be able to bootstrap Git from a tarball extract.
Indeed, the ability to build from a `.tar` extract is important. That's
why we were careful to use `ls-files` in `LIB_H` and in
`FIND_SOURCE_FILE`, falling back on using `find` if the `ls-files` call
failed.
And to be honest, even `LIB_H` and `FIND_SOURCE_FILE` would quite
potentially better be hard-coded (with a CI check to ensure that they're
up to date).
Ciao,
Dscho
Hi Junio,
On Tue, 9 Nov 2021, Junio C Hamano wrote:
quoted
Allowing to be sloppy while maintaining Makefile feels like a false
economy, and having to paper it over by adding exceptions and
forcing developers to learn such ad-hoc rules even more so.
If you ever needed another opinion to back you up on this: I fully agree.
I could go either way on that, but in terms of Makefile maintenance it
does suck a lot less to pick one or the other.
A (I realize, unstated) eventual goal I had was to move these wildcard
declarations to some common list you can include from various Makefiles,
currently we've got dependency bugs in e.g. Makefile &
Documentation/Makefile interaction.
If we're not OK with $(wildcard) as a pattern that would mean changing
all of these to hardcoded (in some cases quite big) lists somewhere:
$ git -P grep -E '^[^~]+\$\(wildcard.+\*' ':!git-gui' ':!gitk-git' ':!contrib'
Documentation/Makefile: $(wildcard git-*.txt))
Documentation/Makefile:HOWTO_TXT += $(wildcard howto/*.txt)
Documentation/Makefile:DOC_DEP_TXT += $(wildcard *.txt)
Documentation/Makefile:DOC_DEP_TXT += $(wildcard config/*.txt)
Documentation/Makefile:API_DOCS = $(patsubst %.txt,%,$(filter-out technical/api-index-skel.txt technical/api-index.txt, $(wildcard technical/api-*.txt)))
Documentation/Makefile:mergetools-list.made: ../git-mergetool--lib.sh $(wildcard ../mergetools/*)
Documentation/Makefile:%.1 %.5 %.7 : %.xml manpage-base-url.xsl $(wildcard manpage*.xsl)
Makefile:command-list.h: $(wildcard Documentation/git*.txt)
Makefile:POFILES := $(wildcard po/*.po)
Makefile:LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm perl/Git/*/*/*.pm)
Makefile:LIB_CPAN := $(wildcard perl/FromCPAN/*.pm perl/FromCPAN/*/*.pm)
Makefile:coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/coccinelle/*.cocci)))
Makefile:coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci))
t/Makefile:T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
t/Makefile:TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))
t/Makefile:THELPERS = $(sort $(filter-out $(T),$(wildcard *.sh)))
t/Makefile:TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
t/Makefile:CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
t/interop/Makefile:T = $(sort $(wildcard i[0-9][0-9][0-9][0-9]-*.sh))
What do you & Junio think about that?
I don't really mind either way, as long as I stop running into
occasional bugs where I need to run "git clean -dxf" because the
Makeefile was too stupid to properly manage its dependencies.
quoted
If we could use "git ls-files" consistently, that may make it
somewhat safer; you'd at least need to "git add" a new file before
it gets into the picture. But it would be impossible, because we
need to be able to bootstrap Git from a tarball extract.
Indeed, the ability to build from a `.tar` extract is important. That's
why we were careful to use `ls-files` in `LIB_H` and in
`FIND_SOURCE_FILE`, falling back on using `find` if the `ls-files` call
failed.
Why would you need any of that to *build* from a .tar extract? I think
we should remove that LIB_H thing entirely.
Its only purpose is to support someone who:
1. Wants to do an *incremental* build, not a "build from tar". I.e. you
build already, changed a header, and now you want to not over-build
again.
Your compiler is perfectly capable of locating headers in an -I dir
for you.
2. Doesn't have gcc or clang installed. Note "installed", not to
build.
Well, currently we require you to build with those to use .depends &
COMPUTE_HEADER_DEPENDENCIES, but that's an easily fixable
implementation detail.
We can easily make the .depend files with gcc/clang and build with
another compiler. I had a 5-10 line local change at some point to do
that.
3. Doesn't find it acceptable to have a fallback of just a glob like
"**.h" for that "depends" target.
I.e. we'd over-rebuild if you dropped in a new *.h we're not
actually using into your extracted tarball, but really, who cares?
4. Wants to run "make hdr-check" or "make pot", both of which I think
are OK to say "you need to run this on a box that has .depends (or
in the case of *.pot, we can use a greedier glob).
And to be honest, even `LIB_H` and `FIND_SOURCE_FILE` would quite
potentially better be hard-coded (with a CI check to ensure that they're
up to date).
That would be a bug, just because I don't build on Windows doesn't mean
that I wouldn't like "make TAGS coccicheck" to find compat/win32/ at
all.
It doesn't do that now for a different reason, but that's a bug that
should be fixed.
From: Johannes Schindelin <hidden> Date: 2021-11-10 15:00:01
Hi Ævar,
On Wed, 10 Nov 2021, Ævar Arnfjörð Bjarmason wrote:
If we're not OK with $(wildcard) as a pattern that would mean changing
all of these to hardcoded (in some cases quite big) lists somewhere:
[...]
No, it would only mean changing these instances if we have a concrete
need. I fail to see a concrete need.
That does not mean that we should make the situation even worse by
converting currently hard-coded lists to wildcards. There is, once again,
no concrete need for that, and there is the good reason Junio brought up
against such a churn: it is too sloppy.
[...] I think we should remove that LIB_H thing entirely.
I think we should take a break from refactoring code where it is unclear
what purpose the refactoring serves.
quoted
And to be honest, even `LIB_H` and `FIND_SOURCE_FILE` would quite
potentially better be hard-coded (with a CI check to ensure that
they're up to date).
That would be a bug, just because I don't build on Windows doesn't mean
that I wouldn't like "make TAGS coccicheck" to find compat/win32/ at
all.
Talking about `coccicheck` in the context of the discussion whether we
should make sweeping changes in our Makefiles, all while we're supposedly
in the -rc phase, strikes me as a distant tangent of a distant tangent.
Ciao,
Johannes
Hi Ævar,
On Wed, 10 Nov 2021, Ævar Arnfjörð Bjarmason wrote:
quoted
If we're not OK with $(wildcard) as a pattern that would mean changing
all of these to hardcoded (in some cases quite big) lists somewhere:
[...]
No, it would only mean changing these instances if we have a concrete
need. I fail to see a concrete need.
That does not mean that we should make the situation even worse by
converting currently hard-coded lists to wildcards. There is, once again,
no concrete need for that, and there is the good reason Junio brought up
against such a churn: it is too sloppy.
I don't it's sloppy. It's just a different approach. It's also an
approach we use now. Add a new build in and your addition in t/*.sh and
Documentation/*.txt will be picked up & built. We just won't pick up the
*.h or builtin/*.c implicitly.
So whether we need to do this now is one thing, but saying it's a big
change in workflow seems to be rather exaggerated.
quoted
[...] I think we should remove that LIB_H thing entirely.
I think we should take a break from refactoring code where it is unclear
what purpose the refactoring serves.
I'm not advocating ripping LIB_H out right now, and have not submitted
any patches to do so.
I'm asking you a follow-up question about your claim that LIB_H is
needed for building from a tarball. I don't think it is, but perhaps I'm
missing something.
It would be useful to get an answer to that for the list records, so
that while it's fresh in your mind we can get an answer one way or the
other.
I agree there's no a strong reason to change it now, but being able to
do so in the future might be useful.
At that point someone will probably dig up this thread. Whether "do we
need LIB_H for what Johannes suggested?" is a dead end or not I'll leave
to you.
quoted
quoted
And to be honest, even `LIB_H` and `FIND_SOURCE_FILE` would quite
potentially better be hard-coded (with a CI check to ensure that
they're up to date).
That would be a bug, just because I don't build on Windows doesn't mean
that I wouldn't like "make TAGS coccicheck" to find compat/win32/ at
all.
Talking about `coccicheck` in the context of the discussion whether we
should make sweeping changes in our Makefiles, all while we're supposedly
in the -rc phase, strikes me as a distant tangent of a distant tangent.
For the past few releases I think I've probably submitted more
last-minute rc fixes than most. In my case it's mostly a matter of
starting some builds and waiting for them to complete:
https://xkcd.com/303/
So yeah, I think we should focus around release time, but saying that
any other ongoing discussion is a needless distraction seems like a
bridge too far.
That being said the reason I'm submitting these sorts of fixes now is
directly related to making rc testing easier.
I test on some obnoxiously slow VMs or otherwise limited computers on
the GCC farm. When something breaks between releases having each step of
a bisect take 30m or 60m makes a big difference.
So having a Makefile that doesn't over-build stuff is important to
release testing.
But getting that across has been frustrating at times. I've pretty much
stopped testing on AIX because my few-lines of patches to the Makefile
to make that drastically easier were categorically rejected. The
response to some other things like over-building <xyz> has been
somewhere between "I don't see why you care, my computer is fast enough"
and "why don't you port ccache to <90s era *nix OS that barely compiles
Hello World without issues".
We can also have some DEVOPTS knob so we'll prune out files found if
a
$(shell)-out to "git status" tells us they're untracked. I wouldn't mind
that (and could implement it) if it was optional.
Also note that you've got some of this already, e.g. we'll pick up
*.h
files via a glob for "make TAGS", the dependency graph etc.
I'd be happier using 'git ls-files' with a glob if we need to move
away from listing the files explicitly rather than having to pass some
exclude list when running make. Having seen your comments below about
ls-files/find I had a look at the Makefile and they always seem to be
used together as "git ls-files ... || find ...". Doing that would mean
we wouldn't try to build any untracked files but still find everything
in a tarball.
I've been quiet on this topic because honestly I do not find the
pros-and-cons favourable for more use of wildcards [*]. Tools like
git (especially .gitignore) and Makefile are to help users to be
safely sloppy by ensuring that random crufts the users may create in
the working tree for their convenience are not picked up by default
unless the project to consciously expresses the desire to use them.
Allowing to be sloppy while maintaining Makefile feels like a false
economy, and having to paper it over by adding exceptions and
forcing developers to learn such ad-hoc rules even more so.
Side note: TAGS generation and some other minor things may use
$(wildcard) and can throw tokens in cruft files in the output,
which is not ideal, but the damage is local. We cannot treat
that the same as building binaries and tarballs.
If we could use "git ls-files" consistently, that may make it
somewhat safer; you'd at least need to "git add" a new file before
it gets into the picture. But it would be impossible, because we
need to be able to bootstrap Git from a tarball extract.
quoted
quoted
quoted
quoted
We could make this simpler still for the Makefile by moving
"unix-socket.c" etc. to e.g. a "conditional-src/" directory, likewise
for $(PROGRAM_OBJS) to e.g. "programs/". If we did that we would not
need the "$(filter-out)" for LIB_OBJS. I don't think that's worth it,
e.g. due to "git log -- <path>" on the files now needing a "--follow".
And it is not quite clear to me why we want to even more pile
workaround like this on top. This also is to paper over the mistake
of being sloppy and using $(wildcard), which makes it unable to
distinguish, among the ones that match a pattern, between FOO_OBJS
and BAR_OBJS, no? Moving files around in the working tree to group
related things together is a good thing, and it has been a good move
to separate built-ins and library-ish parts into different
directories. But the above does not sound like it.
Other than "these source files may or may not be compiled
depending", what trait do files in conditional-src/ share, other
than "dividing them into a separate category makes it simpler to
write Makefile using $(wildcard)"? I do not think of a good one.
The only time I found that the large list of files in Makefile was
problematic was *NOT* when multiple topics added, renamed or removed
the files (it is pretty much bog standard merge conflicts that do
not happen very often to begin with). It is when this kind of
"large scale refactoring" for the sake of refactoring happens.
quoted
I'm not so worried about those other targets, but being able to
reliably build and test git with some cruft lying around is useful
though. I'm still not entirely sure what the motivation for this
change is (adding new files is not that common) but I think using the
established "git ls-files || find" pattern would be a good way of
globbing without picking up rubbish if there is a compelling reason to
drop the lists.
Yes.
Reviewing the reftable coverity topic I was reminded of this
patch. I.e. in it we have this fix:
https://lore.kernel.org/git/xmqqtugl102l.fsf@gitster.g/
Which shows another advantage of using this sort of $(wildcard) pattern,
i.e. if we had this:
diff --git a/Makefile b/Makefile
index 5580859afdb..48ea18afa53 100644
--- a/Makefile
+++ b/Makefile
@@ -2443,33 +2443,9 @@ XDIFF_OBJS += xdiff/xutils.o
.PHONY: xdiff-objs
xdiff-objs: $(XDIFF_OBJS)
-REFTABLE_OBJS += reftable/basics.o
-REFTABLE_OBJS += reftable/error.o
-REFTABLE_OBJS += reftable/block.o
-REFTABLE_OBJS += reftable/blocksource.o
-REFTABLE_OBJS += reftable/iter.o
-REFTABLE_OBJS += reftable/publicbasics.o
-REFTABLE_OBJS += reftable/merged.o
-REFTABLE_OBJS += reftable/pq.o
-REFTABLE_OBJS += reftable/reader.o
-REFTABLE_OBJS += reftable/record.o
-REFTABLE_OBJS += reftable/refname.o
-REFTABLE_OBJS += reftable/generic.o
-REFTABLE_OBJS += reftable/stack.o
-REFTABLE_OBJS += reftable/tree.o
-REFTABLE_OBJS += reftable/writer.o
-
-REFTABLE_TEST_OBJS += reftable/basics_test.o
-REFTABLE_TEST_OBJS += reftable/block_test.o
-REFTABLE_TEST_OBJS += reftable/dump.o
-REFTABLE_TEST_OBJS += reftable/merged_test.o
-REFTABLE_TEST_OBJS += reftable/pq_test.o
-REFTABLE_TEST_OBJS += reftable/record_test.o
-REFTABLE_TEST_OBJS += reftable/readwrite_test.o
-REFTABLE_TEST_OBJS += reftable/refname_test.o
-REFTABLE_TEST_OBJS += reftable/stack_test.o
-REFTABLE_TEST_OBJS += reftable/test_framework.o
-REFTABLE_TEST_OBJS += reftable/tree_test.o
+REFTABLE_SOURCES = $(wildcard reftable/*.c)
+REFTABLE_OBJS += $(filter-out test,$(REFTABLE_SOURCES:%.c=%.o))
+REFTABLE_TEST_OBJS += $(filter test,$(REFTABLE_SOURCES:%.c=%.o))
TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
We'd have a shorter Makefile, not need to manually maintain the list,
and we'd have been getting linker errors all along on the dead code
(just showing one of many here):
$ make
[...]
/usr/bin/ld: reftable/libreftable.a(generic.o): in function `reftable_table_seek_ref':
/home/avar/g/git/reftable/generic.c:17: multiple definition of `reftable_table_seek_ref'; reftable/libreftable.a(reftable.o):/home/avar/g/git/reftable/reftable.c:17: first defined here
[...]
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [Makefile:2925: t/helper/test-tool] Error 1
make: Target 'all' not remade because of errors.
Hi Ævar
On 21/01/2022 12:01, Ævar Arnfjörð Bjarmason wrote:
On Tue, Nov 09 2021, Junio C Hamano wrote:
quoted
Phillip Wood [off-list ref] writes:
quoted
quoted
We can also have some DEVOPTS knob so we'll prune out files found if
a
$(shell)-out to "git status" tells us they're untracked. I wouldn't mind
that (and could implement it) if it was optional.
Also note that you've got some of this already, e.g. we'll pick up
*.h
files via a glob for "make TAGS", the dependency graph etc.
I'd be happier using 'git ls-files' with a glob if we need to move
away from listing the files explicitly rather than having to pass some
exclude list when running make. Having seen your comments below about
ls-files/find I had a look at the Makefile and they always seem to be
used together as "git ls-files ... || find ...". Doing that would mean
we wouldn't try to build any untracked files but still find everything
in a tarball.
I've been quiet on this topic because honestly I do not find the
pros-and-cons favourable for more use of wildcards [*]. Tools like
git (especially .gitignore) and Makefile are to help users to be
safely sloppy by ensuring that random crufts the users may create in
the working tree for their convenience are not picked up by default
unless the project to consciously expresses the desire to use them.
Allowing to be sloppy while maintaining Makefile feels like a false
economy, and having to paper it over by adding exceptions and
forcing developers to learn such ad-hoc rules even more so.
Side note: TAGS generation and some other minor things may use
$(wildcard) and can throw tokens in cruft files in the output,
which is not ideal, but the damage is local. We cannot treat
that the same as building binaries and tarballs.
If we could use "git ls-files" consistently, that may make it
somewhat safer; you'd at least need to "git add" a new file before
it gets into the picture. But it would be impossible, because we
need to be able to bootstrap Git from a tarball extract.
quoted
quoted
quoted
quoted
We could make this simpler still for the Makefile by moving
"unix-socket.c" etc. to e.g. a "conditional-src/" directory, likewise
for $(PROGRAM_OBJS) to e.g. "programs/". If we did that we would not
need the "$(filter-out)" for LIB_OBJS. I don't think that's worth it,
e.g. due to "git log -- <path>" on the files now needing a "--follow".
And it is not quite clear to me why we want to even more pile
workaround like this on top. This also is to paper over the mistake
of being sloppy and using $(wildcard), which makes it unable to
distinguish, among the ones that match a pattern, between FOO_OBJS
and BAR_OBJS, no? Moving files around in the working tree to group
related things together is a good thing, and it has been a good move
to separate built-ins and library-ish parts into different
directories. But the above does not sound like it.
Other than "these source files may or may not be compiled
depending", what trait do files in conditional-src/ share, other
than "dividing them into a separate category makes it simpler to
write Makefile using $(wildcard)"? I do not think of a good one.
The only time I found that the large list of files in Makefile was
problematic was *NOT* when multiple topics added, renamed or removed
the files (it is pretty much bog standard merge conflicts that do
not happen very often to begin with). It is when this kind of
"large scale refactoring" for the sake of refactoring happens.
quoted
I'm not so worried about those other targets, but being able to
reliably build and test git with some cruft lying around is useful
though. I'm still not entirely sure what the motivation for this
change is (adding new files is not that common) but I think using the
established "git ls-files || find" pattern would be a good way of
globbing without picking up rubbish if there is a compelling reason to
drop the lists.
Yes.
Reviewing the reftable coverity topic I was reminded of this
patch. I.e. in it we have this fix:
https://lore.kernel.org/git/xmqqtugl102l.fsf@gitster.g/
Which shows another advantage of using this sort of $(wildcard) pattern,
i.e. if we had this:
diff --git a/Makefile b/Makefile
index 5580859afdb..48ea18afa53 100644
--- a/Makefile
+++ b/Makefile
@@ -2443,33 +2443,9 @@ XDIFF_OBJS += xdiff/xutils.o
.PHONY: xdiff-objs
xdiff-objs: $(XDIFF_OBJS)
-REFTABLE_OBJS += reftable/basics.o
-REFTABLE_OBJS += reftable/error.o
-REFTABLE_OBJS += reftable/block.o
-REFTABLE_OBJS += reftable/blocksource.o
-REFTABLE_OBJS += reftable/iter.o
-REFTABLE_OBJS += reftable/publicbasics.o
-REFTABLE_OBJS += reftable/merged.o
-REFTABLE_OBJS += reftable/pq.o
-REFTABLE_OBJS += reftable/reader.o
-REFTABLE_OBJS += reftable/record.o
-REFTABLE_OBJS += reftable/refname.o
-REFTABLE_OBJS += reftable/generic.o
-REFTABLE_OBJS += reftable/stack.o
-REFTABLE_OBJS += reftable/tree.o
-REFTABLE_OBJS += reftable/writer.o
-
-REFTABLE_TEST_OBJS += reftable/basics_test.o
-REFTABLE_TEST_OBJS += reftable/block_test.o
-REFTABLE_TEST_OBJS += reftable/dump.o
-REFTABLE_TEST_OBJS += reftable/merged_test.o
-REFTABLE_TEST_OBJS += reftable/pq_test.o
-REFTABLE_TEST_OBJS += reftable/record_test.o
-REFTABLE_TEST_OBJS += reftable/readwrite_test.o
-REFTABLE_TEST_OBJS += reftable/refname_test.o
-REFTABLE_TEST_OBJS += reftable/stack_test.o
-REFTABLE_TEST_OBJS += reftable/test_framework.o
-REFTABLE_TEST_OBJS += reftable/tree_test.o
+REFTABLE_SOURCES = $(wildcard reftable/*.c)
+REFTABLE_OBJS += $(filter-out test,$(REFTABLE_SOURCES:%.c=%.o))
+REFTABLE_TEST_OBJS += $(filter test,$(REFTABLE_SOURCES:%.c=%.o))
TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
We'd have a shorter Makefile, not need to manually maintain the list,
and we'd have been getting linker errors all along on the dead code
(just showing one of many here):
$ make
[...]
/usr/bin/ld: reftable/libreftable.a(generic.o): in function `reftable_table_seek_ref':
/home/avar/g/git/reftable/generic.c:17: multiple definition of `reftable_table_seek_ref'; reftable/libreftable.a(reftable.o):/home/avar/g/git/reftable/reftable.c:17: first defined here
[...]
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [Makefile:2925: t/helper/test-tool] Error 1
make: Target 'all' not remade because of errors.
Random cruft breaking the build was the reason I objected to this
change, just because the cruft was being tracked by git in this case
does not change that.
Best Wishes
Phillip
Hi Ævar
On 21/01/2022 12:01, Ævar Arnfjörð Bjarmason wrote:
quoted
On Tue, Nov 09 2021, Junio C Hamano wrote:
quoted
Phillip Wood [off-list ref] writes:
quoted
quoted
We can also have some DEVOPTS knob so we'll prune out files found if
a
$(shell)-out to "git status" tells us they're untracked. I wouldn't mind
that (and could implement it) if it was optional.
Also note that you've got some of this already, e.g. we'll pick up
*.h
files via a glob for "make TAGS", the dependency graph etc.
I'd be happier using 'git ls-files' with a glob if we need to move
away from listing the files explicitly rather than having to pass some
exclude list when running make. Having seen your comments below about
ls-files/find I had a look at the Makefile and they always seem to be
used together as "git ls-files ... || find ...". Doing that would mean
we wouldn't try to build any untracked files but still find everything
in a tarball.
I've been quiet on this topic because honestly I do not find the
pros-and-cons favourable for more use of wildcards [*]. Tools like
git (especially .gitignore) and Makefile are to help users to be
safely sloppy by ensuring that random crufts the users may create in
the working tree for their convenience are not picked up by default
unless the project to consciously expresses the desire to use them.
Allowing to be sloppy while maintaining Makefile feels like a false
economy, and having to paper it over by adding exceptions and
forcing developers to learn such ad-hoc rules even more so.
Side note: TAGS generation and some other minor things may use
$(wildcard) and can throw tokens in cruft files in the output,
which is not ideal, but the damage is local. We cannot treat
that the same as building binaries and tarballs.
If we could use "git ls-files" consistently, that may make it
somewhat safer; you'd at least need to "git add" a new file before
it gets into the picture. But it would be impossible, because we
need to be able to bootstrap Git from a tarball extract.
quoted
quoted
quoted
quoted
We could make this simpler still for the Makefile by moving
"unix-socket.c" etc. to e.g. a "conditional-src/" directory, likewise
for $(PROGRAM_OBJS) to e.g. "programs/". If we did that we would not
need the "$(filter-out)" for LIB_OBJS. I don't think that's worth it,
e.g. due to "git log -- <path>" on the files now needing a "--follow".
And it is not quite clear to me why we want to even more pile
workaround like this on top. This also is to paper over the mistake
of being sloppy and using $(wildcard), which makes it unable to
distinguish, among the ones that match a pattern, between FOO_OBJS
and BAR_OBJS, no? Moving files around in the working tree to group
related things together is a good thing, and it has been a good move
to separate built-ins and library-ish parts into different
directories. But the above does not sound like it.
Other than "these source files may or may not be compiled
depending", what trait do files in conditional-src/ share, other
than "dividing them into a separate category makes it simpler to
write Makefile using $(wildcard)"? I do not think of a good one.
The only time I found that the large list of files in Makefile was
problematic was *NOT* when multiple topics added, renamed or removed
the files (it is pretty much bog standard merge conflicts that do
not happen very often to begin with). It is when this kind of
"large scale refactoring" for the sake of refactoring happens.
quoted
I'm not so worried about those other targets, but being able to
reliably build and test git with some cruft lying around is useful
though. I'm still not entirely sure what the motivation for this
change is (adding new files is not that common) but I think using the
established "git ls-files || find" pattern would be a good way of
globbing without picking up rubbish if there is a compelling reason to
drop the lists.
Yes.
Reviewing the reftable coverity topic I was reminded of this
patch. I.e. in it we have this fix:
https://lore.kernel.org/git/xmqqtugl102l.fsf@gitster.g/
Which shows another advantage of using this sort of $(wildcard)
pattern,
i.e. if we had this:
diff --git a/Makefile b/Makefile
index 5580859afdb..48ea18afa53 100644
--- a/Makefile
+++ b/Makefile
@@ -2443,33 +2443,9 @@ XDIFF_OBJS += xdiff/xutils.o
.PHONY: xdiff-objs
xdiff-objs: $(XDIFF_OBJS)
-REFTABLE_OBJS += reftable/basics.o
-REFTABLE_OBJS += reftable/error.o
-REFTABLE_OBJS += reftable/block.o
-REFTABLE_OBJS += reftable/blocksource.o
-REFTABLE_OBJS += reftable/iter.o
-REFTABLE_OBJS += reftable/publicbasics.o
-REFTABLE_OBJS += reftable/merged.o
-REFTABLE_OBJS += reftable/pq.o
-REFTABLE_OBJS += reftable/reader.o
-REFTABLE_OBJS += reftable/record.o
-REFTABLE_OBJS += reftable/refname.o
-REFTABLE_OBJS += reftable/generic.o
-REFTABLE_OBJS += reftable/stack.o
-REFTABLE_OBJS += reftable/tree.o
-REFTABLE_OBJS += reftable/writer.o
-
-REFTABLE_TEST_OBJS += reftable/basics_test.o
-REFTABLE_TEST_OBJS += reftable/block_test.o
-REFTABLE_TEST_OBJS += reftable/dump.o
-REFTABLE_TEST_OBJS += reftable/merged_test.o
-REFTABLE_TEST_OBJS += reftable/pq_test.o
-REFTABLE_TEST_OBJS += reftable/record_test.o
-REFTABLE_TEST_OBJS += reftable/readwrite_test.o
-REFTABLE_TEST_OBJS += reftable/refname_test.o
-REFTABLE_TEST_OBJS += reftable/stack_test.o
-REFTABLE_TEST_OBJS += reftable/test_framework.o
-REFTABLE_TEST_OBJS += reftable/tree_test.o
+REFTABLE_SOURCES = $(wildcard reftable/*.c)
+REFTABLE_OBJS += $(filter-out test,$(REFTABLE_SOURCES:%.c=%.o))
+REFTABLE_TEST_OBJS += $(filter test,$(REFTABLE_SOURCES:%.c=%.o))
TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
We'd have a shorter Makefile, not need to manually maintain the
list,
and we'd have been getting linker errors all along on the dead code
(just showing one of many here):
$ make
[...]
/usr/bin/ld: reftable/libreftable.a(generic.o): in function `reftable_table_seek_ref':
/home/avar/g/git/reftable/generic.c:17: multiple definition of `reftable_table_seek_ref'; reftable/libreftable.a(reftable.o):/home/avar/g/git/reftable/reftable.c:17: first defined here
[...]
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make: *** [Makefile:2925: t/helper/test-tool] Error 1
make: Target 'all' not remade because of errors.
Random cruft breaking the build was the reason I objected to this
change, just because the cruft was being tracked by git in this case
does not change that.
The difference is that the above is a "good" breakage, i.e. one where we
ended up carrying dead code in-tree, so it wouldn't have escaped the
lab.
Whereas I think that you're concerned that adding a WIP foobar.c to the
tree would break your compilation the next time you invoke 'make'.
Would something like this way of having our cake and eating it too
address your concerns?
I.e. if we have 'git' installed already we could just ask it about any
untracked files, and filter those out:
ifndef NO_WILDCARD_SECOND_GUESSING
UNTRACKED_FILES := $(shell git status --porcelain=1 2>/dev/null | sed -n -e '/^\?? / { s/^.. //; p}')
else
UNTRACKED_FILES =
endif
REFTABLE_SOURCES = $(filter-out $(UNTRACKED_FILES),$(wildcard reftable/*.c))
REFTABLE_OBJS += $(filter-out test,$(REFTABLE_SOURCES:%.c=%.o))
REFTABLE_TEST_OBJS += $(filter test,$(REFTABLE_SOURCES:%.c=%.o))
A non-demo implementation of this would piggy-back on the "git ls-files"
shell-out we already do, but since that requires some refactoring I
thought it would be better to demo it like this.
From: Junio C Hamano <hidden> Date: 2022-01-22 06:45:56
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
quoted
change is (adding new files is not that common) but I think using the
established "git ls-files || find" pattern would be a good way of
globbing without picking up rubbish if there is a compelling reason to
drop the lists.
Yes.
To avoid any misunderstandings, the above "Yes" was given to the
statement, including the "if there is a compelling reason" part
(and there isn't a compelling reason).
Which shows another advantage of using this sort of $(wildcard) pattern,
i.e. if we had this:
diff --git a/Makefile b/Makefile
index 5580859afdb..48ea18afa53 100644
--- a/Makefile
+++ b/Makefile
@@ -2443,33 +2443,9 @@ XDIFF_OBJS += xdiff/xutils.o
.PHONY: xdiff-objs
xdiff-objs: $(XDIFF_OBJS)
+REFTABLE_SOURCES = $(wildcard reftable/*.c)
+REFTABLE_OBJS += $(filter-out test,$(REFTABLE_SOURCES:%.c=%.o))
+REFTABLE_TEST_OBJS += $(filter test,$(REFTABLE_SOURCES:%.c=%.o))
TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
We'd have a shorter Makefile, not need to manually maintain the list,
Both are not all that important.
and we'd have been getting linker errors all along on the dead code
(just showing one of many here):
I am not sure if I follow. You are forgetting to tell us something.
Are you talking about an error you would see when you do what?
Perhaps after you remove reftable/generic.c and have the definition
of reftable_table_seek_ref() that used to be there in
reftable/reftable.c?
Assuming that is the scenario you have in mind, ...
$ make
[...]
/usr/bin/ld: reftable/libreftable.a(generic.o): in function `reftable_table_seek_ref':
/home/avar/g/git/reftable/generic.c:17: multiple definition of `reftable_table_seek_ref'; reftable/libreftable.a(reftable.o):/home/avar/g/git/reftable/reftable.c:17: first defined here
... I do not think concrete list of filenames vs list of filenames
created by $(wildcard) has any effect on that the fact that lib.a
that is incrementally updated by the "ar r lib.a" command does not
lose a stale object file from it.
If we have a concrete filename list and removed generic.c, if we
forget to remove it from the list, it will be noticed way before
"ld" has the chance to complain. We fail to produce generic.o,
which may be a plus. If we did not forget to also remove it from
the list when we removed the file, then $(wildcard) will give us the
same list of filenames, so you'd see the same error from your ld,
no?