Hi Victoria,
On Wed, 31 Aug 2022, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@xxxxxxxxxx>
>
> Move 'scalar' out of 'contrib/' and into the root of the Git tree. The goal
> of this change is to build 'scalar' as part of the standard Git build &
> install processes.
>
> This patch includes both the physical move of Scalar's files out of
> 'contrib/' ('scalar.c', 'scalar.txt', and 't9xxx-scalar.sh'), and the
> changes to the build definitions in 'Makefile' and 'CMakelists.txt' to
> accommodate the new program.
>
> At a high level, Scalar is built so that:
> - there is a 'scalar-objs' target (similar to those created in 029bac01a8
> (Makefile: add {program,xdiff,test,git,fuzz}-objs & objects targets,
> 2021-02-23)) for debugging purposes.
> - it appears in the root of the install directory (rather than the
> gitexecdir).
> - it is included in the 'bin-wrappers/' directory for use in tests.
> - it receives a platform-specific executable suffix (e.g., '.exe'), if
> applicable.
> - 'scalar.txt' is installed as 'man1' documentation.
> - the 'clean' target removes the 'scalar' executable.
>
> Additionally, update the root level '.gitignore' file to ignore the Scalar
> executable.
A great commit message, and even though the diff is large, it is eminently
reviewable, with one exception:
> @@ -3062,7 +3067,7 @@ bin-wrappers/%: wrap-for-bin.sh
> $(call mkdir_p_parent_template)
> $(QUIET_GEN)sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
> -e 's|@@BUILD_DIR@@|$(shell pwd)|' \
> - -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%$(X),$(@F))$(patsubst git%,$(X),$(filter $(@F),$(BINDIR_PROGRAMS_NEED_X)))|' < $< > $@ && \
> + -e 's|@@PROG@@|$(patsubst test-%,t/helper/test-%,$(@F))$(if $(filter-out $(BINDIR_PROGRAMS_NO_X),$(@F)),$(X),)|' < $< > $@ && \
It took me a good while to wrap my head around this (and let me be clear:
I consider none of this your fault, it's the fault of the design of the
Makefile syntax).
Let me untangle this, for posterity's benefit. We substitute the
placeholder `@@PROG@@` with a concatenation of two strings that are both
derived from `@F`, i.e. the basename of the to-be-wrapped command.
Before, the first string would be the dashed command, verbatim, unless its
name starts with a `test-` prefix, in which case we would add another
prefix (`t/helper/`) _and_ append the `.exe` suffix on Windows.
And the second string would be a `.exe` for all remaining cases that need
it: the commands starting with `git` and listed as needing the suffix.
Otherwise, the second string would be empty.
Convoluted, sure, but it worked.
In the new version, the first string would be the basename of the
to-be-wrapped command, with `t/helper/` prefixed for the `test-` helpers.
The second string would be a `.exe` for _all_ commands, except for those
specifically listed as not wanting that suffix as per
`BINDIR_PROGRAMS_NO_X`.
The new logic is so much simpler to understand!
Feel free to add a `Reviewed-by:` footer for me if you send another
iteration of this patch series.
Thank you,
Dscho
