You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
contrib: build & test scalar by default, optionally install
Change how the newly proposed "scalar" command is handle, this builds
on top of the proposed patches that add it along with documentation
and tests to the "contrib/scalar/" directory[1].
With those patches we unconditionally build scalar.o and link it to
libgit.a, but don't build the "scalar" binary itself, and don't run
its tests as part of our normal test suite run or CI .
As the changes to "t/t9099-scalar.sh" here shows that sort of
arrangement leads to breakages. I.e. the scalar tests would fail on
OSX, which our OSX CI jobs will catch, but only if we run the
tests. Let's add a prerequisite to test that, which requires moving
around some of the setup code.
I think that for a libgit.a-using "scalar" in particular, and for
"contrib" in general we're better off by not only compiling the object
in question, but also linking it, and running its tests by
default. What we won't do is install it by default, unless an
"INSTALL_SCALAR=Y" is provided along with "make install".
For context: There's been ongoing discussion about how this command
should be integrated in terms of the "closeness" of its integration,
and how "contrib" sources within git.git should be organized.
That discussion started with my [2], was followed by a WIP patch[3] to
implement the approach being finished up here, and has spawned the
"[Discussion] The architecture of Scalar (and others) within Git"
thread[4]. See also the large earlier discussion hanging off [3].
I really don't think it's important whether a given source file lives
in-tree at "contrib/scalar/scalar.c" or the top-level "scalar.c", but
as the diff below shows the latter approach is a smaller overall
change in our build system, due to how it's organized, more details on
that below under "Build changes".
I think that part of the reason for sticking the new command in
"contrib/scalar/" was to implicitly make it clear from the paths that
it was different. I do think that would be a worthwhile goal in the
abstract.
But given the build simplifications we can attain by moving it to the
top level that we should seek to resolve that ambiguity in the minds
of any potential git.git code maintainers in some other way.
To do that we now have a new 'OPTIONAL CONTRIB COMMANDS' section in
"git help git" explaining our backwards and forwards compatibility
non-promises when it comes to this and similar future commands. Our
users are the ones most likely to be confused about why their git
package has installed a "/usr/bin/scalar" that seemingly duplicates
parts of git's own functionality. We're much better off by
cross-linking our documentation, and mentioning the state of scalar in
git's own documentation, along with the full list of porcelain and
plumbing utilities.
= Build changes =
This fixes dependency bugs in the previous "contrib/scalar/Makefile", as
well as implementing various missing bits of functionality, most
notably "make install" will now install the "scalar" command, but only
if INSTALL_SCALAR is defined.
Those and other changes and non-changes, categorized as "Same", "New"
and "Fixed" below:
== Same ==
A.: We'll unconditionally build scalar.o, as before it's in the
$(OBJECTS) list.
B.: "make contrib/scalar/scalar.o" has the same dependency graph as
before, but is now spelled "make scalar.o", more on the rename below.
== New ==
C.: We'll now unconditionally build and test the scalar command.
Before we'd only build scalar.o, but not link it.
Its test lives in "t/t9099-scalar.sh" now (and take <1s to
run). We should have test coverage of this in-tree command that's
linking to libgit.a.
Previously it had to be manually tested by cd-ing to
contrib/scalar and running "make test", and it would not benefit
from the combination of our various CI targets.
D.: We'll unconditionally build scalar's documentation, and it will be
linted along with the rest, including checking for broken links to
other documentation.
The minor change to scalar.txt here is needed due to the lint
check added in cafd982 (doc lint: lint and fix missing "GIT"
end sections, 2021-04-09), perhaps we should have a different end
blurb for this command, but for now let's make it consistent with
the rest.
When installed (see below) this command is part of the git suite,
so the end blurb should say something to that effect.
E.: "make install" will now install the "scalar" binary, but only if
"INSTALL_SCALAR" is defined.
Relative to our $(prefix) we'll now have (this is with
INSTALL_SYMLINKS=Y):
libexec/git-core/scalar: symbolic link to ../../bin/scalar
bin/scalar: ELF 64-bit LSB pie executable,
Putting it into libexec isn't strictly necessary, but as we do it
with "git" we do that by default, and this will ensure that anyone
who relies on their path being "$(git --exec-path)" will also be
able to find "scalar".
Perhaps we shouldn't put it in "libexec" at all, but for now let's
just follow the herd.
F.: Likewise "make install-man install-doc install-html" works, and
will install "scalar" documentation if "INSTALL_SCALAR" is
defined.
We'll install these files with those targets (and correctly split
them by target, e.g. only scalar.1 with "install-man"):
share/doc/git-doc/scalar.txt: ASCII text
share/doc/git-doc/scalar.html: XML 1.0 document, ASCII text, with CRLF line terminators
share/man/man1/scalar.1: troff or preprocessor input, ASCII text, with very long lines
== Fixed ==
G.: The dependency graph of contrib/scalar/Makefile was broken when it
came to scalar.o, it only depended on ../../libgit.a. So it was a
requirement to run the top-level Makefile first to get around some
of its dependency issues:
make && make -C contrib/scalar
H.: By having the top-level Makefile build scalar.o it'll work as well
as any other git code, including benefiting from
COMPUTE_HEADER_DEPENDENCIES.
Targets such as (and possibly others):
- "make tags TAGS cscope"
- "make coccicheck"
- "make check-docs"
Wouldn't consider contrib/scalar at all, some of those would have
been fixable, but as shown in the rest of this patch it's easier
just to have scalar built like any other program instead.
== Discussion and motivation ==
I've been fixing various dependency issues in git's Makefile
infrastructure recently, some of which has been integrated into
"master", some of which is currently under review on-list, and some of
which isn't submitted yet.
To the extent that we have build dependency issues and cases where we
can't build something in parallel it's usually because we're invoking
one Makefile from another. That's the case in most of our
sub-Makefiles, which will usually need to invoke or depend on
something created in the top-level Makefile.
None of {t,Documentation,template}/Makefile etc. are truly
independent, as looking at $(git grep -F '../' '*Makefile') will
reveal, and we'll sometimes need to duplicate logic between them
purely to get around them not being driven by one top-level Makefile
logic.
Much has been written on this topic, likely most notably the
"Recursive Make Considered Harmful" paper (full PDF link at [5]), and
to be fair, the there's also a well-known "Non-recursive Make
Considered Harmful" retort at [6]).
Theoretical opinions on the topic clearly differ. From a purely
practical perspective I ran into textual and semantic conflicts with
the "contrib/scalar/" work.
I think the diffstat of this patch demonstrates that this is a much
simpler approach, particularly considering that the code being
replaced didn't perform much of the needed integrations that are "new"
here. E.g. we now have "make install". The unsubmitted [7] (linked to
by [8]) shows the boilerplate we'd need for that with the alternate
approach.
It would be more pleasing to not have to hardcode "git" in the
pre-image and now "git" and "scalar" in the post-image in several
places. My initial WIP [3] looks better, but built on a set of
Makefile reorganization patches.
I'd like to not make those patches a dependency of this change, but
readers should rest assured that we're really not ending up with as
many "scalar" special-cases in the long term as this diff indicates,
most or all of those can all be generalized into Makefile refactoring
that teaches our build system to build N number of differently named
top-level commands.
1. https://lore.kernel.org/git/pull.1005.v6.git.1635323239.gitgitgadget@gmail.com/
2. https://lore.kernel.org/git/87czpuxbwp.fsf@evledraar.gmail.com/
3. https://lore.kernel.org/git/87ilz44kdk.fsf@evledraar.gmail.com/
4. https://lore.kernel.org/git/b67bbef4-e4c3-b6a7-1c7f-7d405902ef8b@gmail.com/
5. https://web.archive.org/web/20150330111905/http://miller.emu.id.au/pmiller/books/rmch/
6. https://www.microsoft.com/en-us/research/wp-content/uploads/2016/03/hadrian.pdf
7. dscho@473ca8ae673
8. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110210947590.56@tvgsbejvaqbjf.bet/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
0 commit comments